-
Notifications
You must be signed in to change notification settings - Fork 1
Add bandit assignment cache #120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
6b8e782
f263b50
d4371b9
651cb66
899967f
74bcc0f
8bc8abc
78b2932
b0c347d
7703143
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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<Flag>(); | ||
|
@@ -429,5 +430,129 @@ 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<IAssignmentDetails<string>, 'evaluationDetails'> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find |
||
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 }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where |
||
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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where |
||
actionAttributes: { numericAttributes: {}, categoricalAttributes: {} }, | ||
actionScore: 10, | ||
actionWeight: 0.5, | ||
gamma: 1.0, | ||
optimalityGap: 5, | ||
} as BanditEvaluation; | ||
}); | ||
}); | ||
|
||
beforeEach(() => { | ||
client.useNonExpiringInMemoryAssignmentCache(); | ||
client.useNonExpiringInMemoryBanditAssignmentCache(); | ||
}); | ||
|
||
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(fourthBanditAssignment.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); | ||
}); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rasendubi no early return before setting the cache entry--instead |
||
} 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; | ||
} | ||
|
There was a problem hiding this comment.
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
if
s 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to this comment
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 💪