From 027fd06b75288340da8b2f989b0d99f9bb1e5a76 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Wed, 16 Aug 2023 13:49:22 -0700 Subject: [PATCH 1/2] feat: Remove custom events. --- .../__tests__/MigratioOpEvent.test.ts | 48 ++++++++----------- .../__tests__/MigrationOpTracker.test.ts | 47 ------------------ .../src/MigrationOpEventConversion.ts | 26 ++-------- .../sdk-server/src/MigrationOpTracker.ts | 9 +--- .../src/api/data/LDMigrationDetail.ts | 9 ---- .../src/api/data/LDMigrationOpEvent.ts | 12 +---- 6 files changed, 25 insertions(+), 126 deletions(-) diff --git a/packages/shared/sdk-server/__tests__/MigratioOpEvent.test.ts b/packages/shared/sdk-server/__tests__/MigratioOpEvent.test.ts index 0886b8051..55b5e5375 100644 --- a/packages/shared/sdk-server/__tests__/MigratioOpEvent.test.ts +++ b/packages/shared/sdk-server/__tests__/MigratioOpEvent.test.ts @@ -382,10 +382,7 @@ describe('given an LDClient with test data', () => { }); }); -// Out migrator doesn't create custom measurements. So we need an additional test to ensure -// that custom measurements make it through the conversion process. - -it('can accept custom measurements', () => { +it('ignores invalid measurement keys', () => { const inputEvent: LDMigrationOpEvent = { kind: 'migration_op', operation: 'read', @@ -401,39 +398,20 @@ it('can accept custom measurements', () => { }, measurements: [ { - key: 'custom1', - kind: 'custom', + // @ts-ignore + key: 'bad', values: { old: 1, new: 2, }, }, - { - key: 'custom2', - kind: 'custom', - values: { - new: 2, - }, - }, - { - key: 'custom3', - kind: 'custom', - values: { - old: 2, - }, - }, - { - key: 'custom4', - kind: 'custom', - values: {}, - }, ], }; const validatedEvent = MigrationOpEventConversion(inputEvent); - expect(validatedEvent).toEqual(inputEvent); + expect(validatedEvent).toEqual({...inputEvent, measurements: []}); }); -it('removes bad custom measurements', () => { +it('invalid data types are filtered', () => { const inputEvent: LDMigrationOpEvent = { kind: 'migration_op', operation: 'read', @@ -449,14 +427,26 @@ it('removes bad custom measurements', () => { }, measurements: [ { - key: 'custom1', - kind: 'custom', + key: 'latency_ms', values: { // @ts-ignore old: 'ham', new: 2, }, }, + { + key: 'consistent', + // @ts-ignore + value: undefined + }, + { + key: 'error', + values: { + // @ts-ignore + old: {}, + new: 2, + }, + } ], }; const validatedEvent = MigrationOpEventConversion(inputEvent); diff --git a/packages/shared/sdk-server/__tests__/MigrationOpTracker.test.ts b/packages/shared/sdk-server/__tests__/MigrationOpTracker.test.ts index c96983e77..56369f1fc 100644 --- a/packages/shared/sdk-server/__tests__/MigrationOpTracker.test.ts +++ b/packages/shared/sdk-server/__tests__/MigrationOpTracker.test.ts @@ -175,50 +175,3 @@ it('includes if the result was inconsistent', () => { samplingOdds: 0, }); }); - -it('allows for the addition of custom measurements', () => { - const tracker = new MigrationOpTracker( - 'flag', - { user: 'bob' }, - LDMigrationStage.Off, - LDMigrationStage.Off, - { - kind: 'FALLTHROUGH', - }, - ); - tracker.op('read'); - - tracker.custom({ - kind: 'custom', - key: 'cats', - values: { - old: 3, - new: 10, - }, - }); - - tracker.custom({ - kind: 'custom', - key: 'badgers', - values: { - new: 10, - }, - }); - - const event = tracker.createEvent(); - expect(event?.measurements).toContainEqual({ - key: 'cats', - kind: 'custom', - values: { - old: 3, - new: 10, - }, - }); - expect(event?.measurements).toContainEqual({ - key: 'badgers', - kind: 'custom', - values: { - new: 10, - }, - }); -}); diff --git a/packages/shared/sdk-server/src/MigrationOpEventConversion.ts b/packages/shared/sdk-server/src/MigrationOpEventConversion.ts index 6daa08388..e89f072ef 100644 --- a/packages/shared/sdk-server/src/MigrationOpEventConversion.ts +++ b/packages/shared/sdk-server/src/MigrationOpEventConversion.ts @@ -2,7 +2,6 @@ import { internal, TypeValidators } from '@launchdarkly/js-sdk-common'; import { LDMigrationConsistencyMeasurement, - LDMigrationCustomMeasurement, LDMigrationErrorMeasurement, LDMigrationEvaluation, LDMigrationLatencyMeasurement, @@ -19,10 +18,6 @@ function isOperation(value: LDMigrationOp) { return value === 'read' || value === 'write'; } -function isCustomMeasurement(value: LDMigrationMeasurement): value is LDMigrationCustomMeasurement { - return (value as any).kind === 'custom'; -} - function isLatencyMeasurement( value: LDMigrationMeasurement, ): value is LDMigrationLatencyMeasurement { @@ -54,27 +49,13 @@ function areValidValues(values: { old?: number; new?: number }) { function validateMeasurement( measurement: LDMigrationMeasurement, ): LDMigrationMeasurement | undefined { + // Here we are protecting ourselves from JS callers. TypeScript says that + // it cannot be an empty string, but those using JS can do what they want. + // @ts-ignore if (!TypeValidators.String.is(measurement.key) || measurement.key === '') { return undefined; } - if (isCustomMeasurement(measurement)) { - if (!TypeValidators.Object.is(measurement.values)) { - return undefined; - } - if (!areValidValues(measurement.values)) { - return undefined; - } - return { - kind: measurement.kind, - key: measurement.key, - values: { - old: measurement.values.old, - new: measurement.values.new, - }, - }; - } - if (isLatencyMeasurement(measurement)) { if (!TypeValidators.Object.is(measurement.values)) { return undefined; @@ -121,6 +102,7 @@ function validateMeasurement( }; } + // Not a supported measurement type. return undefined; } diff --git a/packages/shared/sdk-server/src/MigrationOpTracker.ts b/packages/shared/sdk-server/src/MigrationOpTracker.ts index 0d2fc07cc..f269b62b5 100644 --- a/packages/shared/sdk-server/src/MigrationOpTracker.ts +++ b/packages/shared/sdk-server/src/MigrationOpTracker.ts @@ -3,7 +3,6 @@ import { LDEvaluationReason } from '@launchdarkly/js-sdk-common'; import { LDMigrationStage, LDMigrationTracker } from './api'; import { LDConsistencyCheck, - LDMigrationCustomMeasurement, LDMigrationMeasurement, LDMigrationOp, LDMigrationOpEvent, @@ -29,8 +28,6 @@ export default class MigrationOpTracker implements LDMigrationTracker { private operation?: LDMigrationOp; - private customMeasurements: LDMigrationCustomMeasurement[] = []; - constructor( private readonly flagKey: string, private readonly contextKeys: Record, @@ -56,13 +53,9 @@ export default class MigrationOpTracker implements LDMigrationTracker { this.latencyMeasurement[origin] = value; } - custom(measurement: LDMigrationCustomMeasurement) { - this.customMeasurements.push(measurement); - } - createEvent(): LDMigrationOpEvent | undefined { if (this.operation && Object.keys(this.contextKeys).length) { - const measurements = [...this.customMeasurements]; + const measurements: LDMigrationMeasurement[] = []; this.populateConsistency(measurements); this.populateLatency(measurements); diff --git a/packages/shared/sdk-server/src/api/data/LDMigrationDetail.ts b/packages/shared/sdk-server/src/api/data/LDMigrationDetail.ts index 33e56bbf6..78adb1081 100644 --- a/packages/shared/sdk-server/src/api/data/LDMigrationDetail.ts +++ b/packages/shared/sdk-server/src/api/data/LDMigrationDetail.ts @@ -2,7 +2,6 @@ import { LDEvaluationReason } from '@launchdarkly/js-sdk-common'; import { LDMigrationOrigin } from '../LDMigration'; import { - LDMigrationCustomMeasurement, LDMigrationOp, LDMigrationOpEvent, } from './LDMigrationOpEvent'; @@ -52,14 +51,6 @@ export interface LDMigrationTracker { */ latency(origin: LDMigrationOrigin, value: number): void; - /** - * Report a custom measurement. Unlike other methods on the tracker multiple custom - * measurements can be reported. - * - * @param measurement The custom measurement to track. - */ - custom(measurement: LDMigrationCustomMeasurement): void; - /** * Create a migration op event. If the event could not be created, because of a missing * operation, then undefined is returned. diff --git a/packages/shared/sdk-server/src/api/data/LDMigrationOpEvent.ts b/packages/shared/sdk-server/src/api/data/LDMigrationOpEvent.ts index d45980b79..d6f68d649 100644 --- a/packages/shared/sdk-server/src/api/data/LDMigrationOpEvent.ts +++ b/packages/shared/sdk-server/src/api/data/LDMigrationOpEvent.ts @@ -16,15 +16,6 @@ export interface LDMigrationEvaluation { reason: LDEvaluationReason; } -export interface LDMigrationCustomMeasurement { - kind: 'custom'; - key: string; - values: { - old?: number; - new?: number; - }; -} - export interface LDMigrationConsistencyMeasurement { key: 'consistent'; value: number; @@ -53,8 +44,7 @@ export interface LDMigrationErrorMeasurement { export type LDMigrationMeasurement = | LDMigrationLatencyMeasurement | LDMigrationErrorMeasurement - | LDMigrationConsistencyMeasurement - | LDMigrationCustomMeasurement; + | LDMigrationConsistencyMeasurement; /** * Event used to track information about a migration operation. From ef273a74cb70eb066bd777a3d5068727050e735f Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Wed, 16 Aug 2023 13:55:15 -0700 Subject: [PATCH 2/2] Lint. --- .../shared/sdk-server/__tests__/MigratioOpEvent.test.ts | 6 +++--- .../shared/sdk-server/src/api/data/LDMigrationDetail.ts | 5 +---- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/packages/shared/sdk-server/__tests__/MigratioOpEvent.test.ts b/packages/shared/sdk-server/__tests__/MigratioOpEvent.test.ts index 55b5e5375..4b258d5cd 100644 --- a/packages/shared/sdk-server/__tests__/MigratioOpEvent.test.ts +++ b/packages/shared/sdk-server/__tests__/MigratioOpEvent.test.ts @@ -408,7 +408,7 @@ it('ignores invalid measurement keys', () => { ], }; const validatedEvent = MigrationOpEventConversion(inputEvent); - expect(validatedEvent).toEqual({...inputEvent, measurements: []}); + expect(validatedEvent).toEqual({ ...inputEvent, measurements: [] }); }); it('invalid data types are filtered', () => { @@ -437,7 +437,7 @@ it('invalid data types are filtered', () => { { key: 'consistent', // @ts-ignore - value: undefined + value: undefined, }, { key: 'error', @@ -446,7 +446,7 @@ it('invalid data types are filtered', () => { old: {}, new: 2, }, - } + }, ], }; const validatedEvent = MigrationOpEventConversion(inputEvent); diff --git a/packages/shared/sdk-server/src/api/data/LDMigrationDetail.ts b/packages/shared/sdk-server/src/api/data/LDMigrationDetail.ts index 78adb1081..83dd6c3a0 100644 --- a/packages/shared/sdk-server/src/api/data/LDMigrationDetail.ts +++ b/packages/shared/sdk-server/src/api/data/LDMigrationDetail.ts @@ -1,10 +1,7 @@ import { LDEvaluationReason } from '@launchdarkly/js-sdk-common'; import { LDMigrationOrigin } from '../LDMigration'; -import { - LDMigrationOp, - LDMigrationOpEvent, -} from './LDMigrationOpEvent'; +import { LDMigrationOp, LDMigrationOpEvent } from './LDMigrationOpEvent'; import { LDMigrationStage } from './LDMigrationStage'; /**