From c120948d219ad4cc35b5ea6b58bf65d7ad58ee77 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Fri, 22 Sep 2023 09:25:33 -0700 Subject: [PATCH 1/2] feat: Do not generate an event if the measurements are inconsistent. (#282) --- .../__tests__/MigrationOpTracker.test.ts | 105 ++++++++++++++++++ .../sdk-server/src/MigrationOpTracker.ts | 20 ++-- 2 files changed, 118 insertions(+), 7 deletions(-) diff --git a/packages/shared/sdk-server/__tests__/MigrationOpTracker.test.ts b/packages/shared/sdk-server/__tests__/MigrationOpTracker.test.ts index 1cb0dd280..4c045717e 100644 --- a/packages/shared/sdk-server/__tests__/MigrationOpTracker.test.ts +++ b/packages/shared/sdk-server/__tests__/MigrationOpTracker.test.ts @@ -265,3 +265,108 @@ it('can handle exceptions thrown in the consistency check method', () => { }, ]); }); + +it.each([ + [false, true, true, false], + [true, false, false, true], + [false, true, true, true], + [true, false, true, true], +])( + 'does not generate an event if latency measurement without correct invoked measurement' + + ' invoke old: %p invoke new: %p measure old: %p measure new: %p', + (invoke_old, invoke_new, measure_old, measure_new) => { + const tracker = new MigrationOpTracker( + 'flag', + { user: 'bob' }, + LDMigrationStage.Off, + LDMigrationStage.Off, + { + kind: 'FALLTHROUGH', + }, + ); + + tracker.op('write'); + if (invoke_old) { + tracker.invoked('old'); + } + if (invoke_new) { + tracker.invoked('new'); + } + if (measure_old) { + tracker.latency('old', 100); + } + if (measure_new) { + tracker.latency('new', 100); + } + + expect(tracker.createEvent()).toBeUndefined(); + }, +); + +it.each([ + [false, true, true, false], + [true, false, false, true], + [false, true, true, true], + [true, false, true, true], +])( + 'does not generate an event error measurement without correct invoked measurement' + + ' invoke old: %p invoke new: %p measure old: %p measure new: %p', + (invoke_old, invoke_new, measure_old, measure_new) => { + const tracker = new MigrationOpTracker( + 'flag', + { user: 'bob' }, + LDMigrationStage.Off, + LDMigrationStage.Off, + { + kind: 'FALLTHROUGH', + }, + ); + + tracker.op('write'); + if (invoke_old) { + tracker.invoked('old'); + } + if (invoke_new) { + tracker.invoked('new'); + } + if (measure_old) { + tracker.error('old'); + } + if (measure_new) { + tracker.error('new'); + } + + expect(tracker.createEvent()).toBeUndefined(); + }, +); + +it.each([ + [true, false, true], + [false, true, true], + [true, false, false], + [false, true, false], +])( + 'does not generate an event if there is a consistency measurement but both origins were not invoked' + + ' invoke old: %p invoke new: %p consistent: %p', + (invoke_old, invoke_new, consistent) => { + const tracker = new MigrationOpTracker( + 'flag', + { user: 'bob' }, + LDMigrationStage.Off, + LDMigrationStage.Off, + { + kind: 'FALLTHROUGH', + }, + ); + + tracker.op('write'); + if (invoke_old) { + tracker.invoked('old'); + } + if (invoke_new) { + tracker.invoked('new'); + } + tracker.consistency(() => consistent); + expect(tracker.createEvent()).toBeUndefined(); + }, +); diff --git a/packages/shared/sdk-server/src/MigrationOpTracker.ts b/packages/shared/sdk-server/src/MigrationOpTracker.ts index 795572c00..eb30be50b 100644 --- a/packages/shared/sdk-server/src/MigrationOpTracker.ts +++ b/packages/shared/sdk-server/src/MigrationOpTracker.ts @@ -26,7 +26,7 @@ export default class MigrationOpTracker implements LDMigrationTracker { new: false, }; - private consistencyCheck?: LDConsistencyCheck; + private consistencyCheck: LDConsistencyCheck = LDConsistencyCheck.NotChecked; private latencyMeasurement = { old: NaN, @@ -101,13 +101,16 @@ export default class MigrationOpTracker implements LDMigrationTracker { return undefined; } + if (!this.measurementConsistencyCheck()) { + return undefined; + } + const measurements: LDMigrationMeasurement[] = []; this.populateInvoked(measurements); this.populateConsistency(measurements); this.populateLatency(measurements); this.populateErrors(measurements); - this.measurementConsistencyCheck(); return { kind: 'migration_op', @@ -145,32 +148,35 @@ export default class MigrationOpTracker implements LDMigrationTracker { ); } - private checkOriginEventConsistency(origin: LDMigrationOrigin) { + private checkOriginEventConsistency(origin: LDMigrationOrigin): boolean { if (this.wasInvoked[origin]) { - return; + return true; } // If the specific origin was not invoked, but it contains measurements, then // that is a problem. Check each measurement and log a message if it is present. if (!Number.isNaN(this.latencyMeasurement[origin])) { this.logger?.error(`${this.logTag()} ${this.latencyConsistencyMessage(origin)}`); + return false; } if (this.errors[origin]) { this.logger?.error(`${this.logTag()} ${this.errorConsistencyMessage(origin)}`); + return false; } if (this.consistencyCheck !== LDConsistencyCheck.NotChecked) { this.logger?.error(`${this.logTag()} ${this.consistencyCheckConsistencyMessage(origin)}`); + return false; } + return true; } /** * Check that the latency, error, consistency and invoked measurements are self-consistent. */ - private measurementConsistencyCheck() { - this.checkOriginEventConsistency('old'); - this.checkOriginEventConsistency('new'); + private measurementConsistencyCheck(): boolean { + return this.checkOriginEventConsistency('old') && this.checkOriginEventConsistency('new'); } private populateInvoked(measurements: LDMigrationMeasurement[]) { From 9c5f402a677e3a37c366ac94ef676a5805d852b9 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Fri, 22 Sep 2023 09:25:44 -0700 Subject: [PATCH 2/2] feat: Add flag version to migration op event. (#281) --- .../__tests__/MigrationOpTracker.test.ts | 78 +++++++++++++++++++ .../shared/sdk-server/src/LDClientImpl.ts | 2 + .../src/MigrationOpEventConversion.ts | 4 + .../sdk-server/src/MigrationOpTracker.ts | 2 + .../src/api/data/LDMigrationOpEvent.ts | 1 + 5 files changed, 87 insertions(+) diff --git a/packages/shared/sdk-server/__tests__/MigrationOpTracker.test.ts b/packages/shared/sdk-server/__tests__/MigrationOpTracker.test.ts index 4c045717e..8346aa2b5 100644 --- a/packages/shared/sdk-server/__tests__/MigrationOpTracker.test.ts +++ b/packages/shared/sdk-server/__tests__/MigrationOpTracker.test.ts @@ -58,6 +58,83 @@ it('generates an event if the minimal requirements are met.', () => { }); }); +it('can include the variation in the event', () => { + const tracker = new MigrationOpTracker( + 'flag', + { user: 'bob' }, + LDMigrationStage.Off, + LDMigrationStage.Off, + { + kind: 'FALLTHROUGH', + }, + undefined, + 1, + ); + + tracker.op('write'); + tracker.invoked('old'); + + expect(tracker.createEvent()).toMatchObject({ + contextKeys: { user: 'bob' }, + evaluation: { + default: 'off', + key: 'flag', + reason: { kind: 'FALLTHROUGH' }, + value: 'off', + variation: 1, + }, + kind: 'migration_op', + measurements: [ + { + key: 'invoked', + values: { + old: true, + }, + }, + ], + operation: 'write', + }); +}); + +it('can include the version in the event', () => { + const tracker = new MigrationOpTracker( + 'flag', + { user: 'bob' }, + LDMigrationStage.Off, + LDMigrationStage.Off, + { + kind: 'FALLTHROUGH', + }, + undefined, + undefined, + 2, + ); + + tracker.op('write'); + tracker.invoked('old'); + + expect(tracker.createEvent()).toMatchObject({ + contextKeys: { user: 'bob' }, + evaluation: { + default: 'off', + key: 'flag', + reason: { kind: 'FALLTHROUGH' }, + value: 'off', + version: 2, + }, + kind: 'migration_op', + measurements: [ + { + key: 'invoked', + values: { + old: true, + }, + }, + ], + operation: 'write', + }); +}); + it('includes errors if at least one is set', () => { const tracker = new MigrationOpTracker( 'flag', @@ -250,6 +327,7 @@ it('can handle exceptions thrown in the consistency check method', () => { undefined, undefined, undefined, + undefined, logger, ); tracker.op('read'); diff --git a/packages/shared/sdk-server/src/LDClientImpl.ts b/packages/shared/sdk-server/src/LDClientImpl.ts index 3b8590ad7..80c94f904 100644 --- a/packages/shared/sdk-server/src/LDClientImpl.ts +++ b/packages/shared/sdk-server/src/LDClientImpl.ts @@ -336,6 +336,7 @@ export default class LDClientImpl implements LDClient { reason, checkRatio, undefined, + undefined, samplingRatio, ), }); @@ -352,6 +353,7 @@ export default class LDClientImpl implements LDClient { checkRatio, // Can be null for compatibility reasons. detail.variationIndex === null ? undefined : detail.variationIndex, + flag?.version, samplingRatio, ), }); diff --git a/packages/shared/sdk-server/src/MigrationOpEventConversion.ts b/packages/shared/sdk-server/src/MigrationOpEventConversion.ts index f18348c30..75b6a1530 100644 --- a/packages/shared/sdk-server/src/MigrationOpEventConversion.ts +++ b/packages/shared/sdk-server/src/MigrationOpEventConversion.ts @@ -196,6 +196,10 @@ function validateEvaluation(evaluation: LDMigrationEvaluation): LDMigrationEvalu validated.variation = evaluation.variation; } + if (evaluation.version !== undefined && TypeValidators.Number.is(evaluation.version)) { + validated.version = evaluation.version; + } + return validated; } diff --git a/packages/shared/sdk-server/src/MigrationOpTracker.ts b/packages/shared/sdk-server/src/MigrationOpTracker.ts index eb30be50b..9001e42fd 100644 --- a/packages/shared/sdk-server/src/MigrationOpTracker.ts +++ b/packages/shared/sdk-server/src/MigrationOpTracker.ts @@ -43,6 +43,7 @@ export default class MigrationOpTracker implements LDMigrationTracker { private readonly reason: LDEvaluationReason, private readonly checkRatio?: number, private readonly variation?: number, + private readonly version?: number, private readonly samplingRatio?: number, private readonly logger?: LDLogger, ) {} @@ -123,6 +124,7 @@ export default class MigrationOpTracker implements LDMigrationTracker { default: this.defaultStage, reason: this.reason, variation: this.variation, + version: this.version, }, measurements, samplingRatio: this.samplingRatio ?? 1, diff --git a/packages/shared/sdk-server/src/api/data/LDMigrationOpEvent.ts b/packages/shared/sdk-server/src/api/data/LDMigrationOpEvent.ts index c3e69d702..544a7b34c 100644 --- a/packages/shared/sdk-server/src/api/data/LDMigrationOpEvent.ts +++ b/packages/shared/sdk-server/src/api/data/LDMigrationOpEvent.ts @@ -13,6 +13,7 @@ export interface LDMigrationEvaluation { value: LDMigrationStage; default: LDMigrationStage; variation?: number; + version?: number; reason: LDEvaluationReason; }