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

Merged
merged 10 commits into from
Aug 30, 2023
Merged

feat: Add invoked measurement. #258

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c4a118e
feat: Add invoked measurement.
kinyoklion Aug 28, 2023
11ae2a8
Call invoked method from tracker. Add basic test.
kinyoklion Aug 28, 2023
b4709f9
Fix tests for invoked measurement.
kinyoklion Aug 28, 2023
71c4325
Add basic unit tests.
kinyoklion Aug 28, 2023
fc08e8c
Add more robust error handling.
kinyoklion Aug 28, 2023
2d41b50
Prettier
kinyoklion Aug 28, 2023
81a3358
Cleanup log line.
kinyoklion Aug 28, 2023
e1a31d0
Merge branch 'feat/node-migrations' into rlamb/add-invoked-measurement
kinyoklion Aug 28, 2023
5bc9100
Update packages/shared/sdk-server/src/MigrationOpTracker.ts
kinyoklion Aug 30, 2023
69b164e
PR feedback.
kinyoklion Aug 30, 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
1 change: 1 addition & 0 deletions contract-tests/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

],
});
});
Expand Down
4 changes: 2 additions & 2 deletions contract-tests/sdkClientEntity.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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


function getExecution(order) {
switch(order) {
switch (order) {
case 'serial': {
return new LDSerialExecution(LDExecutionOrdering.Fixed);
}
Expand Down Expand Up @@ -164,7 +164,7 @@ export async function newSdkClientEntity(options) {
case 'migrationOperation':
const migrationOperation = params.migrationOperation;
const readExecutionOrder = migrationOperation.readExecutionOrder;

const migration = new Migration(client, {
execution: getExecution(readExecutionOrder),
latencyTracking: migrationOperation.trackLatency,
Expand Down
198 changes: 124 additions & 74 deletions packages/shared/sdk-server/__tests__/MigrationOpEvent.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

// This isn't a precise check, but we should have non-zero values.
expect(migrationEvent.measurements[0].value).toEqual(true);
expect(migrationEvent.measurements[1].value).toEqual(true);
},
);

Expand All @@ -110,9 +110,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');
// This isn't a precise check, but we should have non-zero values.
expect(migrationEvent.measurements[0].value).toEqual(true);
expect(migrationEvent.measurements[1].value).toEqual(true);
expect(internal.shouldSample).toHaveBeenCalledWith(10);
},
);
Expand All @@ -130,7 +130,7 @@ 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.length).toEqual(0);
expect(migrationEvent.measurements.length).toEqual(1);
expect(internal.shouldSample).toHaveBeenCalledWith(12);
},
);
Expand Down Expand Up @@ -163,9 +163,9 @@ describe('given an LDClient with test data', () => {
await events.take();
// Migration event.
const migrationEvent = (await events.take()) as internal.InputMigrationEvent;
expect(migrationEvent.measurements[0].key).toEqual('consistent');
expect(migrationEvent.measurements[1].key).toEqual('consistent');
// This isn't a precise check, but we should have non-zero values.
expect(migrationEvent.measurements[0].value).toEqual(false);
expect(migrationEvent.measurements[1].value).toEqual(false);
},
);
});
Expand All @@ -191,6 +191,44 @@ describe('given an LDClient with test data', () => {
});
});

it.each([
[LDMigrationStage.Off, { old: true }],
[LDMigrationStage.DualWrite, { old: true }],
[LDMigrationStage.Shadow, { old: true, new: true }],
[LDMigrationStage.RampDown, { new: true }],
[LDMigrationStage.Complete, { new: true }],
])('tracks the invoked methods for reads', async (stage, values) => {
const flagKey = 'migration';
td.update(td.flag(flagKey).valueForAll(stage));

await migration.read(flagKey, { key: 'test' }, stage);
// Feature event.
await events.take();
// Migration event.
const migrationEvent = (await events.take()) as internal.InputMigrationEvent;
expect(migrationEvent.measurements[0].key).toEqual('invoked');
expect(migrationEvent.measurements[0].values).toEqual(values);
});

it.each([
[LDMigrationStage.Off, { old: true }],
[LDMigrationStage.DualWrite, { old: true, new: true }],
[LDMigrationStage.Shadow, { old: true, new: true }],
[LDMigrationStage.RampDown, { old: true, new: true }],
[LDMigrationStage.Complete, { new: true }],
])('tracks the invoked methods for writes', async (stage, values) => {
const flagKey = 'migration';
td.update(td.flag(flagKey).valueForAll(stage));

await migration.write(flagKey, { key: 'test' }, stage);
// Feature event.
await events.take();
// Migration event.
const migrationEvent = (await events.take()) as internal.InputMigrationEvent;
expect(migrationEvent.measurements[0].key).toEqual('invoked');
expect(migrationEvent.measurements[0].values).toEqual(values);
});

