feat(events): Add EventDispatcher and related classes
#138
Conversation
| } | ||
|
|
||
| const DEFAULT_EVENT_DISPATCHER_CONFIG = { | ||
| // TODO: Replace with actual ingestion URL |
There was a problem hiding this comment.
this will be handled in a follow up PR by parsing it from the SDK key
|
|
||
| constructor({ | ||
| eventQueue = new ArrayBackedNamedEventQueue('events'), | ||
| eventDispatcher = new NoOpEventDispatcher(), |
There was a problem hiding this comment.
default implementation is a no-op if none provided
| // noinspection JSUnusedGlobalSymbols | ||
| track(event: unknown, params: Record<string, unknown>) { | ||
| this.eventQueue.push({ event, params }); | ||
| this.eventDispatcher.dispatch({ id: randomUUID(), data: event, params }); |
There was a problem hiding this comment.
use a random uuid for avoiding duplicates server side
There was a problem hiding this comment.
I was thinking the payload would contain 4 fields:
- UUID
- timestamp
- event type (hard-coded/not modifiable without SDK code changes)
- payload
What is intended use of data vs params here?
There was a problem hiding this comment.
What is intended use of data vs params here?
That's pretty arbitrary and will change. I wouldn't worry too much about it just yet, since we're not using the actual types here, it's a placeholder.
| if (attempt < maxRetries) { | ||
| return this.retry(batch, attempt + 1); | ||
| } else { | ||
| // TODO: Persist batch to avoid data loss |
There was a problem hiding this comment.
will also do this in a future PR
| "pino": "^8.19.0", | ||
| "semver": "^7.5.4" | ||
| "semver": "^7.5.4", | ||
| "uuid": "^8.3.2" |
There was a problem hiding this comment.
this is a fairly old version of uuid (4 years old) but the last one that's compatible with our module settings
There was a problem hiding this comment.
if we don't want to add an additional dependency, we can delegate the event ID generation to consuming libraries (js-client, node, etc)
There was a problem hiding this comment.
Are you hoping to use uuid v7 because of this https://buildkite.com/resources/blog/goodbye-integers-hello-uuids/ or another feature?
There was a problem hiding this comment.
unfortunately we can't use uuid v7 because it's not available in the version of the library we're using. looks like it was added on uuid v10. however yes, that could be a pretty decent primary key for these events. I don't want to make any assumptions here, the only goal was to have a unique identifier and we can't use sequential IDs reliably because we can't guarantee the sequence ordering (we'd have to persist additional state for the last ID used, which is inconvenient).
| "sourceMap": true, | ||
| "allowSyntheticDefaultImports": true, | ||
| "target": "es2017", | ||
| "target": "es2020", |
| expect(batch).toEqual([ | ||
| { id: 'foo-1', data: 'event1', params: {} }, | ||
| { id: 'foo-2', data: 'event2', params: {} }, | ||
| ]); |
|
this was co-authored with chatgpt 😂 |
src/client/eppo-client.ts
Outdated
| ingestionUrl: 'https://example.com/events', | ||
| batchSize: 10, | ||
| flushInterval: 10_000, | ||
| retryInterval: 5_000, |
There was a problem hiding this comment.
Can we include the unit in this (and flush interval), so retryIntervalMs and flushIntervalMs?
| // noinspection JSUnusedGlobalSymbols | ||
| track(event: unknown, params: Record<string, unknown>) { | ||
| this.eventQueue.push({ event, params }); | ||
| this.eventDispatcher.dispatch({ id: randomUUID(), data: event, params }); |
There was a problem hiding this comment.
I was thinking the payload would contain 4 fields:
- UUID
- timestamp
- event type (hard-coded/not modifiable without SDK code changes)
- payload
What is intended use of data vs params here?
| /** | ||
| * @internal | ||
| * A named event queue backed by an **unbounded** array. | ||
| * This class probably should NOT be used directly, but only as a backing store for |
There was a problem hiding this comment.
Since this shouldn't ever be used directly, I think this should be built in to BoundedEventQueue and not a separate class. We can create it as a separate class it/when we need BoundedEventQueue to support a different backing store.
There was a problem hiding this comment.
BoundedEventQueue uses the NamedEventQueue interface, which is also used by BatchEventProcessor.
ArrayBackedNamedEventQueue is also useful for tests, I think it's a useful abstraction that I'd like to keep around for internal use.
I'm gonna remove it from the exports in index.ts, so no consumers should be able to use it. Let me know if that doesn't address your concerns about it
| maxRetries: config.maxRetries || 3, | ||
| }); | ||
| this.deliveryIntervalMs = config.deliveryIntervalMs; | ||
| this.networkStatusListener.onNetworkStatusChange((isOffline) => { |
There was a problem hiding this comment.
I don't see this listener getting invoked anywhere - how will this work?
There was a problem hiding this comment.
The concrete implementations for each platform will be provided by consuming libraries (node-sdk and js-client-sdk)
Towards: FF-3567
Motivation and Context
Add ability to track and deliver arbitrary events to an ingestion URL
Description
EventDispatcherto encapsulate event queueing and delivery attemptsBatchRetryManagerwith configurable settingsNetworkStatusListenerinterface (implementation will be platform specific) to handle online/offline modeHow has this been tested?
Wrote tests