Adjust actor event API after review #11649

rvagg · 2024-02-23T06:56:40Z

From review discussions in #11618, WIP

TipSetCid -> TipSetKey (this is still nillable on the filter because omitempty doesn't play as nice, but should confirm that this is reasonable given existing API precedents)
EmitterAddr -> Emitter
- Document that this will end up being an ID address in the future
Epoch -> Height
FromHeight and ToHeight take nillable abi.ChainEpochs (ints) that must be >= 0 when set
Heights are interpreted as being exact, there is no longer a "whatever you have"
- Documented that this will be strict in the future after we can say for sure whether or not we have those events
Removed Prefill option in SubscribeActorEvents API and collapsed the argument structure, FromHeight will do that work implicitly.
nil for FromHeight will:
- For GetActorEvents be interpreted to mean "current epoch"
- For SubscribeActorEvents will be interpreted to mean "next epoch" (TODO: confirm we want to do this because current impl I think will just do a prefill of current epoch)
nil for ToHeight will:
- For GetActorEvents be interpreted to mean "current epoch"
- For SubscribeActorEvents be interpreted to mean that there is no height-based stopping condition
EnableActorEventsAPI config option has been moved to a new ~~ActorEvents top-level section; could also just be Events as per previous discussions?~~ Events section. (future TODO of moving other events config options into there so it won't be lonely)
Halt SubscribeActorEvents when we ToHeight is set and we have reached that position; currently will just stay open giving you nothing.
~~Add Limit option to the filter to limit the number of epochs to process, counting from whatever the start epoch is calculated to be~~ (Discussed here, I'm not sure I see a use for this given the global limit that's in place and the way you can use From and To as a user).
Make send-buffer handling for SubscribeActorEvents smarter:
- Prefill should have a minimum rate that is acceptable to send events, abort if they are too slow (perhaps calculate based on the speed at which you receive an epoch—you should be able to catch up in reasonable time)
- Ongoing subscription shouldn't end up lagging but should be able to deal with large numbers of events per epoch
- Tests for all this fancy footwork

rvagg · 2024-02-26T05:14:51Z

node/impl/full/actor_events.go

-				}
+		var buffer []*types.ActorEvent
+		const α = 0.2                        // decay factor for the events per height EMA
+		var eventsPerHeightEma float64 = 256 // exponential moving average of events per height, initially guess at 256


One problem we encounter at this point is if we've spent a lot of time sending historical data and we have a lot of new data backed up and ready to go we may blow our budget (almost) immediately. We could be more forgiving up front (according to what terms? how complicated do we want to get?). Or we could just say "bad luck, you shouldn't have requested so much historical data, use GetActorEvents for that.

See above. IMO, we wait one block time (30s) then give up.

rvagg · 2024-02-26T10:30:04Z

@Stebalien I'm going to call this one "ready for review", but it's lacking some unit testing for the new stream backlog handling code (and maybe some other minor things). I don't want to invest in figuring out how to build something for that without getting agreement on the approach.

rvagg · 2024-02-26T10:30:42Z

Alternatively, we can just merge this in to #11618 and go from there. Whatever's easiest to review.

rvagg · 2024-02-26T10:32:22Z

chain/types/actor_event.go

-	// If `TipSetCid` is present in the filter criteria, then neither `FromEpoch` nor `ToEpoch` are allowed.
-	TipSetCid *cid.Cid `json:"tipsetCid,omitempty"`
+	// If `TipSetKey` is legt empty in the filter criteria, then neither `FromHeight` nor `ToHeight` are allowed.
+	TipSetKey *TipSetKey `json:"tipsetKey,omitempty"`


Pointer here may not be idiomatic; I just wanted to avoid an unnecessary [] in the json form, mainly. I think maybe if [] is omitted on the input side that it may still resolve to the zero-value here, I'm not sure.

Pointer is fine. I'm actually kind of surprised we don't marshal these to strings... but ok.

if [] is omitted on the input side that it may still resolve to the zero-value here, I'm not sure.

It will.

Stebalien

Ideally we'd be able to:

Query for historical events.
Send them.
Query for more.
Send them.
Etc...

Then, when we're all caught up, we'd subscribe. But that seems like a lot of work. Also, I assume the limits will prevent that from really being an issues (we should never have to send that many events.

Stebalien · 2024-02-27T00:19:04Z

chain/types/actor_event.go

-	// If `TipSetCid` is present in the filter criteria, then neither `FromEpoch` nor `ToEpoch` are allowed.
-	TipSetCid *cid.Cid `json:"tipsetCid,omitempty"`
+	// If `TipSetKey` is legt empty in the filter criteria, then neither `FromHeight` nor `ToHeight` are allowed.
+	TipSetKey *TipSetKey `json:"tipsetKey,omitempty"`


Pointer is fine. I'm actually kind of surprised we don't marshal these to strings... but ok.

Stebalien · 2024-02-27T00:20:19Z

chain/types/actor_event.go

-	// If `TipSetCid` is present in the filter criteria, then neither `FromEpoch` nor `ToEpoch` are allowed.
-	TipSetCid *cid.Cid `json:"tipsetCid,omitempty"`
+	// If `TipSetKey` is legt empty in the filter criteria, then neither `FromHeight` nor `ToHeight` are allowed.
+	TipSetKey *TipSetKey `json:"tipsetKey,omitempty"`


if [] is omitted on the input side that it may still resolve to the zero-value here, I'm not sure.

It will.

node/impl/full/actor_events.go

Stebalien · 2024-02-27T02:47:22Z

node/impl/full/actor_events.go

+			ticker := time.NewTicker(time.Second)
+			defer ticker.Stop()
+
+			const maxSlowTicks = 3 // slightly forgiving, allow 3 slow ticks (seconds) before giving up
+			slowTicks := 0
+			sentEvents := 0.0


Given that we're doing a single batch here, there's no need for a ticker. We can just have a global timeout.

(I mean, the right way to handle slow reads is in the JSON-RPC library... anyways)

Stebalien · 2024-02-27T02:48:12Z

node/impl/full/actor_events.go

-				}
+		var buffer []*types.ActorEvent
+		const α = 0.2                        // decay factor for the events per height EMA
+		var eventsPerHeightEma float64 = 256 // exponential moving average of events per height, initially guess at 256


See above. IMO, we wait one block time (30s) then give up.

Use BlockDelay as the window for receiving events on the SubscribeActorEvents channel. We expect the user to have received the initial batch of historical events (if any) in one block's time. For real-time events we expect them to not fall behind by roughly one block's time.

rvagg · 2024-03-01T06:39:54Z

Pretty rough coming up with nice unit tests for SubscribeActorEvents given they're time-based, but I think I got there.

In response to last round of feedback we have two things for keeping track of "are you getting the events fast enough":

Historical events must all be received within one block's time, you have ~30s to receive what you've asked for before we consider you delinquent (really, use GetActorEvents for historical stuff).
For real-time events we (conceptually) keep track of the oldest event you haven't yet received and if it takes you longer than a block's time to get it, then you're delinquent and you get disconnected.

Basically: don't fall more than ~one block's time behind in receiving your events and you'll be dine.

Bunch of related refactorings in the last commit.

rvagg · 2024-03-01T07:01:06Z

Going to take this back to #11618 and proceed from there

rvagg force-pushed the rvagg/actor-events-api-tweaks branch from 47548b4 to ac997e6 Compare February 23, 2024 11:46

Adjust actor event API after review

90f1988

rvagg force-pushed the rvagg/actor-events-api-tweaks branch from ac997e6 to 90f1988 Compare February 26, 2024 01:27

s/ActorEvents/Events/g in global config

0e20f14

rvagg force-pushed the rvagg/actor-events-api-tweaks branch from 538dd6f to 6a34c7d Compare February 26, 2024 04:17

Manage event sending rate for SubscribeActorEvents

8a3a461

rvagg force-pushed the rvagg/actor-events-api-tweaks branch from 6a34c7d to 8a3a461 Compare February 26, 2024 04:35

rvagg commented Feb 26, 2024

View reviewed changes

rvagg added 3 commits February 26, 2024 19:04

Terminate SubscribeActorEvents chan when at max height

28389ff

Document future API changes

d200a43

More clarity in actor event API docs

99ecc47

rvagg marked this pull request as ready for review February 26, 2024 10:28

rvagg requested a review from a team as a code owner February 26, 2024 10:28

rvagg requested review from snadrus and aarshkshah1992 and removed request for a team February 26, 2024 10:28

rvagg commented Feb 26, 2024

View reviewed changes

Stebalien reviewed Feb 27, 2024

View reviewed changes

rvagg force-pushed the rvagg/actor-events-api-tweaks branch from 31feee3 to 9fd3e02 Compare March 1, 2024 06:37

rvagg merged commit 4abc21e into rvagg/feat/builtin-actor-events Mar 1, 2024
89 of 90 checks passed

rvagg deleted the rvagg/actor-events-api-tweaks branch March 1, 2024 07:01

rvagg mentioned this pull request Mar 1, 2024

feat: events: Add Lotus APIs to consume smart contract and built-in actor events #11618

Merged

8 tasks

Fatman13 mentioned this pull request Mar 8, 2024

[venus] actor event API filecoin-project/venus#6282

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust actor event API after review #11649

Adjust actor event API after review #11649

rvagg commented Feb 23, 2024 •

edited

Loading

rvagg Feb 26, 2024

Stebalien Feb 27, 2024

rvagg commented Feb 26, 2024

rvagg commented Feb 26, 2024

rvagg Feb 26, 2024

Stebalien Feb 27, 2024

Stebalien Feb 27, 2024

Stebalien left a comment

Stebalien Feb 27, 2024

Stebalien Feb 27, 2024

Stebalien Feb 27, 2024

Stebalien Feb 27, 2024

Stebalien Feb 27, 2024

rvagg commented Mar 1, 2024

rvagg commented Mar 1, 2024

Adjust actor event API after review #11649

Adjust actor event API after review #11649

Conversation

rvagg commented Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvagg commented Feb 26, 2024

rvagg commented Feb 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvagg commented Mar 1, 2024

rvagg commented Mar 1, 2024

rvagg commented Feb 23, 2024 •

edited

Loading