it.each([LDMigrationStage.Shadow, LDMigrationStage.Live])(
'can report read latency for new and old',
async (stage) => {
Expand All @@ -202,10 +240,10 @@ describe('given an LDClient with test data', () => {
await events.take();
// Migration event.
const migrationEvent = (await events.take()) as internal.InputMigrationEvent;
expect(migrationEvent.measurements[0].key).toEqual('latency_ms');
expect(migrationEvent.measurements[1].key).toEqual('latency_ms');
// This isn't a precise check, but we should have non-zero values.
expect(migrationEvent.measurements[0].values.old).toBeGreaterThanOrEqual(1);
expect(migrationEvent.measurements[0].values.new).toBeGreaterThanOrEqual(1);
expect(migrationEvent.measurements[1].values.old).toBeGreaterThanOrEqual(1);
expect(migrationEvent.measurements[1].values.new).toBeGreaterThanOrEqual(1);
},
);

Expand All @@ -220,10 +258,10 @@ describe('given an LDClient with test data', () => {
await events.take();
// Migration event.
const migrationEvent = (await events.take()) as internal.InputMigrationEvent;
expect(migrationEvent.measurements[0].key).toEqual('latency_ms');
expect(migrationEvent.measurements[1].key).toEqual('latency_ms');
// This isn't a precise check, but we should have non-zero values.
expect(migrationEvent.measurements[0].values.old).toBeGreaterThanOrEqual(1);
expect(migrationEvent.measurements[0].values.new).toBeUndefined();
expect(migrationEvent.measurements[1].values.old).toBeGreaterThanOrEqual(1);
expect(migrationEvent.measurements[1].values.new).toBeUndefined();
},
);

Expand All @@ -238,10 +276,10 @@ describe('given an LDClient with test data', () => {
await events.take();
// Migration event.
const migrationEvent = (await events.take()) as internal.InputMigrationEvent;
expect(migrationEvent.measurements[0].key).toEqual('latency_ms');
expect(migrationEvent.measurements[1].key).toEqual('latency_ms');
// This isn't a precise check, but we should have non-zero values.
expect(migrationEvent.measurements[0].values.new).toBeGreaterThanOrEqual(1);
expect(migrationEvent.measurements[0].values.old).toBeUndefined();
expect(migrationEvent.measurements[1].values.new).toBeGreaterThanOrEqual(1);
expect(migrationEvent.measurements[1].values.old).toBeUndefined();
},
);

Expand All @@ -254,10 +292,10 @@ describe('given an LDClient with test data', () => {
await events.take();
// Migration event.
const migrationEvent = (await events.take()) as internal.InputMigrationEvent;
expect(migrationEvent.measurements[0].key).toEqual('latency_ms');
expect(migrationEvent.measurements[1].key).toEqual('latency_ms');
// This isn't a precise check, but we should have non-zero values.
expect(migrationEvent.measurements[0].values.old).toBeGreaterThanOrEqual(1);
expect(migrationEvent.measurements[0].values.new).toBeUndefined();
expect(migrationEvent.measurements[1].values.old).toBeGreaterThanOrEqual(1);
expect(migrationEvent.measurements[1].values.new).toBeUndefined();
});

it.each([LDMigrationStage.Complete])(
Expand All @@ -271,10 +309,10 @@ describe('given an LDClient with test data', () => {
await events.take();
// Migration event.
const migrationEvent = (await events.take()) as internal.InputMigrationEvent;
expect(migrationEvent.measurements[0].key).toEqual('latency_ms');
expect(migrationEvent.measurements[1].key).toEqual('latency_ms');
// This isn't a precise check, but we should have non-zero values.
expect(migrationEvent.measurements[0].values.new).toBeGreaterThanOrEqual(1);
expect(migrationEvent.measurements[0].values.old).toBeUndefined();
expect(migrationEvent.measurements[1].values.new).toBeGreaterThanOrEqual(1);
expect(migrationEvent.measurements[1].values.old).toBeUndefined();
},
);

Expand All @@ -289,10 +327,10 @@ describe('given an LDClient with test data', () => {
await events.take();
// Migration event.
const migrationEvent = (await events.take()) as internal.InputMigrationEvent;
expect(migrationEvent.measurements[0].key).toEqual('latency_ms');
expect(migrationEvent.measurements[1].key).toEqual('latency_ms');
// This isn't a precise check, but we should have non-zero values.
expect(migrationEvent.measurements[0].values.old).toBeGreaterThanOrEqual(1);
expect(migrationEvent.measurements[0].values.new).toBeGreaterThanOrEqual(1);
expect(migrationEvent.measurements[1].values.old).toBeGreaterThanOrEqual(1);
expect(migrationEvent.measurements[1].values.new).toBeGreaterThanOrEqual(1);
},
);

Expand All @@ -305,10 +343,10 @@ describe('given an LDClient with test data', () => {
await events.take();
// Migration event.
const migrationEvent = (await events.take()) as internal.InputMigrationEvent;
expect(migrationEvent.measurements[0].key).toEqual('latency_ms');
expect(migrationEvent.measurements[1].key).toEqual('latency_ms');
// This isn't a precise check, but we should have non-zero values.
expect(migrationEvent.measurements[0].values.old).toBeGreaterThanOrEqual(1);
expect(migrationEvent.measurements[0].values.new).toBeGreaterThanOrEqual(1);
expect(migrationEvent.measurements[1].values.old).toBeGreaterThanOrEqual(1);
expect(migrationEvent.measurements[1].values.new).toBeGreaterThanOrEqual(1);
});
});

Expand Down Expand Up @@ -338,15 +376,11 @@ 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).toMatchObject({
measurements: [
{
key: 'error',
values: {
old: true,
},
},
],
expect(migrationEvent.measurements).toContainEqual({
key: 'error',
values: {
old: true,
},
});
},
);
Expand All @@ -362,15 +396,11 @@ describe('given an LDClient with test data', () => {
await events.take();
// Migration event.
const migrationEvent = (await events.take()) as internal.InputMigrationEvent;
expect(migrationEvent).toMatchObject({
measurements: [
{
key: 'error',
values: {
new: true,
},
},
],
expect(migrationEvent.measurements).toContainEqual({
key: 'error',
values: {
new: true,
},
});
},
);
Expand All @@ -387,16 +417,12 @@ 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).toMatchObject({
measurements: [
{
key: 'error',
values: {
old: true,
new: true,
},
},
],
expect(migrationEvent.measurements).toContainEqual({
key: 'error',
values: {
old: true,
new: true,
},
});
},
);
Expand All @@ -412,19 +438,47 @@ describe('given an LDClient with test data', () => {
await events.take();
// Migration event.
const migrationEvent = (await events.take()) as internal.InputMigrationEvent;
expect(migrationEvent).toMatchObject({
measurements: [
{
key: 'error',
values: {
old: true,
},
},
],
expect(migrationEvent.measurements).toContainEqual({
key: 'error',
values: {
old: true,
},
});
},
);

it.each([LDMigrationStage.Off, LDMigrationStage.DualWrite, LDMigrationStage.Shadow])(
'it does not invoke non-authoritative write after an error with authoritative old',
async (stage) => {
const flagKey = 'migration';
td.update(td.flag(flagKey).valueForAll(stage));

await migration.write(flagKey, { key: 'test' }, stage);
// Feature event.
await events.take();
// Migration event.
const migrationEvent = (await events.take()) as internal.InputMigrationEvent;
expect(migrationEvent.measurements[0].key).toEqual('invoked');
expect(migrationEvent.measurements[0].values).toEqual({ old: true });
},
);

it.each([LDMigrationStage.Live, LDMigrationStage.RampDown, LDMigrationStage.Complete])(
'it does not invoke non-authoritative write after an error with authoritative new',
async (stage) => {
const flagKey = 'migration';
td.update(td.flag(flagKey).valueForAll(stage));

await migration.write(flagKey, { key: 'test' }, stage);
// Feature event.
await events.take();
// Migration event.
const migrationEvent = (await events.take()) as internal.InputMigrationEvent;
expect(migrationEvent.measurements[0].key).toEqual('invoked');
expect(migrationEvent.measurements[0].values).toEqual({ new: true });
},
);

it.each([LDMigrationStage.Live, LDMigrationStage.RampDown, LDMigrationStage.Complete])(
'can report errors for new writes: %p',
async (stage) => {
Expand All @@ -437,15 +491,11 @@ 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).toMatchObject({
measurements: [
{
key: 'error',
values: {
new: true,
},
},
],
expect(migrationEvent.measurements).toContainEqual({
key: 'error',
values: {
new: true,
},
});
},
);
Expand Down
Loading
Loading