Skip to content

Conversation

@protolambda
Copy link
Contributor

Description

This PR:

  • Introduces event.System, an interface that allows:
    • To register/unregister Deriver / Emitter
    • If registered, the emitter can be used to emit events, while tagging the events for tracing purposes.
    • Individual emitters can be rate-limited, to avoid any one sub-component from running too hot.
    • Running as deriver is optional. Emitters do not have to process events. If it does, then tracers can connect emitted events to the deriver execution they were emitted from.
  • Refactors event.Queue into event.GlobalSyncExec: implements the new event.Executor interface. The system uses an executor to queue up the emitted events to, and run the derivers with. This implementation is a synchronous queue, but more implementations are coming. (see parallel events processing work).
    • Note: "event draining" is separated from the event-emission, since it depends on the executor how/when events are consumed.
  • Changes the Deriver interface to OnEvent(ev Event) bool. The new bool return value identifies whether it was an ignored pass-through (= false) or actually processed (= true). This helps with reducing noise in event tracing. Later on we can maybe also stop the System from running an event-type that was previously ignored.

Tests

  • Executor/System have unit tests
  • Event system is integrated into all op-e2e & action tests.

Additional context

This could be the start of more runtime customization; adding/removing derivers is really easy, allowing customization by composing the system differently. Perhaps a plugin system can be built in the future on this.

Metadata

Fix #11097

@protolambda
Copy link
Contributor Author

protolambda commented Jul 8, 2024

TODO: breaking op-e2e tests: a few test cases seem to hot-loop on a temporary error after system shutdown, when it is supposed to stop processing events. The event-system should stop the Drain() from running when it is closed. The synchronous-event-executor now has a context that is canceled on shutdown of the driver.

Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just the one question about thread safety when iterating a slice that may be modified from another thread at the same time.

Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think that's nicer. Not as elegant in some ways but much easier to understand the setup code.

@ajsutton ajsutton added this pull request to the merge queue Jul 9, 2024
Merged via the queue into develop with commit 029fb1c Jul 9, 2024
@ajsutton ajsutton deleted the event-system branch July 9, 2024 07:19
@zhiqiangxu
Copy link
Contributor

zhiqiangxu commented Sep 6, 2024

@protolambda While reading code of PipelineDeriver, I spot a problem: do you think it's better to remove the AttachEmitter method and move the event.Emitter to the constructor instead? This makes it more explicit that event.Emitter is a required parameter instead of an optional one, as the OnEvent method assumes event.Emitter is already set.

This question also applies to other XXXDeriver.

@protolambda
Copy link
Contributor Author

The AttachEmitter choice is deliberate, since otherwise it makes it difficult to register the deriver with the event system, as the emitter is a unique instance created for the deriver.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Interop: clean up event deriver/emitter system

3 participants