-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Add public API for setting event dispatcher #139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
354d3a5
05f66aa
1abba22
11bd6b2
0bd1de8
66aa699
de38e1a
5b33e17
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,17 +4,32 @@ import { MAX_EVENT_QUEUE_SIZE } from '../constants'; | |
import NamedEventQueue from './named-event-queue'; | ||
|
||
/** A bounded event queue that drops events when it reaches its maximum size. */ | ||
export class BoundedEventQueue<T> { | ||
export class BoundedEventQueue<T> implements NamedEventQueue<T> { | ||
constructor( | ||
private readonly queue: NamedEventQueue<T>, | ||
readonly name: string, | ||
private readonly queue = new Array<T>(), | ||
private readonly maxSize = MAX_EVENT_QUEUE_SIZE, | ||
) {} | ||
|
||
length = this.queue.length; | ||
|
||
splice(count: number): T[] { | ||
return this.queue.splice(count); | ||
} | ||
|
||
isEmpty(): boolean { | ||
return this.queue.length === 0; | ||
} | ||
|
||
[Symbol.iterator](): IterableIterator<T> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your ruby is bleeding into typescript 😛 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lol this is actually part of the standard typescript API :) maybe they were inspired by ruby 🤣 |
||
return this.queue[Symbol.iterator](); | ||
} | ||
|
||
push(event: T) { | ||
if (this.queue.length < this.maxSize) { | ||
this.queue.push(event); | ||
} else { | ||
logger.warn(`Dropping event for queue ${this.queue.name} since the queue is full`); | ||
logger.warn(`Dropping event for queue ${this.name} since the queue is full`); | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,9 @@ | ||
import { resolve } from 'eslint-import-resolver-typescript'; | ||
|
||
import ArrayBackedNamedEventQueue from './array-backed-named-event-queue'; | ||
import BatchEventProcessor from './batch-event-processor'; | ||
import DefaultEventDispatcher, { EventDispatcherConfig } from './default-event-dispatcher'; | ||
import DefaultEventDispatcher, { | ||
EventDispatcherConfig, | ||
newDefaultEventDispatcher, | ||
} from './default-event-dispatcher'; | ||
import { Event } from './event-dispatcher'; | ||
import NetworkStatusListener from './network-status-listener'; | ||
|
||
|
@@ -10,7 +11,7 @@ | |
|
||
const mockNetworkStatusListener = { | ||
isOffline: () => false, | ||
onNetworkStatusChange: (_: (_: boolean) => void) => null as unknown as void, | ||
Check warning on line 14 in src/events/default-event-dispatcher.spec.ts
|
||
}; | ||
|
||
const createDispatcher = ( | ||
|
@@ -140,7 +141,7 @@ | |
describe('offline handling', () => { | ||
it('skips delivery when offline', async () => { | ||
let isOffline = false; | ||
let cb = (_: boolean) => null as unknown as void; | ||
Check warning on line 144 in src/events/default-event-dispatcher.spec.ts
|
||
const networkStatusListener = { | ||
isOffline: () => isOffline, | ||
onNetworkStatusChange: (callback: (isOffline: boolean) => void) => { | ||
|
@@ -164,7 +165,7 @@ | |
|
||
it('resumes delivery when back online', async () => { | ||
let isOffline = true; | ||
let cb = (_: boolean) => null as unknown as void; | ||
Check warning on line 168 in src/events/default-event-dispatcher.spec.ts
|
||
const networkStatusListener = { | ||
isOffline: () => isOffline, | ||
onNetworkStatusChange: (callback: (isOffline: boolean) => void) => { | ||
|
@@ -198,4 +199,26 @@ | |
expect(global.fetch).toHaveBeenCalled(); | ||
}); | ||
}); | ||
|
||
describe('newDefaultEventDispatcher', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🙌 |
||
it('should throw if SDK key is invalid', () => { | ||
expect(() => { | ||
newDefaultEventDispatcher( | ||
new ArrayBackedNamedEventQueue('test-queue'), | ||
mockNetworkStatusListener, | ||
'invalid-sdk-key', | ||
); | ||
}).toThrow('Unable to parse Event ingestion URL from SDK key'); | ||
}); | ||
|
||
it('should create a new DefaultEventDispatcher with the provided configuration', () => { | ||
const eventQueue = new ArrayBackedNamedEventQueue('test-queue'); | ||
const dispatcher = newDefaultEventDispatcher( | ||
eventQueue, | ||
mockNetworkStatusListener, | ||
'zCsQuoHJxVPp895.ZWg9MTIzNDU2LmUudGVzdGluZy5lcHBvLmNsb3Vk', | ||
); | ||
expect(dispatcher).toBeInstanceOf(DefaultEventDispatcher); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,9 @@ import BatchEventProcessor from './batch-event-processor'; | |
import BatchRetryManager from './batch-retry-manager'; | ||
import EventDelivery from './event-delivery'; | ||
import EventDispatcher, { Event } from './event-dispatcher'; | ||
import NamedEventQueue from './named-event-queue'; | ||
import NetworkStatusListener from './network-status-listener'; | ||
import SdkKeyDecoder from './sdk-key-decoder'; | ||
|
||
export type EventDispatcherConfig = { | ||
// target url to deliver events to | ||
|
@@ -19,6 +21,16 @@ export type EventDispatcherConfig = { | |
maxRetries?: number; | ||
}; | ||
|
||
// TODO: Have more realistic default batch size based on average event payload size once we have | ||
// more concrete data. | ||
export const DEFAULT_EVENT_DISPATCHER_BATCH_SIZE = 100; | ||
export const DEFAULT_EVENT_DISPATCHER_CONFIG: Omit<EventDispatcherConfig, 'ingestionUrl'> = { | ||
deliveryIntervalMs: 10_000, | ||
retryIntervalMs: 5_000, | ||
maxRetryDelayMs: 30_000, | ||
maxRetries: 3, | ||
}; | ||
|
||
/** | ||
* @internal | ||
* An {@link EventDispatcher} that, given the provided config settings, delivers events in batches | ||
|
@@ -37,6 +49,7 @@ export default class DefaultEventDispatcher implements EventDispatcher { | |
private readonly networkStatusListener: NetworkStatusListener, | ||
config: EventDispatcherConfig, | ||
) { | ||
this.ensureConfigFields(config); | ||
this.eventDelivery = new EventDelivery(config.ingestionUrl); | ||
this.retryManager = new BatchRetryManager(this.eventDelivery, { | ||
retryIntervalMs: config.retryIntervalMs, | ||
|
@@ -94,4 +107,39 @@ export default class DefaultEventDispatcher implements EventDispatcher { | |
this.dispatchTimer = setTimeout(() => this.deliverNextBatch(), this.deliveryIntervalMs); | ||
} | ||
} | ||
|
||
private ensureConfigFields(config: EventDispatcherConfig) { | ||
if (!config.ingestionUrl) { | ||
throw new Error('Missing required ingestionUrl in EventDispatcherConfig'); | ||
} | ||
if (!config.deliveryIntervalMs) { | ||
throw new Error('Missing required deliveryIntervalMs in EventDispatcherConfig'); | ||
} | ||
if (!config.retryIntervalMs) { | ||
throw new Error('Missing required retryIntervalMs in EventDispatcherConfig'); | ||
} | ||
if (!config.maxRetryDelayMs) { | ||
throw new Error('Missing required maxRetryDelayMs in EventDispatcherConfig'); | ||
} | ||
} | ||
Comment on lines
+112
to
+124
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor, but this could probably be made into a loop to simplify. |
||
} | ||
|
||
/** Creates a new {@link DefaultEventDispatcher} with the provided configuration. */ | ||
export function newDefaultEventDispatcher( | ||
eventQueue: NamedEventQueue<unknown>, | ||
networkStatusListener: NetworkStatusListener, | ||
sdkKey: string, | ||
batchSize: number = DEFAULT_EVENT_DISPATCHER_BATCH_SIZE, | ||
config: Omit<EventDispatcherConfig, 'ingestionUrl'> = DEFAULT_EVENT_DISPATCHER_CONFIG, | ||
): EventDispatcher { | ||
const sdkKeyDecoder = new SdkKeyDecoder(); | ||
const ingestionUrl = sdkKeyDecoder.decodeEventIngestionHostName(sdkKey); | ||
if (!ingestionUrl) { | ||
throw new Error('Unable to parse Event ingestion URL from SDK key'); | ||
} | ||
return new DefaultEventDispatcher( | ||
new BatchEventProcessor(eventQueue, batchSize), | ||
networkStatusListener, | ||
{ ...config, ingestionUrl }, | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1831,6 +1831,11 @@ eslint-plugin-promise@^6.0.0: | |
resolved "https://registry.npmjs.org/eslint-plugin-promise/-/eslint-plugin-promise-6.0.0.tgz" | ||
integrity sha512-7GPezalm5Bfi/E22PnQxDWH2iW9GTvAlUNTztemeHb6c1BniSyoeTrM87JkC0wYdi6aQrZX9p2qEiAno8aTcbw== | ||
|
||
eslint-plugin-unused-imports@^4.1.4: | ||
version "4.1.4" | ||
resolved "https://registry.yarnpkg.com/eslint-plugin-unused-imports/-/eslint-plugin-unused-imports-4.1.4.tgz#62ddc7446ccbf9aa7b6f1f0b00a980423cda2738" | ||
integrity sha512-YptD6IzQjDardkl0POxnnRBhU1OEePMV0nd6siHaRBbd+lyh6NAhFEobiznKU7kTsSsDeSD62Pe7kAM1b7dAZQ== | ||
|
||
[email protected], eslint-scope@^5.1.1: | ||
version "5.1.1" | ||
resolved "https://registry.npmjs.org/eslint-scope/-/eslint-scope-5.1.1.tgz" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i really don't love these mutable setter APIs we have in the
EppoClient
, as a fan of immutable data types, i'd much rather do it like that, but I'm following the existing pattern hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, anything that won't toggle after instantion should be constructor options and immutable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that would be a bigger refactoring, so maybe we can consider for a future major release