Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 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.0.2",
"version": "4.1.2",
"description": "Eppo SDK for client-side JavaScript applications (base for both web and react native)",
"main": "dist/index.js",
"files": [
Expand Down
21 changes: 11 additions & 10 deletions src/cache/abstract-assignment-cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,28 +2,29 @@ import { getMD5Hash } from '../obfuscation';

import { LRUCache } from './lru-cache';

export type AssignmentCacheValue = {
allocationKey: string;
variationKey: string;
};

/**
* Assignment cache keys are only on the subject and flag level, while the entire value is used
* for uniqueness checking. This way that if an assigned variation or bandit action changes for a
* flag, it evicts the old one. Then, if an older assignment is later reassigned, it will be treated
* as new.
*/
Comment on lines +5 to +10
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rasendubi moved comment here

export type AssignmentCacheKey = {
subjectKey: string;
flagKey: string;
};

export type AssignmentCacheValue = Record<string, string>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rasendubi & @felipecsl cache is now generic

Copy link
Member

Choose a reason for hiding this comment

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

We will lose type-safety by using generic Record. Would using generics be possible?

export type CacheKeyPair<T extends string, U extends string> = {
  [K in T]: string;
} & {
  [K in U]: string;
};

export type AssignmentCacheValue = CacheKeyPair<'allocationKey', 'variationKey'>;
export type BanditCacheValue = CacheKeyPair<'banditKey', 'actionKey'>;

// Usage example
const assignmentCache: AssignmentCacheValue = {
  allocationKey: 'someAllocationKey',
  variationKey: 'someVariationKey'
};

const banditCache: BanditCacheValue = {
  banditKey: 'someBanditKey',
  actionKey: 'someActionKey'
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh this is an interesting thought! I think this would be possible, but I don't know if we need to be not generic. If they send in something other than bandit key and action key, we'd still hash it and use that to define uniqueness for the entry, which I think is ok. Keeps it simpler and makes it more extensible if we need to cache other stuff going forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Paired on this and got 80% the way there; will merge then possibly go for the other 20% later.


export type AssignmentCacheEntry = AssignmentCacheKey & AssignmentCacheValue;

/** Converts an {@link AssignmentCacheKey} to a string. */
export function assignmentCacheKeyToString({ subjectKey, flagKey }: AssignmentCacheKey): string {
return getMD5Hash([subjectKey, flagKey].join(';'));
}

export function assignmentCacheValueToString({
allocationKey,
variationKey,
}: AssignmentCacheValue): string {
return getMD5Hash([allocationKey, variationKey].join(';'));
/** Converts an {@link AssignmentCacheValue} to a string. */
export function assignmentCacheValueToString(cacheValue: AssignmentCacheValue): string {
return getMD5Hash(Object.values(cacheValue).join(';'));
}

export interface AsyncMap<K, V> {
Expand Down
153 changes: 151 additions & 2 deletions src/client/eppo-client-with-bandits.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@ import {
} from '../../test/testHelpers';
import ApiEndpoints from '../api-endpoints';
import { IAssignmentEvent, IAssignmentLogger } from '../assignment-logger';
import { BanditEvaluator } from '../bandit-evaluator';
import { BanditEvaluation, BanditEvaluator } from '../bandit-evaluator';
import { IBanditEvent, IBanditLogger } from '../bandit-logger';
import ConfigurationRequestor from '../configuration-requestor';
import { MemoryOnlyConfigurationStore } from '../configuration-store/memory.store';
import { Evaluator, FlagEvaluation } from '../evaluator';
import {
AllocationEvaluationCode,
IFlagEvaluationDetails,
Expand All @@ -20,7 +21,7 @@ import FetchHttpClient from '../http-client';
import { BanditVariation, BanditParameters, Flag } from '../interfaces';
import { Attributes, ContextAttributes } from '../types';

import EppoClient from './eppo-client';
import EppoClient, { IAssignmentDetails } from './eppo-client';

describe('EppoClient Bandits E2E test', () => {
const flagStore = new MemoryOnlyConfigurationStore<Flag>();
Expand Down Expand Up @@ -204,6 +205,8 @@ describe('EppoClient Bandits E2E test', () => {
});

it('Flushed queued logging events when a logger is set', () => {
client.useLRUInMemoryAssignmentCache(5);
client.useLRUInMemoryBanditAssignmentCache(5);
client.setAssignmentLogger(null as unknown as IAssignmentLogger);
client.setBanditLogger(null as unknown as IBanditLogger);
const banditAssignment = client.getBanditAction(
Expand All @@ -220,6 +223,20 @@ describe('EppoClient Bandits E2E test', () => {
expect(mockLogAssignment).not.toHaveBeenCalled();
expect(mockLogBanditAction).not.toHaveBeenCalled();

const repeatAssignment = client.getBanditAction(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rasendubi adjusted test that flushes events to also ensure they are deduped as well (verified later with the .toHaveBeenCalledTimes(1) check)

flagKey,
subjectKey,
subjectAttributes,
actions,
'control',
);

expect(repeatAssignment.variation).toBe('banner_bandit');
expect(repeatAssignment.action).toBe('adidas');

expect(mockLogAssignment).not.toHaveBeenCalled();
expect(mockLogBanditAction).not.toHaveBeenCalled();

client.setAssignmentLogger({ logAssignment: mockLogAssignment });
client.setBanditLogger({ logBanditAction: mockLogBanditAction });

Expand Down Expand Up @@ -429,5 +446,137 @@ describe('EppoClient Bandits E2E test', () => {
expect(mockLogBanditAction.mock.calls[1][0].actionProbability).toBeCloseTo(0.256);
});
});

describe('Assignment logging deduplication', () => {
let mockEvaluateFlag: jest.SpyInstance;
let mockEvaluateBandit: jest.SpyInstance;
// The below two variables allow easily changing what the mock evaluation functions return throughout the test
let variationToReturn: string;
let actionToReturn: string | null;

// Convenience method for repeatedly making the exact same assignment call
function requestClientBanditAction(): Omit<IAssignmentDetails<string>, 'evaluationDetails'> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find function more descriptive than const, and since I don't need lexical this nor does this being hoisted matter, I'm thus opting to go for it.

return client.getBanditAction(
flagKey,
subjectKey,
subjectAttributes,
['toyota', 'honda'],
'control',
);
}

beforeAll(() => {
mockEvaluateFlag = jest
.spyOn(Evaluator.prototype, 'evaluateFlag')
.mockImplementation(() => {
return {
flagKey,
subjectKey,
subjectAttributes,
allocationKey: 'mock-allocation',
variation: { key: variationToReturn, value: variationToReturn },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where variationToReturn is injected as the assigned variation for mocking Evaluator.evaluateFlag()

extraLogging: {},
doLog: true,
flagEvaluationDetails: {
flagEvaluationCode: 'MATCH',
flagEvaluationDescription: 'Mocked evaluation',
},
} as FlagEvaluation;
});

mockEvaluateBandit = jest
.spyOn(BanditEvaluator.prototype, 'evaluateBandit')
.mockImplementation(() => {
return {
flagKey,
subjectKey,
subjectAttributes: { numericAttributes: {}, categoricalAttributes: {} },
actionKey: actionToReturn,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where actionToReturn is injected as the assigned action for mocking BanditEvaluator.evaluateBandit()

actionAttributes: { numericAttributes: {}, categoricalAttributes: {} },
actionScore: 10,
actionWeight: 0.5,
gamma: 1.0,
optimalityGap: 5,
} as BanditEvaluation;
});
});

beforeEach(() => {
client.useNonExpiringInMemoryAssignmentCache();
client.useNonExpiringInMemoryBanditAssignmentCache();
});

afterEach(() => {
client.disableAssignmentCache();
client.disableBanditAssignmentCache();
});

afterAll(() => {
mockEvaluateFlag.mockClear();
mockEvaluateBandit.mockClear();
});

it('handles bandit actions appropriately', async () => {
// First assign to non-bandit variation
variationToReturn = 'non-bandit';
actionToReturn = null;
const firstNonBanditAssignment = requestClientBanditAction();

expect(firstNonBanditAssignment.variation).toBe('non-bandit');
expect(firstNonBanditAssignment.action).toBeNull();
expect(mockLogAssignment).toHaveBeenCalledTimes(1); // new variation assignment
expect(mockLogBanditAction).not.toHaveBeenCalled(); // no bandit assignment

// Assign bandit action
variationToReturn = 'banner_bandit';
actionToReturn = 'toyota';
const firstBanditAssignment = requestClientBanditAction();

expect(firstBanditAssignment.variation).toBe('banner_bandit');
expect(firstBanditAssignment.action).toBe('toyota');
expect(mockLogAssignment).toHaveBeenCalledTimes(2); // new variation assignment
expect(mockLogBanditAction).toHaveBeenCalledTimes(1); // new bandit assignment

// Repeat bandit action assignment
variationToReturn = 'banner_bandit';
actionToReturn = 'toyota';
const secondBanditAssignment = requestClientBanditAction();

expect(secondBanditAssignment.variation).toBe('banner_bandit');
expect(secondBanditAssignment.action).toBe('toyota');
expect(mockLogAssignment).toHaveBeenCalledTimes(2); // repeat variation assignment
expect(mockLogBanditAction).toHaveBeenCalledTimes(1); // repeat bandit assignment

// New bandit action assignment
variationToReturn = 'banner_bandit';
actionToReturn = 'honda';
const thirdBanditAssignment = requestClientBanditAction();

expect(thirdBanditAssignment.variation).toBe('banner_bandit');
expect(thirdBanditAssignment.action).toBe('honda');
expect(mockLogAssignment).toHaveBeenCalledTimes(2); // repeat variation assignment
expect(mockLogBanditAction).toHaveBeenCalledTimes(2); // new bandit assignment

// Flip-flop to an earlier action assignment
variationToReturn = 'banner_bandit';
actionToReturn = 'toyota';
const fourthBanditAssignment = requestClientBanditAction();

expect(fourthBanditAssignment.variation).toBe('banner_bandit');
expect(fourthBanditAssignment.action).toBe('toyota');
expect(mockLogAssignment).toHaveBeenCalledTimes(2); // repeat variation assignment
expect(mockLogBanditAction).toHaveBeenCalledTimes(3); // "new" bandit assignment

// Flip-flop back to non-bandit assignment
variationToReturn = 'non-bandit';
actionToReturn = null;
const secondNonBanditAssignment = requestClientBanditAction();

expect(secondNonBanditAssignment.variation).toBe('non-bandit');
expect(secondNonBanditAssignment.action).toBeNull();
expect(mockLogAssignment).toHaveBeenCalledTimes(3); // "new" variation assignment
expect(mockLogBanditAction).toHaveBeenCalledTimes(3); // no bandit assignment
});
});
});
});
63 changes: 49 additions & 14 deletions src/client/eppo-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ export default class EppoClient {
private banditLogger?: IBanditLogger;
private isGracefulFailureMode = true;
private assignmentCache?: AssignmentCache;
private banditAssignmentCache?: AssignmentCache;
private requestPoller?: IPoller;
private readonly evaluator = new Evaluator();
private readonly banditEvaluator = new BanditEvaluator();
Expand Down Expand Up @@ -651,16 +652,34 @@ export default class EppoClient {
}

private logBanditAction(banditEvent: IBanditEvent): void {
if (!this.banditLogger) {
// No bandit logger set; enqueue the event in case a logger is later set
if (this.queuedBanditEvents.length < MAX_EVENT_QUEUE_SIZE) {
this.queuedBanditEvents.push(banditEvent);
}
// First we check if this bandit action has been logged before
const subjectKey = banditEvent.subject;
const flagKey = banditEvent.featureFlag;
const banditKey = banditEvent.bandit;
const actionKey = banditEvent.action ?? '__eppo_no_action';

const banditAssignmentCacheProperties = {
flagKey,
subjectKey,
banditKey,
actionKey,
};

if (this.banditAssignmentCache?.has(banditAssignmentCacheProperties)) {
// Ignore repeat assignment
return;
}
// If here, we have a logger

// If here, we have a logger and a new assignment to be logged
try {
this.banditLogger.logBanditAction(banditEvent);
if (this.banditLogger) {
this.banditLogger.logBanditAction(banditEvent);
} else if (this.queuedBanditEvents.length < MAX_EVENT_QUEUE_SIZE) {
// If no logger defined, queue up the events (up to a max) to flush if a logger is later defined
this.queuedBanditEvents.push(banditEvent);
}
// Record in the assignment cache, if active, to deduplicate subsequent repeat assignments
this.banditAssignmentCache?.set(banditAssignmentCacheProperties);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rasendubi no early return before setting the cache entry--instead if-else if is used to handle the flow control for calling banditLogger

} catch (err) {
logger.warn('Error encountered logging bandit action', err);
}
Expand Down Expand Up @@ -904,6 +923,22 @@ export default class EppoClient {
this.assignmentCache = cache;
}

public disableBanditAssignmentCache() {
this.banditAssignmentCache = undefined;
}

public useNonExpiringInMemoryBanditAssignmentCache() {
this.banditAssignmentCache = new NonExpiringInMemoryAssignmentCache();
}

public useLRUInMemoryBanditAssignmentCache(maxSize: number) {
this.banditAssignmentCache = new LRUInMemoryAssignmentCache(maxSize);
}

public useCustomBanditAssignmentCache(cache: AssignmentCache) {
this.banditAssignmentCache = cache;
}

public setIsGracefulFailureMode(gracefulFailureMode: boolean) {
this.isGracefulFailureMode = gracefulFailureMode;
}
Expand Down Expand Up @@ -956,14 +991,14 @@ export default class EppoClient {
}
}

// assignment logger may be null while waiting for initialization
if (!this.assignmentLogger) {
this.queuedAssignmentEvents.length < MAX_EVENT_QUEUE_SIZE &&
this.queuedAssignmentEvents.push(event);
return;
}
try {
this.assignmentLogger.logAssignment(event);
if (this.assignmentLogger) {
this.assignmentLogger.logAssignment(event);
} else if (this.queuedAssignmentEvents.length < MAX_EVENT_QUEUE_SIZE) {
// assignment logger may be null while waiting for initialization, queue up events (up to a max)
// to be flushed when set
this.queuedAssignmentEvents.push(event);
}
this.assignmentCache?.set({
flagKey,
subjectKey,
Expand Down
Loading