Skip to content
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

Jump to bottom

FF-2525 Add Assignment Details to logger #106

Merged
merged 1 commit into from
Jun 28, 2024
+53 −36
Merged

FF-2525 Add Assignment Details to logger #106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 17 additions & 0 deletions src/assignment-logger.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { IAssignmentEvent } from './assignment-logger';
import { AllocationEvaluationCode } from './flag-evaluation-details-builder';

describe('IAssignmentEvent', () => {
it('should allow adding arbitrary fields', () => {
Expand All @@ -11,6 +12,22 @@ describe('IAssignmentEvent', () => {
timestamp: new Date().toISOString(),
subjectAttributes: { age: 25, country: 'USA' },
holdoutKey: 'holdout_key_123',
details: {
Copy link
Contributor

Choose a reason for hiding this comment

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

The purpose of this test was not obvious to me until I ran it myself. It will fail if any required attribute in IAssignmentEvent is missing but will pass if attributes not specified in IAssignmentEvent are included in the event object (the latter behavior is described by the test "it" statement)

Just a suggestion (feel free to take it or leave it): if it's not too much trouble, you could add a test that checks for a thrown exception if details (a required attribute in the event object) is missing

variationKey: 'variationKey',
variationValue: 'variation_123',
flagEvaluationCode: 'MATCH',
flagEvaluationDescription: '',
configFetchedAt: new Date().toISOString(),
configPublishedAt: new Date().toISOString(),
matchedRule: null,
matchedAllocation: {
key: 'allocation_123',
allocationEvaluationCode: AllocationEvaluationCode.MATCH,
orderPosition: 1,
},
unmatchedAllocations: [],
unevaluatedAllocations: [],
},
};

expect(event.holdoutKey).toBe('holdout_key_123');
Expand Down
7 changes: 7 additions & 0 deletions src/assignment-logger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { IFlagEvaluationDetails } from './flag-evaluation-details-builder';

export enum HoldoutVariationEnum {
STATUS_QUO = 'status_quo',
ALL_SHIPPED = 'all_shipped_variants',
Expand Down Expand Up @@ -46,6 +48,11 @@ export interface IAssignmentEvent {
[propName: string]: unknown;

metaData?: Record<string, unknown>;

/**
* The flag evaluation details
*/
details: IFlagEvaluationDetails;
}

/**
Expand Down
16 changes: 8 additions & 8 deletions src/client/eppo-client-assignment-details.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import FetchHttpClient from '../http-client';
import { Flag, ObfuscatedFlag, VariationType } from '../interfaces';
import { OperatorType } from '../rules';

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

async function init(configurationStore: IConfigurationStore<Flag | ObfuscatedFlag>) {
const apiEndpoints = new ApiEndpoints({
Expand Down Expand Up @@ -57,7 +57,7 @@ describe('EppoClient get*AssignmentDetails', () => {
subjectAttributes,
0,
);
const expected: AssignmentDetails<number> = {
const expected: IAssignmentDetails<number> = {
value: 3,
variationKey: 'three',
variationValue: 3,
Expand Down Expand Up @@ -92,7 +92,7 @@ describe('EppoClient get*AssignmentDetails', () => {
expect(result).toMatchObject(expected);
});

it('should set the details for a matched split', () => {
it.only('should set the details for a matched split', () => {
const client = new EppoClient(storage);
client.setIsGracefulFailureMode(false);
const subjectAttributes = { email: '[email protected]', country: 'Brazil' };
Expand All @@ -102,7 +102,7 @@ describe('EppoClient get*AssignmentDetails', () => {
subjectAttributes,
0,
);
const expected: AssignmentDetails<number> = {
const expected: IAssignmentDetails<number> = {
value: 2,
variationKey: 'two',
variationValue: 2,
Expand All @@ -115,13 +115,13 @@ describe('EppoClient get*AssignmentDetails', () => {
matchedAllocation: {
key: '50/50 split',
allocationEvaluationCode: AllocationEvaluationCode.MATCH,
orderPosition: 1,
orderPosition: 2,
},
unmatchedAllocations: [
{
key: 'targeted allocation',
allocationEvaluationCode: AllocationEvaluationCode.FAILING_RULE,
orderPosition: 0,
orderPosition: 1,
},
],
unevaluatedAllocations: [],
Expand All @@ -139,7 +139,7 @@ describe('EppoClient get*AssignmentDetails', () => {
subjectAttributes,
'',
);
const expected: AssignmentDetails<string> = {
const expected: IAssignmentDetails<string> = {
value: 'control',
flagEvaluationCode: 'MATCH',
flagEvaluationDescription:
Expand Down Expand Up @@ -201,7 +201,7 @@ describe('EppoClient get*AssignmentDetails', () => {
matchedAllocation: null,
unmatchedAllocations: [],
unevaluatedAllocations: [],
} as AssignmentDetails<number>);
} as IAssignmentDetails<number>);
});

describe('UFC General Test Cases', () => {
Expand Down
10 changes: 1 addition & 9 deletions src/client/eppo-client.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,9 @@ import {
MOCK_UFC_RESPONSE_FILE,
OBFUSCATED_MOCK_UFC_RESPONSE_FILE,
SubjectTestCase,
getTestAssignmentDetails,
getTestAssignments,
readAssignmentTestData,
readMockUFCResponse,
validateTestAssignmentDetails,
validateTestAssignments,
} from '../../test/testHelpers';
import ApiEndpoints from '../api-endpoints';
Expand All @@ -19,16 +17,10 @@ import { IConfigurationStore } from '../configuration-store/configuration-store'
import { MemoryOnlyConfigurationStore } from '../configuration-store/memory.store';
import { MAX_EVENT_QUEUE_SIZE, POLL_INTERVAL_MS, POLL_JITTER_PCT } from '../constants';
import FlagConfigurationRequestor from '../flag-configuration-requestor';
import { AllocationEvaluationCode } from '../flag-evaluation-details-builder';
import FetchHttpClient from '../http-client';
import { Flag, ObfuscatedFlag, VariationType } from '../interfaces';
import { OperatorType } from '../rules';

import EppoClient, {
AssignmentDetails,
Copy link
Contributor

Choose a reason for hiding this comment

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

Guessing these were unused imports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup!

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

export async function init(configurationStore: IConfigurationStore<Flag | ObfuscatedFlag>) {
const apiEndpoints = new ApiEndpoints({
Expand Down
7 changes: 4 additions & 3 deletions src/client/eppo-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { EppoValue } from '../eppo_value';
import { Evaluator, FlagEvaluation, noneResult } from '../evaluator';
import FlagConfigurationRequestor from '../flag-configuration-requestor';
import {
FlagEvaluationDetails,
IFlagEvaluationDetails,
FlagEvaluationDetailsBuilder,
} from '../flag-evaluation-details-builder';
import FetchHttpClient from '../http-client';
Expand All @@ -31,7 +31,7 @@ import { validateNotBlank } from '../validation';
import { LIB_VERSION } from '../version';

export interface IAssignmentDetails<T extends Variation['value'] | object>
extends FlagEvaluationDetails {
extends IFlagEvaluationDetails {
value: T;
}

Expand Down Expand Up @@ -400,7 +400,7 @@ export default class EppoClient {
subjectAttributes: Record<string, AttributeType>,
defaultValue: EppoValue,
expectedVariationType: VariationType,
): { eppoValue: EppoValue; flagEvaluationDetails: FlagEvaluationDetails } {
): { eppoValue: EppoValue; flagEvaluationDetails: IFlagEvaluationDetails } {
try {
const result = this.getAssignmentDetail(
flagKey,
Expand Down Expand Up @@ -622,6 +622,7 @@ export default class EppoClient {
sdkLanguage: 'javascript',
sdkLibVersion: LIB_VERSION,
},
details: result.flagEvaluationDetails,
Copy link
Contributor

Choose a reason for hiding this comment

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

🕵️‍♂️ Spotted the actual change! 😛

};

if (variation && allocationKey) {
Expand Down
6 changes: 3 additions & 3 deletions src/evaluator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {
AllocationEvaluation,
AllocationEvaluationCode,
FlagEvaluationDetails,
IFlagEvaluationDetails,
FlagEvaluationDetailsBuilder,
} from './flag-evaluation-details-builder';
import { Flag, Shard, Range, Variation, Allocation, Split, VariationType } from './interfaces';
Expand All @@ -17,7 +17,7 @@ export interface FlagEvaluation {
variation: Variation | null;
extraLogging: Record<string, string>;
doLog: boolean;
flagEvaluationDetails: FlagEvaluationDetails;
flagEvaluationDetails: IFlagEvaluationDetails;
}

export class Evaluator {
Expand Down Expand Up @@ -166,7 +166,7 @@ export function noneResult(
flagKey: string,
subjectKey: string,
subjectAttributes: SubjectAttributes,
flagEvaluationDetails: FlagEvaluationDetails,
flagEvaluationDetails: IFlagEvaluationDetails,
): FlagEvaluation {
return {
flagKey,
Expand Down
18 changes: 9 additions & 9 deletions src/flag-evaluation-details-builder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export interface AllocationEvaluation {
orderPosition: number;
}

export interface FlagEvaluationDetails {
export interface IFlagEvaluationDetails {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Yes this is our naming convention for interfaces

variationKey: string | null;
variationValue: Variation['value'] | null;
flagEvaluationCode: FlagEvaluationCode;
Expand All @@ -40,12 +40,12 @@ export interface FlagEvaluationDetails {
}

export class FlagEvaluationDetailsBuilder {
private variationKey: FlagEvaluationDetails['variationKey'];
private variationValue: FlagEvaluationDetails['variationValue'];
private matchedRule: FlagEvaluationDetails['matchedRule'];
private matchedAllocation: FlagEvaluationDetails['matchedAllocation'];
private unmatchedAllocations: FlagEvaluationDetails['unmatchedAllocations'];
private unevaluatedAllocations: FlagEvaluationDetails['unevaluatedAllocations'];
private variationKey: IFlagEvaluationDetails['variationKey'];
private variationValue: IFlagEvaluationDetails['variationValue'];
private matchedRule: IFlagEvaluationDetails['matchedRule'];
private matchedAllocation: IFlagEvaluationDetails['matchedAllocation'];
private unmatchedAllocations: IFlagEvaluationDetails['unmatchedAllocations'];
private unevaluatedAllocations: IFlagEvaluationDetails['unevaluatedAllocations'];

constructor(
private readonly allocations: Allocation[],
Expand Down Expand Up @@ -122,12 +122,12 @@ export class FlagEvaluationDetailsBuilder {
buildForNoneResult = (
flagEvaluationCode: FlagEvaluationCode,
flagEvaluationDescription: string,
): FlagEvaluationDetails => this.setNone().build(flagEvaluationCode, flagEvaluationDescription);
): IFlagEvaluationDetails => this.setNone().build(flagEvaluationCode, flagEvaluationDescription);

build = (
flagEvaluationCode: FlagEvaluationCode,
flagEvaluationDescription: string,
): FlagEvaluationDetails => ({
): IFlagEvaluationDetails => ({
flagEvaluationCode,
flagEvaluationDescription,
variationKey: this.variationKey,
Expand Down
8 changes: 4 additions & 4 deletions test/testHelpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as fs from 'fs';

import { AssignmentDetails } from '../src/client/eppo-client';
import { IAssignmentDetails } from '../src/client/eppo-client';
import { Flag, VariationType } from '../src/interfaces';
import { AttributeType } from '../src/types';

Expand All @@ -13,7 +13,7 @@ export interface SubjectTestCase {
subjectKey: string;
subjectAttributes: Record<string, AttributeType>;
assignment: string | number | boolean | object;
assignmentDetails: AssignmentDetails<string | number | boolean | object>;
assignmentDetails: IAssignmentDetails<string | number | boolean | object>;
}

export interface IAssignmentTestCase {
Expand Down Expand Up @@ -70,7 +70,7 @@ export function getTestAssignmentDetails(
) => never,
): {
subject: SubjectTestCase;
assignmentDetails: AssignmentDetails<string | boolean | number | object>;
assignmentDetails: IAssignmentDetails<string | boolean | number | object>;
}[] {
return testCase.subjects.map((subject) => ({
subject,
Expand Down Expand Up @@ -108,7 +108,7 @@ export function validateTestAssignments(
export function validateTestAssignmentDetails(
assignments: {
subject: SubjectTestCase;
assignmentDetails: AssignmentDetails<string | boolean | number | object>;
assignmentDetails: IAssignmentDetails<string | boolean | number | object>;
}[],
flag: string,
) {
Expand Down
Loading