Skip to content

fix: non-bandit flags return string assignment #238

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 11, 2025
Merged

fix: non-bandit flags return string assignment #238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
264202c
fix: non-bandit flags return string assignment
greghuels Mar 11, 2025
7dbd361
CR changes
greghuels Mar 11, 2025
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
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@eppo/js-client-sdk-common",
"version": "4.13.1",
"version": "4.13.2",
"description": "Common library for Eppo JavaScript SDKs (web, react native, and node)",
"main": "dist/index.js",
"files": [
Expand Down
37 changes: 27 additions & 10 deletions src/client/eppo-precomputed-client-with-bandits.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import {
MOCK_PRECOMPUTED_WIRE_FILE,
readMockConfigurationWireResponse,
MOCK_DEOBFUSCATED_PRECOMPUTED_RESPONSE_FILE,
} from '../../test/testHelpers';
import ApiEndpoints from '../api-endpoints';
import { MemoryOnlyConfigurationStore } from '../configuration-store/memory.store';
import FetchHttpClient from '../http-client';
import { PrecomputedFlag, IObfuscatedPrecomputedBandit } from '../interfaces';
import { IObfuscatedPrecomputedBandit, PrecomputedFlag } from '../interfaces';
import PrecomputedFlagRequestor from '../precomputed-requestor';

import EppoPrecomputedClient from './eppo-precomputed-client';
Expand All @@ -17,12 +17,10 @@ describe('EppoPrecomputedClient Bandits E2E test', () => {
const mockLogAssignment = jest.fn();
const mockLogBanditAction = jest.fn();

const precomputedConfigurationWire = readMockConfigurationWireResponse(
MOCK_DEOBFUSCATED_PRECOMPUTED_RESPONSE_FILE,
);
const parsedPrecomputedResponse = JSON.parse(precomputedConfigurationWire).precomputed.response;
const obfuscatedConfigurationWire = readMockConfigurationWireResponse(MOCK_PRECOMPUTED_WIRE_FILE);
const obfuscatedResponse = JSON.parse(obfuscatedConfigurationWire).precomputed.response;

const testModes = ['offline', 'online'] as const;
const testModes = ['offline'];

testModes.forEach((mode) => {
describe(`${mode} mode`, () => {
Expand All @@ -33,7 +31,7 @@ describe('EppoPrecomputedClient Bandits E2E test', () => {
return Promise.resolve({
ok: true,
status: 200,
json: () => Promise.resolve(parsedPrecomputedResponse),
json: () => Promise.resolve(JSON.parse(obfuscatedResponse)),
});
}) as jest.Mock;

Expand Down Expand Up @@ -62,13 +60,17 @@ describe('EppoPrecomputedClient Bandits E2E test', () => {
categoricalAttributes: { loyalty_tier: 'bronze' },
},
},
'not-a-bandit-flag': {},
},
);
await configurationRequestor.fetchAndStorePrecomputedFlags();
} else if (mode === 'offline') {
const parsed = JSON.parse(obfuscatedResponse);
// Offline mode: directly populate stores with precomputed response
await precomputedFlagStore.setEntries(parsedPrecomputedResponse.flags);
await precomputedBanditStore.setEntries(parsedPrecomputedResponse.bandits);
precomputedFlagStore.salt = parsed.salt;
precomputedBanditStore.salt = parsed.salt;
await precomputedFlagStore.setEntries(parsed.flags);
await precomputedBanditStore.setEntries(parsed.bandits);
}
});

Expand Down Expand Up @@ -102,6 +104,21 @@ describe('EppoPrecomputedClient Bandits E2E test', () => {
const precomputedConfiguration = client.getBanditAction('banner_bandit_flag', 'nike');
expect(precomputedConfiguration).toEqual({ action: null, variation: 'nike' });
});

it('should return the assigned variation if a flag is not a bandit', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

🙌

const precomputedConfiguration = client.getBanditAction('not-a-bandit-flag', 'default');
expect(precomputedConfiguration).toEqual({ action: null, variation: 'control' });
Copy link
Contributor

Choose a reason for hiding this comment

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

also consider a situation where a flag IS a bandit flag, but they got assigned a non-bandit variation.

Also we should be using mocks to make sure the proper event loggers are called or not

Copy link
Contributor Author

@greghuels greghuels Mar 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

also consider a situation where a flag IS a bandit flag, but they got assigned a non-bandit variation.

For the precomputed response, wouldn't that be the same situation? The flag would be in flags but not in bandits

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also we should be using mocks to make sure the proper event loggers are called or not

Updated! And I also added the happy path case of a bandit flag with a bandit variation

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

it('should return the bandit variation and action if a flag is a bandit', () => {
const precomputedConfiguration = client.getBanditAction('string-flag', 'default');
expect(precomputedConfiguration).toEqual({
action: 'show_red_button',
variation: 'red',
});
expect(mockLogBanditAction).toHaveBeenCalled();
});
});
});
});
57 changes: 28 additions & 29 deletions src/client/eppo-precomputed-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,39 +324,38 @@ export default class EppoPrecomputedClient {
flagKey: string,
defaultValue: string,
): Omit<IAssignmentDetails<string>, 'evaluationDetails'> {
const banditEvaluation = this.getPrecomputedBandit(flagKey);

if (banditEvaluation == null) {
logger.warn(`${loggerPrefix} No assigned variation. Bandit not found: ${flagKey}`);
const precomputedFlag = this.getPrecomputedFlag(flagKey);
if (!precomputedFlag) {
logger.warn(`${loggerPrefix} No assigned variation. Flag not found: ${flagKey}`);
return { variation: defaultValue, action: null };
Copy link
Contributor

Choose a reason for hiding this comment

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

Returning this if the precomputed flag is not found, otherwise returning the precomputed flag assignment if the bandit could not be evaluated. Makes sense

}

const banditEvaluation = this.getPrecomputedBandit(flagKey);
const assignedVariation = this.getStringAssignment(flagKey, defaultValue);

const banditEvent: IBanditEvent = {
timestamp: new Date().toISOString(),
featureFlag: flagKey,
bandit: banditEvaluation.banditKey,
subject: this.subject.subjectKey ?? '',
action: banditEvaluation.action,
actionProbability: banditEvaluation.actionProbability,
optimalityGap: banditEvaluation.optimalityGap,
modelVersion: banditEvaluation.modelVersion,
subjectNumericAttributes: banditEvaluation.actionNumericAttributes,
subjectCategoricalAttributes: banditEvaluation.actionCategoricalAttributes,
actionNumericAttributes: banditEvaluation.actionNumericAttributes,
actionCategoricalAttributes: banditEvaluation.actionCategoricalAttributes,
metaData: this.buildLoggerMetadata(),
evaluationDetails: null,
};

try {
this.logBanditAction(banditEvent);
} catch (error) {
logger.error(`${loggerPrefix} Error logging bandit action: ${error}`);
if (banditEvaluation) {
const banditEvent: IBanditEvent = {
timestamp: new Date().toISOString(),
featureFlag: flagKey,
bandit: banditEvaluation.banditKey,
subject: this.subject.subjectKey ?? '',
action: banditEvaluation.action,
actionProbability: banditEvaluation.actionProbability,
optimalityGap: banditEvaluation.optimalityGap,
modelVersion: banditEvaluation.modelVersion,
subjectNumericAttributes: banditEvaluation.actionNumericAttributes,
subjectCategoricalAttributes: banditEvaluation.actionCategoricalAttributes,
actionNumericAttributes: banditEvaluation.actionNumericAttributes,
actionCategoricalAttributes: banditEvaluation.actionCategoricalAttributes,
metaData: this.buildLoggerMetadata(),
evaluationDetails: null,
};
try {
this.logBanditAction(banditEvent);
} catch (error) {
logger.error(`${loggerPrefix} Error logging bandit action: ${error}`);
}
return { variation: assignedVariation, action: banditEvent.action };
Copy link
Member

Choose a reason for hiding this comment

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

Returning the banditEvent.action is interesting here. Are you counting on it being undefined | null? If so please return that directly - or is it preforming randomization in this case?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we ax this line all together, just keeping that final return with optional chaining and null coalesce:

return { variation: assignedVariation, action: banditEvent?.action ?? null }

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry banditEvaluation.action

Copy link
Member

Choose a reason for hiding this comment

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

All good I misread it ✅

}

return { variation: assignedVariation, action: banditEvent.action };
return { variation: assignedVariation, action: null };
}

private getPrecomputedFlag(flagKey: string): DecodedPrecomputedFlag | null {
Expand Down
Loading