From 7c08e8285fd4a8c354845d20e4fd4b3c0fe95dd3 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Thu, 31 Aug 2023 15:34:36 -0700 Subject: [PATCH] feat: Refactor variation method and consistency tracking. (#264) --- .../__tests__/MigrationOpTracker.test.ts | 6 +-- .../shared/sdk-server/src/LDClientImpl.ts | 10 ++-- packages/shared/sdk-server/src/Migration.ts | 22 ++++----- .../sdk-server/src/MigrationOpTracker.ts | 9 ++-- .../shared/sdk-server/src/api/LDClient.ts | 6 +-- ...ationDetail.ts => LDMigrationVariation.ts} | 49 +++++++------------ .../shared/sdk-server/src/api/data/index.ts | 2 +- 7 files changed, 42 insertions(+), 62 deletions(-) rename packages/shared/sdk-server/src/api/data/{LDMigrationDetail.ts => LDMigrationVariation.ts} (61%) diff --git a/packages/shared/sdk-server/__tests__/MigrationOpTracker.test.ts b/packages/shared/sdk-server/__tests__/MigrationOpTracker.test.ts index 64df78f3d5..57b1536272 100644 --- a/packages/shared/sdk-server/__tests__/MigrationOpTracker.test.ts +++ b/packages/shared/sdk-server/__tests__/MigrationOpTracker.test.ts @@ -1,4 +1,4 @@ -import { LDConsistencyCheck, LDMigrationStage } from '../src'; +import { LDMigrationStage } from '../src'; import { LDMigrationOrigin } from '../src/api/LDMigration'; import MigrationOpTracker from '../src/MigrationOpTracker'; @@ -160,7 +160,7 @@ it('includes if the result was consistent', () => { }, ); tracker.op('read'); - tracker.consistency(LDConsistencyCheck.Consistent); + tracker.consistency(() => true); tracker.invoked('old'); tracker.invoked('new'); @@ -185,7 +185,7 @@ it('includes if the result was inconsistent', () => { tracker.op('read'); tracker.invoked('old'); tracker.invoked('new'); - tracker.consistency(LDConsistencyCheck.Inconsistent); + tracker.consistency(() => false); const event = tracker.createEvent(); expect(event?.measurements).toContainEqual({ diff --git a/packages/shared/sdk-server/src/LDClientImpl.ts b/packages/shared/sdk-server/src/LDClientImpl.ts index 1ccff6083e..3a5913d340 100644 --- a/packages/shared/sdk-server/src/LDClientImpl.ts +++ b/packages/shared/sdk-server/src/LDClientImpl.ts @@ -17,9 +17,9 @@ import { LDClient, LDFlagsState, LDFlagsStateOptions, - LDMigrationDetail, LDMigrationOpEvent, LDMigrationStage, + LDMigrationVariation, LDOptions, LDStreamProcessor, } from './api'; @@ -307,13 +307,13 @@ export default class LDClientImpl implements LDClient { key: string, context: LDContext, defaultValue: LDMigrationStage, - ): Promise { + ): Promise { const convertedContext = Context.fromLDContext(context); const [{ detail }, flag] = await this.evaluateIfPossible( key, context, defaultValue, - this.eventFactoryWithReasons, + this.eventFactoryDefault, ); const contextKeys = convertedContext.valid ? convertedContext.kindsAndKeys : {}; @@ -328,8 +328,6 @@ export default class LDClientImpl implements LDClient { }; return { value: defaultValue, - reason, - checkRatio, tracker: new MigrationOpTracker( key, contextKeys, @@ -343,9 +341,7 @@ export default class LDClientImpl implements LDClient { }; } return { - ...detail, value: detail.value as LDMigrationStage, - checkRatio, tracker: new MigrationOpTracker( key, contextKeys, diff --git a/packages/shared/sdk-server/src/Migration.ts b/packages/shared/sdk-server/src/Migration.ts index 49457d2c76..a645a7e3e5 100644 --- a/packages/shared/sdk-server/src/Migration.ts +++ b/packages/shared/sdk-server/src/Migration.ts @@ -1,6 +1,6 @@ -import { internal, LDContext } from '@launchdarkly/js-sdk-common'; +import { LDContext } from '@launchdarkly/js-sdk-common'; -import { LDClient, LDConsistencyCheck, LDMigrationStage, LDMigrationTracker } from './api'; +import { LDClient, LDMigrationStage, LDMigrationTracker } from './api'; import { LDMigration, LDMigrationOrigin, @@ -17,8 +17,6 @@ import { LDSerialExecution, } from './api/options/LDMigrationOptions'; -const { shouldSample } = internal; - type MultipleReadResult = { fromOld: LDMigrationReadResult; fromNew: LDMigrationReadResult; @@ -88,7 +86,6 @@ export function LDMigrationError(error: Error): { success: false; error: Error } interface MigrationContext { payload?: TPayload; tracker: LDMigrationTracker; - checkRatio?: number; } /** @@ -239,7 +236,6 @@ export default class Migration< const res = await this.readTable[stage.value]({ payload, tracker: stage.tracker, - checkRatio: stage.checkRatio, }); stage.tracker.op('read'); this.sendEvent(stage.tracker); @@ -274,13 +270,13 @@ export default class Migration< oldValue: LDMethodResult, newValue: LDMethodResult, ) { - if (this.config.check && shouldSample(context.checkRatio ?? 1)) { - if (oldValue.success && newValue.success) { - const res = this.config.check(oldValue.result, newValue.result); - context.tracker.consistency( - res ? LDConsistencyCheck.Consistent : LDConsistencyCheck.Inconsistent, - ); - } + if (!this.config.check) { + return; + } + + if (oldValue.success && newValue.success) { + // Check is validated before this point, so it is force unwrapped. + context.tracker.consistency(() => this.config.check!(oldValue.result, newValue.result)); } } diff --git a/packages/shared/sdk-server/src/MigrationOpTracker.ts b/packages/shared/sdk-server/src/MigrationOpTracker.ts index 4de63abd98..f88c28493d 100644 --- a/packages/shared/sdk-server/src/MigrationOpTracker.ts +++ b/packages/shared/sdk-server/src/MigrationOpTracker.ts @@ -1,4 +1,4 @@ -import { LDEvaluationReason, LDLogger } from '@launchdarkly/js-sdk-common'; +import { internal, LDEvaluationReason, LDLogger } from '@launchdarkly/js-sdk-common'; import { LDMigrationStage, LDMigrationTracker } from './api'; import { @@ -55,8 +55,11 @@ export default class MigrationOpTracker implements LDMigrationTracker { this.errors[origin] = true; } - consistency(result: LDConsistencyCheck) { - this.consistencyCheck = result; + consistency(check: () => boolean) { + if (internal.shouldSample(this.checkRatio ?? 1)) { + const res = check(); + this.consistencyCheck = res ? LDConsistencyCheck.Consistent : LDConsistencyCheck.Inconsistent; + } } latency(origin: LDMigrationOrigin, value: number) { diff --git a/packages/shared/sdk-server/src/api/LDClient.ts b/packages/shared/sdk-server/src/api/LDClient.ts index 5b6e95407d..5dfa788734 100644 --- a/packages/shared/sdk-server/src/api/LDClient.ts +++ b/packages/shared/sdk-server/src/api/LDClient.ts @@ -1,6 +1,6 @@ import { LDContext, LDEvaluationDetail, LDFlagValue } from '@launchdarkly/js-sdk-common'; -import { LDMigrationDetail, LDMigrationOpEvent } from './data'; +import { LDMigrationOpEvent, LDMigrationVariation } from './data'; import { LDFlagsState } from './data/LDFlagsState'; import { LDFlagsStateOptions } from './data/LDFlagsStateOptions'; import { LDMigrationStage } from './data/LDMigrationStage'; @@ -135,13 +135,13 @@ export interface LDClient { * @param defaultValue The default value of the flag, to be used if the value is not available * from LaunchDarkly. * @returns - * A Promise which will be resolved with the result (as an{@link LDMigrationDetail}). + * A Promise which will be resolved with the result (as an{@link LDMigrationVariation}). */ variationMigration( key: string, context: LDContext, defaultValue: LDMigrationStage, - ): Promise; + ): Promise; /** * Builds an object that encapsulates the state of all feature flags for a given context. diff --git a/packages/shared/sdk-server/src/api/data/LDMigrationDetail.ts b/packages/shared/sdk-server/src/api/data/LDMigrationVariation.ts similarity index 61% rename from packages/shared/sdk-server/src/api/data/LDMigrationDetail.ts rename to packages/shared/sdk-server/src/api/data/LDMigrationVariation.ts index 9b80c55e5d..5c6c8f85d1 100644 --- a/packages/shared/sdk-server/src/api/data/LDMigrationDetail.ts +++ b/packages/shared/sdk-server/src/api/data/LDMigrationVariation.ts @@ -1,5 +1,3 @@ -import { LDEvaluationReason } from '@launchdarkly/js-sdk-common'; - import { LDMigrationOrigin } from '../LDMigration'; import { LDMigrationOp, LDMigrationOpEvent } from './LDMigrationOpEvent'; import { LDMigrationStage } from './LDMigrationStage'; @@ -32,11 +30,21 @@ export interface LDMigrationTracker { error(origin: LDMigrationOrigin): void; /** - * Report the result of a consistency check. + * Check the consistency of a read result. This method should be invoked if the `check` function + * is defined for the migration and both reads ("new"/"old") were done. + * + * The function will use the checkRatio to determine if the check should be executed, and it + * will record the result. * - * @param result The result of the check. + * Example calling the check function from the migration config. + * ``` + * context.tracker.consistency(() => config.check!(oldValue.result, newValue.result)); + * ``` + * + * @param check The function which executes the check. This is not the `check` function from the + * migration options, but instead should be a parameter-less function that calls that function. */ - consistency(result: LDConsistencyCheck): void; + consistency(check: () => boolean): void; /** * Call this to report that an origin was invoked (executed). There are some situations where the @@ -64,40 +72,17 @@ export interface LDMigrationTracker { } /** - * Detailed information about a migration variation. + * Migration value and tracker. */ -export interface LDMigrationDetail { +export interface LDMigrationVariation { /** * The result of the flag evaluation. This will be either one of the flag's variations or - * the default value that was passed to `LDClient.variationDetail`. + * the default value that was passed to `LDClient.variationMigration`. */ value: LDMigrationStage; /** - * The index of the returned value within the flag's list of variations, e.g. 0 for the - * first variation-- or `null` if the default value was returned. - */ - variationIndex?: number | null; - - /** - * An object describing the main factor that influenced the flag evaluation value. - */ - reason: LDEvaluationReason; - - /** - * A tracker which which can be used to generate analytics for the migration. + * A tracker which can be used to generate analytics for the migration. */ tracker: LDMigrationTracker; - - /** - * When present represents a 1 in X ratio indicating the probability that a given operation - * should have its consistency checked. A 1 indicates that it should always be sampled and - * 0 indicates that it never should be sampled. - */ - checkRatio?: number; - - /** - * Sampling ratio for the migration event. Defaults to 1 if not specified. - */ - samplingRatio?: number; } diff --git a/packages/shared/sdk-server/src/api/data/index.ts b/packages/shared/sdk-server/src/api/data/index.ts index 0aeca0e144..0ce283a473 100644 --- a/packages/shared/sdk-server/src/api/data/index.ts +++ b/packages/shared/sdk-server/src/api/data/index.ts @@ -1,5 +1,5 @@ export * from './LDFlagsStateOptions'; export * from './LDFlagsState'; export * from './LDMigrationStage'; -export * from './LDMigrationDetail'; export * from './LDMigrationOpEvent'; +export * from './LDMigrationVariation';