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

feat: Remove custom measurements. #243

Merged
merged 3 commits into from
Aug 16, 2023
Merged

feat: Remove custom measurements. #243

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
027fd06
feat: Remove custom events.
kinyoklion Aug 16, 2023
ef273a7
Lint.
kinyoklion Aug 16, 2023
75d7a18
Merge branch 'feat/node-migrations' into rlamb/remove-custom-events
kinyoklion Aug 16, 2023
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
48 changes: 19 additions & 29 deletions packages/shared/sdk-server/__tests__/MigratioOpEvent.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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',
Expand All @@ -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);
Expand Down
47 changes: 0 additions & 47 deletions packages/shared/sdk-server/__tests__/MigrationOpTracker.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
});
});
26 changes: 4 additions & 22 deletions packages/shared/sdk-server/src/MigrationOpEventConversion.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { internal, TypeValidators } from '@launchdarkly/js-sdk-common';

import {
LDMigrationConsistencyMeasurement,
LDMigrationCustomMeasurement,
LDMigrationErrorMeasurement,
LDMigrationEvaluation,
LDMigrationLatencyMeasurement,
Expand All @@ -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 {
Expand Down Expand Up @@ -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
Comment on lines +52 to +54
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

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;
Expand Down Expand Up @@ -121,6 +102,7 @@ function validateMeasurement(
};
}

// Not a supported measurement type.
return undefined;
}

Expand Down
9 changes: 1 addition & 8 deletions packages/shared/sdk-server/src/MigrationOpTracker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { LDEvaluationReason } from '@launchdarkly/js-sdk-common';
import { LDMigrationStage, LDMigrationTracker } from './api';
import {
LDConsistencyCheck,
LDMigrationCustomMeasurement,
LDMigrationMeasurement,
LDMigrationOp,
LDMigrationOpEvent,
Expand All @@ -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<string, string>,
Expand All @@ -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);
Expand Down
9 changes: 0 additions & 9 deletions packages/shared/sdk-server/src/api/data/LDMigrationDetail.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { LDEvaluationReason } from '@launchdarkly/js-sdk-common';

import { LDMigrationOrigin } from '../LDMigration';
import {
LDMigrationCustomMeasurement,
LDMigrationOp,
LDMigrationOpEvent,
} from './LDMigrationOpEvent';
Expand Down Expand Up @@ -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.
Expand Down
12 changes: 1 addition & 11 deletions packages/shared/sdk-server/src/api/data/LDMigrationOpEvent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -53,8 +44,7 @@ export interface LDMigrationErrorMeasurement {
export type LDMigrationMeasurement =
| LDMigrationLatencyMeasurement
| LDMigrationErrorMeasurement
| LDMigrationConsistencyMeasurement
| LDMigrationCustomMeasurement;
Copy link
Contributor

Choose a reason for hiding this comment

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

So this will break existing applications. This is ok, if the plan is for a major release?

Copy link
Member Author

Choose a reason for hiding this comment

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

None of this is released. All on a feature branch.

Copy link
Member Author

Choose a reason for hiding this comment

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

(If it were, then yes, this would be a major version.)

| LDMigrationConsistencyMeasurement;

/**
* Event used to track information about a migration operation.
Expand Down