Adding event dispatcher #763
Conversation
Codecov Report
@@ Coverage Diff @@
## main #763 +/- ##
============================================
- Coverage 82.86% 78.66% -4.20%
- Complexity 1295 1348 +53
============================================
Files 150 156 +6
Lines 3192 3314 +122
============================================
- Hits 2645 2607 -38
- Misses 547 707 +160
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
I don't know what the spec requirements that should be in that case. However I see the spec as a minimum requirement which must be fulfilled to get to the fun parts. Using an event listener for logs would allow for more fine grained control, but this could still be implemented for (additional logs) in the future. |
|
After reading through the CloudEvents spec: I think that using PSR-14 (as initially done in #708) makes more sense, it is widely used and would allow users to use a single event system in their application. CloudEvents main goal seems to be propagation between independent applications; users can still create CloudEvents from PSR-14 events by registering a listener that converts and emits these events. Creating CloudEvents:
|
- add dispatcher interface - remove singleton - move some logic into methods - make generator-returning method private
@Nevay I thought that initially, too. But without an interface in PSR-14 for registering events in a listener, it wasn't possible to have our code register its own events in a user-provided PSR-14 implementation. Effectively, you could bring your own psr-14, but you'd then be responsible for knowing about and hooking up of our internal events, including future events that we come up with. I'm happy to be corrected here, though - we're all for standards and choice |
|
@tidal I think I've addressed all review comments now |
|
@brettmc |
There was a problem hiding this comment.
Effectively, you could bring your own psr-14, but you'd then be responsible for knowing about and hooking up of our internal events, including future events that we come up with.
I assumed that this event dispatcher was solely for user facing events. If priority handling between multiple dispatchers is not required we could use a composite dispatcher that contains our internal dispatcher and the user provided dispatcher(s). What kind of events/internal events are currently planned?
if we did have our own internal events (eg for metrics) [708]
For metrics it might make more sense to use the meter provider and emit metrics directly, partly because we cannot use asynchronous instruments otherwise.
| private static function createInstance(): self | ||
| { | ||
| $dispatcher = new self(); | ||
| Context::getCurrent()->with(self::getConstantKeyInstance(), $dispatcher)->activate(); |
There was a problem hiding this comment.
The "root" dispatcher should use a static instance and not the context; currently leaks the created scope.
Additionally leads to different behavior depending on when the dispatcher is retrieved for the first time as listeners will then be added to the same instance.
When discussing #708 in SIG last week, we decided that using it to perform logging was overkill (since psr-3 can provide enough control to meet the spec requirements).
This implements a simplified event dispatcher, based loosely on psr-14 and using
CloudEventsto represent events (as suggested during SIG)Replaces #708