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.

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: Add invoked measurement. #258

Merged
merged 10 commits into from
Aug 30, 2023

Conversation

kinyoklion
Copy link
Member

@kinyoklion kinyoklion commented Aug 28, 2023

The migration_op event now has a measurement for which "origins" were invoked. Basically was the 'new' method, the 'old' method, or both executed.

This is because from other data you cannot safely infer what was executed. We need to know total executions in order to properly analyze results.

@@ -29,6 +29,7 @@ app.get('/', (req, res) => {
'big-segments',
'user-type',
'migrations',
'event-sampling',
Copy link
Member Author

Choose a reason for hiding this comment

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

To opt-in to the event-sampling tests in the contract test.

@@ -64,7 +64,7 @@ export function makeSdkConfig(options, tag) {
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Prettier attacked this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Prettier kindly reformatted this file

@@ -91,9 +91,9 @@ describe('given an LDClient with test data', () => {
// Migration event.
const migrationEvent = (await events.take()) as internal.InputMigrationEvent;
// Only check the measurements component of the event.
expect(migrationEvent.measurements[0].key).toEqual('consistent');
expect(migrationEvent.measurements[1].key).toEqual('consistent');
Copy link
Member Author

Choose a reason for hiding this comment

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

Adding the new measurement consistently shifted things.

};
}

private logTag(): string {
Copy link
Member Author

Choose a reason for hiding this comment

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

I added various log messages for things that an be wrong with an event. These basically only apply if you are using the blocks but not the kit.

@@ -302,6 +302,7 @@ export default class Migration<
origin: LDMigrationOrigin,
method: (payload?: TInput) => Promise<LDMethodResult<TOutput>>,
): Promise<LDMigrationResult<TOutput>> {
context.tracker.invoked(origin);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the call that actually causes the tracker method to be invoked in all cases.

return undefined;
}

if (!this.wasInvoked.old && !this.wasInvoked.new) {
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 neither the old or the new implementation was invoked, then that isn't valid, and we will not create the event.

@kinyoklion kinyoklion marked this pull request as ready for review August 28, 2023 22:52
@@ -20,6 +21,11 @@ export default class MigrationOpTracker implements LDMigrationTracker {
new: false,
};

private wasInvoked = {
Copy link
Member Author

Choose a reason for hiding this comment

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

Used to track which implementations were executed.

@@ -38,6 +44,7 @@ export default class MigrationOpTracker implements LDMigrationTracker {
private readonly checkRatio?: number,
private readonly variation?: number,
private readonly samplingRatio?: number,
private readonly logger?: LDLogger,
Copy link
Member Author

Choose a reason for hiding this comment

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

Added the logger so we can provide some useful information for invalid cases. Tracking what method was invoked allows us to perform some consistency checks on the data and log when there is an issue.

measurements,
samplingRatio: this.samplingRatio ?? 1,
};
if (!this.operation) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Inverted the logic here to have early returns.

@@ -64,7 +64,7 @@ export function makeSdkConfig(options, tag) {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Prettier kindly reformatted this file

packages/shared/sdk-server/src/MigrationOpTracker.ts Outdated Show resolved Hide resolved
@kinyoklion kinyoklion merged commit ee4ebbf into feat/node-migrations Aug 30, 2023
13 checks passed
@kinyoklion kinyoklion deleted the rlamb/add-invoked-measurement branch August 30, 2023 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants