[RUM-14242] Introduce event bus pattern for data processing#6
Conversation
|
|
Early review: scope: I think we could limit this one to EventManager introduction only. we could deal with assembly, trace processing, ... in subsequent PRs. API:
💭 I am expecting some classes to const eventManager = new EventManager();
const assembly = new Assembly(hooks, eventManager);
class Assembly {
constructor(...) {
this.eventManager.registerHandler({
canHandle: (event: Event) => event.status === RAW,
process: this.processRawEvent
})
}
}it could also allow one class to register multiple handlers which could provide some flexibility.
Any intent with differentiating handle and process?
I would like to introduce some lifecycle events at some point, like session expired / session renew. Do we think we should use the same event manager or split lifecycle and collected events? |
2c40382 to
aa8b185
Compare
bcaudan
left a comment
There was a problem hiding this comment.
What about using Event manager with CreateDummyEvent and sendEvent to have a first integration with it?
| export const EventTrack = { | ||
| LOG: 'logs', | ||
| RUM: 'rum', | ||
| TRACE: 'spans', | ||
| } as const; |
There was a problem hiding this comment.
💬 suggestion: what about using endpoint naming for consistency?
| export const EventTrack = { | |
| LOG: 'logs', | |
| RUM: 'rum', | |
| TRACE: 'spans', | |
| } as const; | |
| export const EventTrack = { | |
| LOGS: 'logs', | |
| RUM: 'rum', | |
| SPANS: 'spans', | |
| } as const; |
| export const EventType = { | ||
| LOG: 'log', | ||
| RUM: 'rum', | ||
| TRACE: 'span', | ||
| } as const; |
There was a problem hiding this comment.
💬 suggestion: does not seem to be used for now
| export const EventType = { | |
| LOG: 'log', | |
| RUM: 'rum', | |
| TRACE: 'span', | |
| } as const; |
| export interface CollectableEvent { | ||
| kind: 'collectable'; | ||
| track: (typeof EventTrack)[keyof typeof EventTrack]; | ||
| source: (typeof EventSource)[keyof typeof EventSource]; | ||
| status: (typeof EventStatus)[keyof typeof EventStatus]; | ||
| data: unknown; | ||
| } | ||
|
|
||
| export interface LifetimeEvent { | ||
| kind: 'lifetime'; | ||
| data: unknown; | ||
| } |
There was a problem hiding this comment.
💬 suggestion: instead of kind and status, what about only using kind? not sure that Raw and Server event need to share the same properties
RawEvent {
kind: 'raw'
source
data
}
ServerEvent {
kind: 'server'
track
data
}
LifetimeEvent {
kind: 'lifetime'
data
}
| export interface LifetimeEvent { | ||
| kind: 'lifetime'; |
There was a problem hiding this comment.
💬 suggestion: In browser sdk, we are using the LifeCycle naming which seems more appropriate to me than Lifetime for events like "end user activity", session renew", ...
| continue; | ||
| } | ||
|
|
||
| handler.handle(event, (e) => this.notify(e)); |
There was a problem hiding this comment.
💬 suggestion: it is not obvious to me that we already need to introduce:
- handling of multiple events though notify
- calling notify from the handler
I think we could keep things simple at first, see how we end up using it and add more complexity along the way
There was a problem hiding this comment.
I don't think having a bit more flexibility in the implementation at this point is an issue. As for the notify from the handler, that goes allow what we discussed previously on how data should flow, I could have misunderstood something, but then we should resync to make sure were aligned on what we're aiming for.
aa8b185 to
2a17405
Compare
- Add Assembly to transform RawEvents into ServerEvents - Convert Transport to class that listens for ServerEvents - Convert DummyMainView to class that emits RawEvents - Connect components via EventManager
2a17405 to
042ef39
Compare
Motivation
Create an event management system to handle the various types of events required by the SDK where all the main event handlers are decoupled from each other and self select which events to handle.
Communication happens through a central hub that acts as dispatch point.
Changes
Adds
EventManagerclass and defines the required types and constants necessary for its use.Test instructions
In order to use this system, we must first instantiate the
EventManagerand the required handler classes like so:Checklist