Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
2 changes: 1 addition & 1 deletion 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
31 changes: 24 additions & 7 deletions src/client/eppo-client.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { randomUUID } from 'crypto';

import ApiEndpoints from '../api-endpoints';
import { logger } from '../application-logger';
import { IAssignmentEvent, IAssignmentLogger } from '../assignment-logger';
Expand All @@ -21,7 +23,10 @@ import { EppoValue } from '../eppo_value';
import { Evaluator, FlagEvaluation, noneResult } from '../evaluator';
import ArrayBackedNamedEventQueue from '../events/array-backed-named-event-queue';
import { BoundedEventQueue } from '../events/bounded-event-queue';
import DefaultEventDispatcher from '../events/default-event-dispatcher';
import EventDispatcher from '../events/event-dispatcher';
import NamedEventQueue from '../events/named-event-queue';
import NoOpEventDispatcher from '../events/no-op-event-dispatcher';
import {
FlagEvaluationDetailsBuilder,
IFlagEvaluationDetails,
Expand Down Expand Up @@ -77,8 +82,17 @@ export interface IContainerExperiment<T> {
treatmentVariationEntries: Array<T>;
}

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,
flushInterval: 10_000,
retryInterval: 5_000,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we include the unit in this (and flush interval), so retryIntervalMs and flushIntervalMs?

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 +113,23 @@ export default class EppoClient {
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 +923,10 @@ export default class EppoClient {
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 +944,9 @@ export default class EppoClient {
return {
configFetchedAt: this.flagConfigurationStore.getConfigFetchedAt() ?? '',
configPublishedAt: this.flagConfigurationStore.getConfigPublishedAt() ?? '',
configEnvironment: this.flagConfigurationStore.getEnvironment() ?? { name: '' },
configEnvironment: this.flagConfigurationStore.getEnvironment() ?? {
name: '',
},
};
}

Expand Down
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