Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ module.exports = {
],
'import/named': 'off',
'import/no-unresolved': 'off',
'@typescript-eslint/no-explicit-any': 'off',
'import/order': [
'warn',
{
Expand All @@ -33,7 +34,7 @@ module.exports = {
group: 'parent',
position: 'before',
},
],
],
groups: ['builtin', 'external', 'parent', 'sibling', 'index'],
'newlines-between': 'always',
alphabetize: {
Expand Down
6 changes: 4 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@eppo/js-client-sdk-common",
"version": "4.3.0",
"version": "4.4.0",
"description": "Eppo SDK for client-side JavaScript applications (base for both web and react native)",
"main": "dist/index.js",
"files": [
Expand Down Expand Up @@ -46,6 +46,7 @@
"@types/lodash": "^4.17.5",
"@types/md5": "^2.3.2",
"@types/semver": "^7.5.6",
"@types/uuid": "^10.0.0",
"@typescript-eslint/eslint-plugin": "^5.13.0",
"@typescript-eslint/parser": "^5.13.0",
"eslint": "^8.17.0",
Expand All @@ -71,7 +72,8 @@
"js-base64": "^3.7.7",
"md5": "^2.3.0",
"pino": "^8.19.0",
"semver": "^7.5.4"
"semver": "^7.5.4",
"uuid": "^8.3.2"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a fairly old version of uuid (4 years old) but the last one that's compatible with our module settings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we don't want to add an additional dependency, we can delegate the event ID generation to consuming libraries (js-client, node, etc)

Copy link
Member

Choose a reason for hiding this comment

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

Are you hoping to use uuid v7 because of this https://buildkite.com/resources/blog/goodbye-integers-hello-uuids/ or another feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

},
"packageManager": "[email protected]+sha512.a6b2f7906b721bba3d67d4aff083df04dad64c399707841b7acf00f6b133b7ac24255f2652fa22ae3534329dc6180534e98d17432037ff6fd140556e2bb3137e"
}
33 changes: 24 additions & 9 deletions src/client/eppo-client.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { v4 as randomUUID } from 'uuid';

import ApiEndpoints from '../api-endpoints';
import { logger } from '../application-logger';
import { IAssignmentEvent, IAssignmentLogger } from '../assignment-logger';
Expand All @@ -21,7 +23,8 @@
import { Evaluator, FlagEvaluation, noneResult } from '../evaluator';
import ArrayBackedNamedEventQueue from '../events/array-backed-named-event-queue';
import { BoundedEventQueue } from '../events/bounded-event-queue';
import NamedEventQueue from '../events/named-event-queue';
import EventDispatcher from '../events/event-dispatcher';
import NoOpEventDispatcher from '../events/no-op-event-dispatcher';
import {
FlagEvaluationDetailsBuilder,
IFlagEvaluationDetails,
Expand Down Expand Up @@ -76,9 +79,18 @@
controlVariationEntry: T;
treatmentVariationEntries: Array<T>;
}

Check warning on line 82 in src/client/eppo-client.ts

View workflow job for this annotation

GitHub Actions / lint-test-sdk (18)

'DEFAULT_EVENT_DISPATCHER_CONFIG' is assigned a value but never used

Check warning on line 82 in src/client/eppo-client.ts

View workflow job for this annotation

GitHub Actions / lint-test-sdk (20)

'DEFAULT_EVENT_DISPATCHER_CONFIG' is assigned a value but never used

Check warning on line 82 in src/client/eppo-client.ts

View workflow job for this annotation

GitHub Actions / lint-test-sdk (22)

'DEFAULT_EVENT_DISPATCHER_CONFIG' is assigned a value but never used

Check warning on line 82 in src/client/eppo-client.ts

View workflow job for this annotation

GitHub Actions / lint-test-sdk (23)

'DEFAULT_EVENT_DISPATCHER_CONFIG' is assigned a value but never used
const DEFAULT_EVENT_DISPATCHER_CONFIG = {
// TODO: Replace with actual ingestion URL
Copy link
Contributor Author

@felipecsl felipecsl Nov 22, 2024

Choose a reason for hiding this comment

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

this will be handled in a follow up PR by parsing it from the SDK key

ingestionUrl: 'https://example.com/events',
batchSize: 10,
flushIntervalMs: 10_000,
retryIntervalMs: 5_000,
maxRetries: 3,
};

