-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat: Add invoked measurement. #258
Conversation
@@ -29,6 +29,7 @@ app.get('/', (req, res) => { | |||
'big-segments', | |||
'user-type', | |||
'migrations', | |||
'event-sampling', |
There was a problem hiding this comment.
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) { | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prettier attacked this file.
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
@@ -20,6 +21,11 @@ export default class MigrationOpTracker implements LDMigrationTracker { | |||
new: false, | |||
}; | |||
|
|||
private wasInvoked = { |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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) { | |||
} |
There was a problem hiding this comment.
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
Co-authored-by: Yusinto Ngadiman <[email protected]>
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.