Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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
26 changes: 20 additions & 6 deletions src/cache/abstract-assignment-cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,18 @@ import { getMD5Hash } from '../obfuscation';

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

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

type BanditAssignmentCacheValue = {
banditKey: string;
actionKey: string;
};

export type AssignmentCacheValue = FlagAssignmentCacheValue | BanditAssignmentCacheValue;

export type AssignmentCacheKey = {
subjectKey: string;
flagKey: string;
Expand All @@ -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(';'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: this code looks like it's trying to cram two functions into one which makes it convoluted to follow (e.g., is it possible that both ifs match? none?)

If we add new event types (or decide to add allocation/variation key to bandit action), this is going to get messy.

A cleaner solution would be to make cache type generic over what it caches and/or accept key/value mapping functions as parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to this comment

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 don't think we could do generic because we only want to hash a specific subset of the provided properties. The code is indeed just handling two specific possible code paths

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: since the other fields are part of the cache key and we know they will be unique, we can just simply hash the entire value, making this generic 💪

}

export interface AsyncMap<K, V> {
Expand Down
137 changes: 135 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 @@ -429,5 +430,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
});
});
});
});
42 changes: 41 additions & 1 deletion 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,39 @@ 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. 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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: this comment seems misplaced. The best place for it is calculation of cache key and value (as these are controlling this behavior). If cache key or value change, this comment could easily become outdated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was just trying to explain what banditAssignmentCacheProperties is and how it will be used, but agree the comment could get stale. I'll experiment with better spatially colocating it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would move cache entry construction inside the caching module. Otherwise, it looks like a leaking implementation detail — the caller needs to know what keys are used in caching and copy them into banditAssignmentCacheProperties. It would be simpler if the cache was accepting the event

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 found a good home for this in abstract-assignment-cache.ts after all! 🎉

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

if (this.banditAssignmentCache?.has(banditAssignmentCacheProperties)) {
// Ignore repeat assignment
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) {
this.queuedBanditEvents.push(banditEvent);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: shall we deduplicate events in the queuedBanditEvents?

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 don't think so as these are basically all sent to a logger if one is added, so we want deduping to happen beforehand like we're doing

Copy link
Collaborator

Choose a reason for hiding this comment

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

We're not deduplicating them beforehand here because return on the next line prevents us from setting the entry in the cache.

If we log the same event twice before the user sets the logger, the event will be sent to user's logger twice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see what you are saying now, good catch! Will address

}
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);
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 +928,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
Loading