export default class EppoClient {
private readonly eventQueue: NamedEventQueue<unknown>;
private readonly eventDispatcher: EventDispatcher;
private readonly assignmentEventsQueue: BoundedEventQueue<IAssignmentEvent> =
newBoundedArrayEventQueue<IAssignmentEvent>('assignments');
private readonly banditEventsQueue: BoundedEventQueue<IBanditEvent> =
Expand All @@ -99,23 +111,23 @@
private readonly evaluator = new Evaluator();

constructor({
eventQueue = new ArrayBackedNamedEventQueue('events'),
eventDispatcher = new NoOpEventDispatcher(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

default implementation is a no-op if none provided

isObfuscated = false,
flagConfigurationStore,
banditVariationConfigurationStore,
banditModelConfigurationStore,
configurationRequestParameters,
}: {
// Queue for arbitrary, application-level events (not to be confused with Eppo specific assignment
// Dispatcher for arbitrary, application-level events (not to be confused with Eppo specific assignment
// or bandit events). These events are application-specific and captures by EppoClient#track API.
eventQueue?: NamedEventQueue<unknown>;
eventDispatcher?: EventDispatcher;
flagConfigurationStore: IConfigurationStore<Flag | ObfuscatedFlag>;
banditVariationConfigurationStore?: IConfigurationStore<BanditVariation[]>;
banditModelConfigurationStore?: IConfigurationStore<BanditParameters>;
configurationRequestParameters?: FlagConfigurationRequestParameters;
isObfuscated?: boolean;
}) {
this.eventQueue = eventQueue;
this.eventDispatcher = eventDispatcher;
this.flagConfigurationStore = flagConfigurationStore;
this.banditVariationConfigurationStore = banditVariationConfigurationStore;
this.banditModelConfigurationStore = banditModelConfigurationStore;
Expand Down Expand Up @@ -909,9 +921,10 @@
return result;
}

/** TODO */
// noinspection JSUnusedGlobalSymbols
track(event: unknown, params: Record<string, unknown>) {
this.eventQueue.push({ event, params });
this.eventDispatcher.dispatch({ id: randomUUID(), data: event, params });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

use a random uuid for avoiding duplicates server side

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

private newFlagEvaluationDetailsBuilder(flagKey: string): FlagEvaluationDetailsBuilder {
Expand All @@ -929,7 +942,9 @@
return {
configFetchedAt: this.flagConfigurationStore.getConfigFetchedAt() ?? '',
configPublishedAt: this.flagConfigurationStore.getConfigPublishedAt() ?? '',
configEnvironment: this.flagConfigurationStore.getEnvironment() ?? { name: '' },
configEnvironment: this.flagConfigurationStore.getEnvironment() ?? {
name: '',
},
};
}

Expand Down Expand Up @@ -1128,6 +1143,6 @@
}
}

export function newBoundedArrayEventQueue<T>(name: string): BoundedEventQueue<T> {
function newBoundedArrayEventQueue<T>(name: string): BoundedEventQueue<T> {
return new BoundedEventQueue<T>(new ArrayBackedNamedEventQueue<T>(name));
}
5 changes: 4 additions & 1 deletion src/evaluator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,10 @@ export class Evaluator {
split: Split,
subjectKey: string,
expectedVariationType: VariationType | undefined,
): { flagEvaluationCode: FlagEvaluationCode; flagEvaluationDescription: string } => {
): {
flagEvaluationCode: FlagEvaluationCode;
flagEvaluationDescription: string;
} => {
if (!checkValueTypeMatch(expectedVariationType, variation.value)) {
const { key: vKey, value: vValue } = variation;
return {
Expand Down
15 changes: 12 additions & 3 deletions src/events/array-backed-named-event-queue.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
import NamedEventQueue from './named-event-queue';

/** A named event queue backed by an array. */
/**
* @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
Copy link
Contributor

@petzel petzel Nov 25, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

* {@link BoundedEventQueue}.
*/
export default class ArrayBackedNamedEventQueue<T> implements NamedEventQueue<T> {
private readonly events: T[] = [];

Expand All @@ -22,7 +27,11 @@ export default class ArrayBackedNamedEventQueue<T> implements NamedEventQueue<T>
return this.events[Symbol.iterator]();
}

shift(): T | undefined {
return this.events.shift();
splice(count: number): T[] {
return this.events.splice(0, count);
}

isEmpty(): boolean {
return this.events.length === 0;
}
}
24 changes: 24 additions & 0 deletions src/events/batch-event-processor.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import ArrayBackedNamedEventQueue from './array-backed-named-event-queue';
import BatchEventProcessor from './batch-event-processor';

describe('BatchEventProcessor', () => {
describe('nextBatch', () => {
it('should return a batch and remove items from the queue', () => {
const eventQueue = new ArrayBackedNamedEventQueue('test-queue');
const processor = new BatchEventProcessor(eventQueue, 2);
expect(processor.isEmpty()).toBeTruthy();
expect(processor.nextBatch()).toHaveLength(0);
processor.push({ id: 'foo-1', data: 'event1', params: {} });
processor.push({ id: 'foo-2', data: 'event2', params: {} });
processor.push({ id: 'foo-3', data: 'event3', params: {} });
expect(processor.isEmpty()).toBeFalsy();
const batch = processor.nextBatch();
expect(batch).toEqual([
{ id: 'foo-1', data: 'event1', params: {} },
{ id: 'foo-2', data: 'event2', params: {} },
]);
Comment on lines +16 to +19
Copy link
Member

Choose a reason for hiding this comment

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

nice

expect(processor.nextBatch()).toEqual([{ id: 'foo-3', data: 'event3', params: {} }]);
expect(processor.isEmpty()).toBeTruthy();
});
});
});
20 changes: 20 additions & 0 deletions src/events/batch-event-processor.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import NamedEventQueue from './named-event-queue';

export default class BatchEventProcessor {
constructor(
private readonly eventQueue: NamedEventQueue<unknown>,
private readonly batchSize: number,
) {}

nextBatch(): unknown[] {
return this.eventQueue.splice(this.batchSize);
}

push(event: unknown): void {
this.eventQueue.push(event);
}

isEmpty(): boolean {
return this.eventQueue.isEmpty();
}
}
44 changes: 44 additions & 0 deletions src/events/batch-retry-manager.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { logger } from '../application-logger';

import EventDelivery from './event-delivery';

/**
* Attempts to retry delivering a batch of events to the ingestionUrl up to `maxRetries` times
* using exponential backoff.
*/
export default class BatchRetryManager {
/**
* @param config.retryInterval - The minimum retry interval in milliseconds
* @param config.maxRetryDelayMs - The maximum retry delay in milliseconds
* @param config.maxRetries - The maximum number of retries
*/
constructor(
private readonly delivery: EventDelivery,
private readonly config: {
retryIntervalMs: number;
maxRetryDelayMs: number;
maxRetries: number;
},
) {}

async retry(batch: unknown[], attempt = 0): Promise<void> {
const { retryIntervalMs, maxRetryDelayMs, maxRetries } = this.config;
const delay = Math.min(retryIntervalMs * Math.pow(2, attempt), maxRetryDelayMs);
logger.info(`[BatchRetryManager] Retrying batch delivery in ${delay}ms...`);
await new Promise((resolve) => setTimeout(resolve, delay));

const success = await this.delivery.deliver(batch);
if (success) {
logger.info(`[BatchRetryManager] Batch delivery successfully after ${attempt} retries.`);
return;
}
if (attempt < maxRetries) {
return this.retry(batch, attempt + 1);
} else {
// TODO: Persist batch to avoid data loss
Copy link
Contributor Author

Choose a reason for hiding this comment

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

will also do this in a future PR

logger.warn(
`[BatchRetryManager] Failed to deliver batch after ${maxRetries} retries, bailing`,
);
}
}
}
5 changes: 0 additions & 5 deletions src/events/bounded-event-queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,4 @@ export class BoundedEventQueue<T> {
this.queue.length = 0;
return events;
}

/** Returns the first event in the queue and removes it. */
shift(): T | undefined {
return this.queue.shift();
}
}
Loading
Loading