From 6b8e782a91f3cd0ea59d46061caa79a53dd0df72 Mon Sep 17 00:00:00 2001 From: Aaron Silverman Date: Fri, 23 Aug 2024 16:32:12 -0600 Subject: [PATCH 01/10] test in place --- src/client/eppo-client-with-bandits.spec.ts | 128 +++++++++++++++++++- 1 file changed, 126 insertions(+), 2 deletions(-) diff --git a/src/client/eppo-client-with-bandits.spec.ts b/src/client/eppo-client-with-bandits.spec.ts index 500c523..f823649 100644 --- a/src/client/eppo-client-with-bandits.spec.ts +++ b/src/client/eppo-client-with-bandits.spec.ts @@ -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, @@ -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(); @@ -429,5 +430,128 @@ 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; + let variationToReturn: string; + let actionToReturn: string | null; + + // Convenience method for repeatedly making the exact same assignment call + function requestClientBanditAction(): Omit, 'evaluationDetails'> { + 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 }, + 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, + actionAttributes: { numericAttributes: {}, categoricalAttributes: {} }, + actionScore: 10, + actionWeight: 0.5, + gamma: 1.0, + optimalityGap: 5, + } as BanditEvaluation; + }); + }); + + beforeEach(() => { + client.useNonExpiringInMemoryAssignmentCache(); + }); + + afterEach(() => { + client.disableAssignmentCache(); + }); + + 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); + expect(mockLogBanditAction).not.toHaveBeenCalled(); + + // 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); + expect(mockLogBanditAction).toHaveBeenCalledTimes(1); + + // 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); + expect(mockLogBanditAction).toHaveBeenCalledTimes(1); + + // 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); + expect(mockLogBanditAction).toHaveBeenCalledTimes(2); + + // Flip-flop to an earlier action assignment + variationToReturn = 'banner_bandit'; + actionToReturn = 'toyota'; + const fourthBanditAssignment = requestClientBanditAction(); + + expect(fourthBanditAssignment.variation).toBe('banner_bandit'); + expect(thirdBanditAssignment.action).toBe('toyota'); + expect(mockLogAssignment).toHaveBeenCalledTimes(2); + expect(mockLogBanditAction).toHaveBeenCalledTimes(3); + + // 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); + expect(mockLogBanditAction).toHaveBeenCalledTimes(3); + }); + }); }); }); From f263b505f92d8883e1211fcb6058163b4d24581e Mon Sep 17 00:00:00 2001 From: Aaron Silverman Date: Mon, 26 Aug 2024 17:45:49 -0600 Subject: [PATCH 02/10] use an assignment cache to deduplicate bandit action assignments --- src/cache/abstract-assignment-cache.ts | 26 +++++++++++---- src/client/eppo-client-with-bandits.spec.ts | 3 +- src/client/eppo-client.ts | 37 +++++++++++++++++++++ 3 files changed, 59 insertions(+), 7 deletions(-) diff --git a/src/cache/abstract-assignment-cache.ts b/src/cache/abstract-assignment-cache.ts index 7581f06..94a0ab5 100644 --- a/src/cache/abstract-assignment-cache.ts +++ b/src/cache/abstract-assignment-cache.ts @@ -2,11 +2,18 @@ import { getMD5Hash } from '../obfuscation'; import { LRUCache } from './lru-cache'; -export type AssignmentCacheValue = { +export type FlagAssignmentCacheValue = { allocationKey: string; variationKey: string; }; +export type BanditAssignmentCacheValue = { + banditKey: string; + actionKey: string; +}; + +type AssignmentCacheValue = FlagAssignmentCacheValue | BanditAssignmentCacheValue; + export type AssignmentCacheKey = { subjectKey: string; flagKey: string; @@ -19,11 +26,18 @@ export function assignmentCacheKeyToString({ subjectKey, flagKey }: AssignmentCa return getMD5Hash([subjectKey, flagKey].join(';')); } -export function assignmentCacheValueToString({ - allocationKey, - variationKey, -}: AssignmentCacheValue): string { - return getMD5Hash([allocationKey, variationKey].join(';')); +export function assignmentCacheValueToString(cacheValue: AssignmentCacheValue): string { + const fieldsToHash: string[] = []; + + if ('allocationKey' in cacheValue && 'variationKey' in cacheValue) { + fieldsToHash.push(cacheValue.allocationKey, cacheValue.variationKey); + } + + if ('banditKey' in cacheValue && 'actionKey' in cacheValue) { + fieldsToHash.push(cacheValue.banditKey, cacheValue.actionKey); + } + + return getMD5Hash(fieldsToHash.join(';')); } export interface AsyncMap { diff --git a/src/client/eppo-client-with-bandits.spec.ts b/src/client/eppo-client-with-bandits.spec.ts index f823649..b18dd24 100644 --- a/src/client/eppo-client-with-bandits.spec.ts +++ b/src/client/eppo-client-with-bandits.spec.ts @@ -480,6 +480,7 @@ describe('EppoClient Bandits E2E test', () => { beforeEach(() => { client.useNonExpiringInMemoryAssignmentCache(); + client.useNonExpiringInMemoryBanditAssignmentCache(); }); afterEach(() => { @@ -538,7 +539,7 @@ describe('EppoClient Bandits E2E test', () => { const fourthBanditAssignment = requestClientBanditAction(); expect(fourthBanditAssignment.variation).toBe('banner_bandit'); - expect(thirdBanditAssignment.action).toBe('toyota'); + expect(fourthBanditAssignment.action).toBe('toyota'); expect(mockLogAssignment).toHaveBeenCalledTimes(2); expect(mockLogBanditAction).toHaveBeenCalledTimes(3); diff --git a/src/client/eppo-client.ts b/src/client/eppo-client.ts index 3927af7..ec12e43 100644 --- a/src/client/eppo-client.ts +++ b/src/client/eppo-client.ts @@ -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(); @@ -651,6 +652,25 @@ export default class EppoClient { } private logBanditAction(banditEvent: IBanditEvent): void { + const subjectKey = banditEvent.subject; + const flagKey = banditEvent.featureFlag; + const banditKey = banditEvent.bandit; + const actionKey = banditEvent.action ?? '__eppo_no_action'; + + // What our bandit assignment cache cares about for avoiding logging duplicate bandit assignments, + // if one is active. Like the flag assignment cache, entries are only stored for a given flag + // and subject. + const banditAssignmentCacheProperties = { + flagKey, + subjectKey, + banditKey, + actionKey, + }; + + if (this.banditAssignmentCache?.has(banditAssignmentCacheProperties)) { + return; + } + 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) { @@ -661,6 +681,7 @@ export default class EppoClient { // If here, we have a logger try { this.banditLogger.logBanditAction(banditEvent); + this.banditAssignmentCache?.set(banditAssignmentCacheProperties); } catch (err) { logger.warn('Error encountered logging bandit action', err); } @@ -904,6 +925,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; } From d4371b982ba67111c635fb23e44bb40c35ec5c9c Mon Sep 17 00:00:00 2001 From: Aaron Silverman Date: Mon, 26 Aug 2024 18:10:20 -0600 Subject: [PATCH 03/10] feedback from self-review of PR --- src/client/eppo-client-with-bandits.spec.ts | 26 +++++++++++---------- src/client/eppo-client.ts | 7 ++++-- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/src/client/eppo-client-with-bandits.spec.ts b/src/client/eppo-client-with-bandits.spec.ts index b18dd24..0c7fc05 100644 --- a/src/client/eppo-client-with-bandits.spec.ts +++ b/src/client/eppo-client-with-bandits.spec.ts @@ -434,6 +434,7 @@ describe('EppoClient Bandits E2E test', () => { 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; @@ -485,6 +486,7 @@ describe('EppoClient Bandits E2E test', () => { afterEach(() => { client.disableAssignmentCache(); + client.disableBanditAssignmentCache(); }); afterAll(() => { @@ -500,8 +502,8 @@ describe('EppoClient Bandits E2E test', () => { expect(firstNonBanditAssignment.variation).toBe('non-bandit'); expect(firstNonBanditAssignment.action).toBeNull(); - expect(mockLogAssignment).toHaveBeenCalledTimes(1); - expect(mockLogBanditAction).not.toHaveBeenCalled(); + expect(mockLogAssignment).toHaveBeenCalledTimes(1); // new variation assignment + expect(mockLogBanditAction).not.toHaveBeenCalled(); // no bandit assignment // Assign bandit action variationToReturn = 'banner_bandit'; @@ -510,8 +512,8 @@ describe('EppoClient Bandits E2E test', () => { expect(firstBanditAssignment.variation).toBe('banner_bandit'); expect(firstBanditAssignment.action).toBe('toyota'); - expect(mockLogAssignment).toHaveBeenCalledTimes(2); - expect(mockLogBanditAction).toHaveBeenCalledTimes(1); + expect(mockLogAssignment).toHaveBeenCalledTimes(2); // new variation assignment + expect(mockLogBanditAction).toHaveBeenCalledTimes(1); // new bandit assignment // Repeat bandit action assignment variationToReturn = 'banner_bandit'; @@ -520,8 +522,8 @@ describe('EppoClient Bandits E2E test', () => { expect(secondBanditAssignment.variation).toBe('banner_bandit'); expect(secondBanditAssignment.action).toBe('toyota'); - expect(mockLogAssignment).toHaveBeenCalledTimes(2); - expect(mockLogBanditAction).toHaveBeenCalledTimes(1); + expect(mockLogAssignment).toHaveBeenCalledTimes(2); // repeat variation assignment + expect(mockLogBanditAction).toHaveBeenCalledTimes(1); // repeat bandit assignment // New bandit action assignment variationToReturn = 'banner_bandit'; @@ -530,8 +532,8 @@ describe('EppoClient Bandits E2E test', () => { expect(thirdBanditAssignment.variation).toBe('banner_bandit'); expect(thirdBanditAssignment.action).toBe('honda'); - expect(mockLogAssignment).toHaveBeenCalledTimes(2); - expect(mockLogBanditAction).toHaveBeenCalledTimes(2); + expect(mockLogAssignment).toHaveBeenCalledTimes(2); // repeat variation assignment + expect(mockLogBanditAction).toHaveBeenCalledTimes(2); // new bandit assignment // Flip-flop to an earlier action assignment variationToReturn = 'banner_bandit'; @@ -540,8 +542,8 @@ describe('EppoClient Bandits E2E test', () => { expect(fourthBanditAssignment.variation).toBe('banner_bandit'); expect(fourthBanditAssignment.action).toBe('toyota'); - expect(mockLogAssignment).toHaveBeenCalledTimes(2); - expect(mockLogBanditAction).toHaveBeenCalledTimes(3); + expect(mockLogAssignment).toHaveBeenCalledTimes(2); // repeat variation assignment + expect(mockLogBanditAction).toHaveBeenCalledTimes(3); // "new" bandit assignment // Flip-flop back to non-bandit assignment variationToReturn = 'non-bandit'; @@ -550,8 +552,8 @@ describe('EppoClient Bandits E2E test', () => { expect(secondNonBanditAssignment.variation).toBe('non-bandit'); expect(secondNonBanditAssignment.action).toBeNull(); - expect(mockLogAssignment).toHaveBeenCalledTimes(3); - expect(mockLogBanditAction).toHaveBeenCalledTimes(3); + expect(mockLogAssignment).toHaveBeenCalledTimes(3); // "new" variation assignment + expect(mockLogBanditAction).toHaveBeenCalledTimes(3); // no bandit assignment }); }); }); diff --git a/src/client/eppo-client.ts b/src/client/eppo-client.ts index ec12e43..a58439d 100644 --- a/src/client/eppo-client.ts +++ b/src/client/eppo-client.ts @@ -659,7 +659,9 @@ export default class EppoClient { // What our bandit assignment cache cares about for avoiding logging duplicate bandit assignments, // if one is active. Like the flag assignment cache, entries are only stored for a given flag - // and subject. + // and subject. However, Bandit and action keys are also used for determining assignment uniqueness. + // This means that if a flag's bandit assigns one action, and then later a new action, the first + // one will be evicted from the cache. If later assigned again, it will be treated as new. const banditAssignmentCacheProperties = { flagKey, subjectKey, @@ -668,6 +670,7 @@ export default class EppoClient { }; if (this.banditAssignmentCache?.has(banditAssignmentCacheProperties)) { + // Ignore repeat assignment return; } @@ -678,7 +681,7 @@ export default class EppoClient { } 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); this.banditAssignmentCache?.set(banditAssignmentCacheProperties); From 651cb66019cc6dc354172c8d7a88f124b1ae3e5b Mon Sep 17 00:00:00 2001 From: Aaron Silverman Date: Mon, 26 Aug 2024 18:16:29 -0600 Subject: [PATCH 04/10] fix type and lint errors --- src/cache/abstract-assignment-cache.ts | 6 +++--- src/client/eppo-client-with-bandits.spec.ts | 8 +++++++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/cache/abstract-assignment-cache.ts b/src/cache/abstract-assignment-cache.ts index 94a0ab5..d1e5cf9 100644 --- a/src/cache/abstract-assignment-cache.ts +++ b/src/cache/abstract-assignment-cache.ts @@ -2,17 +2,17 @@ import { getMD5Hash } from '../obfuscation'; import { LRUCache } from './lru-cache'; -export type FlagAssignmentCacheValue = { +type FlagAssignmentCacheValue = { allocationKey: string; variationKey: string; }; -export type BanditAssignmentCacheValue = { +type BanditAssignmentCacheValue = { banditKey: string; actionKey: string; }; -type AssignmentCacheValue = FlagAssignmentCacheValue | BanditAssignmentCacheValue; +export type AssignmentCacheValue = FlagAssignmentCacheValue | BanditAssignmentCacheValue; export type AssignmentCacheKey = { subjectKey: string; diff --git a/src/client/eppo-client-with-bandits.spec.ts b/src/client/eppo-client-with-bandits.spec.ts index 0c7fc05..af6bc6a 100644 --- a/src/client/eppo-client-with-bandits.spec.ts +++ b/src/client/eppo-client-with-bandits.spec.ts @@ -440,7 +440,13 @@ describe('EppoClient Bandits E2E test', () => { // Convenience method for repeatedly making the exact same assignment call function requestClientBanditAction(): Omit, 'evaluationDetails'> { - return client.getBanditAction(flagKey, subjectKey, subjectAttributes, ['toyota', 'honda'], 'control'); + return client.getBanditAction( + flagKey, + subjectKey, + subjectAttributes, + ['toyota', 'honda'], + 'control', + ); } beforeAll(() => { From 899967fdaaa4792a61d101f2086e65f8b25c41e7 Mon Sep 17 00:00:00 2001 From: Aaron Silverman Date: Tue, 27 Aug 2024 11:19:21 -0600 Subject: [PATCH 05/10] bump minor version --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index af7b3fa..8e58079 100644 --- a/package.json +++ b/package.json @@ -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": [ From 74bcc0f2f0242657cf460c021449acc0c500f099 Mon Sep 17 00:00:00 2001 From: Aaron Silverman Date: Tue, 27 Aug 2024 14:53:21 -0600 Subject: [PATCH 06/10] just hash whole cache value --- src/cache/abstract-assignment-cache.ts | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/cache/abstract-assignment-cache.ts b/src/cache/abstract-assignment-cache.ts index d1e5cf9..5b8a501 100644 --- a/src/cache/abstract-assignment-cache.ts +++ b/src/cache/abstract-assignment-cache.ts @@ -26,18 +26,8 @@ export function assignmentCacheKeyToString({ subjectKey, flagKey }: AssignmentCa return getMD5Hash([subjectKey, flagKey].join(';')); } -export function assignmentCacheValueToString(cacheValue: AssignmentCacheValue): string { - const fieldsToHash: string[] = []; - - if ('allocationKey' in cacheValue && 'variationKey' in cacheValue) { - fieldsToHash.push(cacheValue.allocationKey, cacheValue.variationKey); - } - - if ('banditKey' in cacheValue && 'actionKey' in cacheValue) { - fieldsToHash.push(cacheValue.banditKey, cacheValue.actionKey); - } - - return getMD5Hash(fieldsToHash.join(';')); +export function assignmentCacheValueToString(cacheValue: Record): string { + return getMD5Hash(Object.values(cacheValue).join(';')); } export interface AsyncMap { From 8bc8abce42a4656c9365fcb775f4791f8a6d9f92 Mon Sep 17 00:00:00 2001 From: Aaron Silverman Date: Tue, 27 Aug 2024 15:02:39 -0600 Subject: [PATCH 07/10] feedback from PR --- src/cache/abstract-assignment-cache.ts | 23 ++++++++++------------- src/client/eppo-client.ts | 8 +++----- 2 files changed, 13 insertions(+), 18 deletions(-) diff --git a/src/cache/abstract-assignment-cache.ts b/src/cache/abstract-assignment-cache.ts index 5b8a501..125b6cc 100644 --- a/src/cache/abstract-assignment-cache.ts +++ b/src/cache/abstract-assignment-cache.ts @@ -2,23 +2,19 @@ import { getMD5Hash } from '../obfuscation'; import { LRUCache } from './lru-cache'; -type FlagAssignmentCacheValue = { - allocationKey: string; - variationKey: string; -}; - -type BanditAssignmentCacheValue = { - banditKey: string; - actionKey: string; -}; - -export type AssignmentCacheValue = FlagAssignmentCacheValue | BanditAssignmentCacheValue; - +/** + * 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. + */ export type AssignmentCacheKey = { subjectKey: string; flagKey: string; }; +export type AssignmentCacheValue = Record; + export type AssignmentCacheEntry = AssignmentCacheKey & AssignmentCacheValue; /** Converts an {@link AssignmentCacheKey} to a string. */ @@ -26,7 +22,8 @@ export function assignmentCacheKeyToString({ subjectKey, flagKey }: AssignmentCa return getMD5Hash([subjectKey, flagKey].join(';')); } -export function assignmentCacheValueToString(cacheValue: Record): string { +/** Converts an {@link AssignmentCacheValue} to a string. */ +export function assignmentCacheValueToString(cacheValue: AssignmentCacheValue): string { return getMD5Hash(Object.values(cacheValue).join(';')); } diff --git a/src/client/eppo-client.ts b/src/client/eppo-client.ts index a58439d..fb46a99 100644 --- a/src/client/eppo-client.ts +++ b/src/client/eppo-client.ts @@ -652,16 +652,12 @@ export default class EppoClient { } private logBanditAction(banditEvent: IBanditEvent): void { + // 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'; - // What our bandit assignment cache cares about for avoiding logging duplicate bandit assignments, - // if one is active. Like the flag assignment cache, entries are only stored for a given flag - // and subject. However, Bandit and action keys are also used for determining assignment uniqueness. - // This means that if a flag's bandit assigns one action, and then later a new action, the first - // one will be evicted from the cache. If later assigned again, it will be treated as new. const banditAssignmentCacheProperties = { flagKey, subjectKey, @@ -674,6 +670,7 @@ export default class EppoClient { return; } + // If no logger defined, queue up the events (up to a max) to flush if a logger is later defined 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) { @@ -681,6 +678,7 @@ export default class EppoClient { } return; } + // If here, we have a logger and a new assignment to be logged try { this.banditLogger.logBanditAction(banditEvent); From 78b29329a16d46096f774ec532593b502b3d2933 Mon Sep 17 00:00:00 2001 From: Aaron Silverman Date: Tue, 27 Aug 2024 15:14:42 -0600 Subject: [PATCH 08/10] dedupe when queuing assignments as well --- src/client/eppo-client-with-bandits.spec.ts | 16 +++++++++++ src/client/eppo-client.ts | 31 ++++++++++----------- 2 files changed, 30 insertions(+), 17 deletions(-) diff --git a/src/client/eppo-client-with-bandits.spec.ts b/src/client/eppo-client-with-bandits.spec.ts index af6bc6a..0610a90 100644 --- a/src/client/eppo-client-with-bandits.spec.ts +++ b/src/client/eppo-client-with-bandits.spec.ts @@ -205,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( @@ -221,6 +223,20 @@ describe('EppoClient Bandits E2E test', () => { expect(mockLogAssignment).not.toHaveBeenCalled(); expect(mockLogBanditAction).not.toHaveBeenCalled(); + const repeatAssignment = client.getBanditAction( + 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 }); diff --git a/src/client/eppo-client.ts b/src/client/eppo-client.ts index fb46a99..d791b20 100644 --- a/src/client/eppo-client.ts +++ b/src/client/eppo-client.ts @@ -670,18 +670,15 @@ export default class EppoClient { return; } - // If no logger defined, queue up the events (up to a max) to flush if a logger is later defined - 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); - } - return; - } - // 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); } catch (err) { logger.warn('Error encountered logging bandit action', err); @@ -994,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, From b0c347d96444dc8dec8dc8c83baceaa997fb1079 Mon Sep 17 00:00:00 2001 From: Aaron Silverman Date: Thu, 29 Aug 2024 07:47:35 -0600 Subject: [PATCH 09/10] Bump package.json version correctly Co-authored-by: Leo Romanovsky --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 8e58079..fb7305d 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@eppo/js-client-sdk-common", - "version": "4.1.2", + "version": "4.1.0", "description": "Eppo SDK for client-side JavaScript applications (base for both web and react native)", "main": "dist/index.js", "files": [ From 7703143c736ef3882f26d7c80a730065614bc56e Mon Sep 17 00:00:00 2001 From: Aaron Silverman Date: Thu, 29 Aug 2024 12:47:47 -0600 Subject: [PATCH 10/10] feedback from PR --- src/cache/abstract-assignment-cache.spec.ts | 24 +++++++++++++++++++-- src/cache/abstract-assignment-cache.ts | 10 ++++++++- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/src/cache/abstract-assignment-cache.spec.ts b/src/cache/abstract-assignment-cache.spec.ts index e89a7ae..6482dd7 100644 --- a/src/cache/abstract-assignment-cache.spec.ts +++ b/src/cache/abstract-assignment-cache.spec.ts @@ -5,7 +5,7 @@ import { } from './abstract-assignment-cache'; describe('NonExpiringInMemoryAssignmentCache', () => { - it('read and write entries', () => { + it('read and write variation entries', () => { const cache = new NonExpiringInMemoryAssignmentCache(); const key1 = { subjectKey: 'a', flagKey: 'b', allocationKey: 'c', variationKey: 'd' }; const key2 = { subjectKey: '1', flagKey: '2', allocationKey: '3', variationKey: '4' }; @@ -14,7 +14,7 @@ describe('NonExpiringInMemoryAssignmentCache', () => { expect(cache.has(key2)).toBeFalsy(); cache.set(key2); expect(cache.has(key2)).toBeTruthy(); - // this makes an assumption about the internal implementation of the cache, which is not ideal + // this makes an assumption about the internal implementation of the cache, which is not ideal, // but it's the only way to test the cache without exposing the internal state expect(Array.from(cache.entries())).toEqual([ [assignmentCacheKeyToString(key1), assignmentCacheValueToString(key1)], @@ -24,4 +24,24 @@ describe('NonExpiringInMemoryAssignmentCache', () => { expect(cache.has({ ...key1, allocationKey: 'c1' })).toBeFalsy(); expect(cache.has({ ...key2, variationKey: 'd1' })).toBeFalsy(); }); + + it('read and write bandit entries', () => { + const cache = new NonExpiringInMemoryAssignmentCache(); + const key1 = { subjectKey: 'a', flagKey: 'b', banditKey: 'c', actionKey: 'd' }; + const key2 = { subjectKey: '1', flagKey: '2', banditKey: '3', actionKey: '4' }; + cache.set(key1); + expect(cache.has(key1)).toBeTruthy(); + expect(cache.has(key2)).toBeFalsy(); + cache.set(key2); + expect(cache.has(key2)).toBeTruthy(); + // this makes an assumption about the internal implementation of the cache, which is not ideal, + // but it's the only way to test the cache without exposing the internal state + expect(Array.from(cache.entries())).toEqual([ + [assignmentCacheKeyToString(key1), assignmentCacheValueToString(key1)], + [assignmentCacheKeyToString(key2), assignmentCacheValueToString(key2)], + ]); + + expect(cache.has({ ...key1, banditKey: 'c1' })).toBeFalsy(); + expect(cache.has({ ...key2, actionKey: 'd1' })).toBeFalsy(); + }); }); diff --git a/src/cache/abstract-assignment-cache.ts b/src/cache/abstract-assignment-cache.ts index 125b6cc..d5bb9d7 100644 --- a/src/cache/abstract-assignment-cache.ts +++ b/src/cache/abstract-assignment-cache.ts @@ -13,7 +13,15 @@ export type AssignmentCacheKey = { flagKey: string; }; -export type AssignmentCacheValue = Record; +export type CacheKeyPair = { + [K in T]: string; +} & { + [K in U]: string; +}; + +type VariationCacheValue = CacheKeyPair<'allocationKey', 'variationKey'>; +type BanditCacheValue = CacheKeyPair<'banditKey', 'actionKey'>; +export type AssignmentCacheValue = VariationCacheValue | BanditCacheValue; export type AssignmentCacheEntry = AssignmentCacheKey & AssignmentCacheValue;