From 8a07c48e9d96f3cffff2ba0355e6319084e0f40c Mon Sep 17 00:00:00 2001 From: Aaron Michael Lamb Date: Mon, 22 Nov 2021 03:11:53 -0800 Subject: [PATCH 1/2] fix: prevent putDimensions from storing duplicate dimensions Conditions previously checked for any dimension set which did not match before adding the input dimension set. This would allow different-sized dimension sets from being mistaken as non-duplicate and being added (see unit test for example). New conditions check for any dimension set which does match before skipping adding the input dimension set to the collection. [TESTING] Unit test updated to address edge case; multiple dimension sets of variable size are added and checked. --- src/logger/MetricsContext.ts | 43 +++++++----------- src/logger/__tests__/MetricsContext.test.ts | 49 +++++++++------------ 2 files changed, 39 insertions(+), 53 deletions(-) diff --git a/src/logger/MetricsContext.ts b/src/logger/MetricsContext.ts index 502f0b2..dd9599f 100644 --- a/src/logger/MetricsContext.ts +++ b/src/logger/MetricsContext.ts @@ -58,22 +58,22 @@ export class MetricsContext { dimensions?: Array>, defaultDimensions?: Record, shouldUseDefaultDimensions?: boolean, - timestamp?: Date | number + timestamp?: Date | number, ) { - this.namespace = namespace || Configuration.namespace + this.namespace = namespace || Configuration.namespace; this.properties = properties || {}; this.dimensions = dimensions || []; this.timestamp = timestamp; this.meta.Timestamp = MetricsContext.resolveMetaTimestamp(timestamp); this.defaultDimensions = defaultDimensions || {}; if (shouldUseDefaultDimensions != undefined) { - this.shouldUseDefaultDimensions = shouldUseDefaultDimensions + this.shouldUseDefaultDimensions = shouldUseDefaultDimensions; } } private static resolveMetaTimestamp(timestamp?: Date | number): number { if (timestamp instanceof Date) { - return timestamp.getTime() + return timestamp.getTime(); } else if (timestamp) { return timestamp; } else { @@ -111,33 +111,24 @@ export class MetricsContext { * @param dimensions */ public putDimensions(incomingDimensionSet: Record): void { - if (this.dimensions.length === 0) { - this.dimensions.push(incomingDimensionSet); - return; - } - - for (let i = 0; i < this.dimensions.length; i++) { - const existingDimensionSet = this.dimensions[i]; + const incomingDimensionSetKeys = Object.keys(incomingDimensionSet); - // check for duplicate dimensions when putting - // this is an O(n^2) operation, but since we never expect to have more than - // 10 dimensions, this is acceptable for almost all cases. - // This makes re-using loggers much easier. + // This operation is O(n^2), but acceptable given sets are capped at 10 dimensions + const doesDimensionSetExist = this.dimensions.some(existingDimensionSet => { const existingDimensionSetKeys = Object.keys(existingDimensionSet); - const incomingDimensionSetKeys = Object.keys(incomingDimensionSet); if (existingDimensionSetKeys.length !== incomingDimensionSetKeys.length) { - this.dimensions.push(incomingDimensionSet); - return; + return false; } + return existingDimensionSetKeys.every(existingDimensionSetKey => + incomingDimensionSetKeys.includes(existingDimensionSetKey), + ); + }); - for (let j = 0; j < existingDimensionSetKeys.length; j++) { - if (!incomingDimensionSetKeys.includes(existingDimensionSetKeys[j])) { - // we're done now because we know that the dimensions keys are not identical - this.dimensions.push(incomingDimensionSet); - return; - } - } + if (doesDimensionSetExist) { + return; } + + this.dimensions.push(incomingDimensionSet); } /** @@ -196,7 +187,7 @@ export class MetricsContext { Object.assign([], this.dimensions), this.defaultDimensions, this.shouldUseDefaultDimensions, - this.timestamp + this.timestamp, ); } } diff --git a/src/logger/__tests__/MetricsContext.test.ts b/src/logger/__tests__/MetricsContext.test.ts index ee9d4f6..3484aea 100644 --- a/src/logger/__tests__/MetricsContext.test.ts +++ b/src/logger/__tests__/MetricsContext.test.ts @@ -16,7 +16,7 @@ test('can set property', () => { expect(actualValue).toBe(expectedValue); }); -test('putDimension adds key to dimension and sets the dimension as a property', () => { +test('putDimensions adds key to dimension and sets the dimension as a property', () => { // arrange const context = MetricsContext.empty(); const dimension = faker.random.word(); @@ -29,51 +29,46 @@ test('putDimension adds key to dimension and sets the dimension as a property', expect(context.getDimensions()[0]).toStrictEqual(expectedDimension); }); -test('putDimension will not duplicate dimensions', () => { +test('putDimensions accepts multiple unique dimension sets', () => { // arrange const context = MetricsContext.empty(); - const dimension = faker.random.word(); - const expectedDimension = { dimension }; + const expectedDimension1 = { d1: faker.random.word(), d2: faker.random.word() }; + const expectedDimension2 = { d2: faker.random.word(), d3: faker.random.word() }; // act - context.putDimensions({ dimension }); - context.putDimensions({ dimension }); + context.putDimensions(expectedDimension1); + context.putDimensions(expectedDimension2); // assert - expect(context.getDimensions().length).toBe(1); - expect(context.getDimensions()[0]).toStrictEqual(expectedDimension); + expect(context.getDimensions().length).toBe(2); + expect(context.getDimensions()[0]).toStrictEqual(expectedDimension1); + expect(context.getDimensions()[1]).toStrictEqual(expectedDimension2); }); -test('putDimension will not duplicate dimensions, multiple in different order', () => { +test('putDimensions will not duplicate dimensions', () => { // arrange const context = MetricsContext.empty(); const dimension1 = faker.random.word(); const dimension2 = faker.random.word(); - const expectedDimension = { dimension1, dimension2 }; + const expectedDimension1 = { dimension1 }; + const expectedDimension2 = { dimension1, dimension2 }; + const expectedDimension3 = { dimension2 }; // act + context.putDimensions({ dimension1 }); context.putDimensions({ dimension1, dimension2 }); context.putDimensions({ dimension2, dimension1 }); + context.putDimensions({ dimension2 }); + context.putDimensions({ dimension1 }); + context.putDimensions({ dimension1, dimension2 }); + context.putDimensions({ dimension2, dimension1 }); + context.putDimensions({ dimension2 }); // assert - expect(context.getDimensions().length).toBe(1); - expect(context.getDimensions()[0]).toStrictEqual(expectedDimension); -}); - -test('putDimension accepts multiple unique dimension sets', () => { - // arrange - const context = MetricsContext.empty(); - const expectedDimension1 = { d1: faker.random.word(), d2: faker.random.word() }; - const expectedDimension2 = { d2: faker.random.word(), d3: faker.random.word() }; - - // act - context.putDimensions(expectedDimension1); - context.putDimensions(expectedDimension2); - - // assert - expect(context.getDimensions().length).toBe(2); + expect(context.getDimensions().length).toBe(3); expect(context.getDimensions()[0]).toStrictEqual(expectedDimension1); expect(context.getDimensions()[1]).toStrictEqual(expectedDimension2); + expect(context.getDimensions()[2]).toStrictEqual(expectedDimension3); }); test('getDimensions returns default dimensions if custom dimensions not set', () => { @@ -201,5 +196,5 @@ test('createCopyWithContext copies shouldUseDefaultDimensions', () => { // assert expect(newContext).not.toBe(context); - expect(newContext.getDimensions()).toEqual([]) + expect(newContext.getDimensions()).toEqual([]); }); From a9705f2cef5efcceec84f257bd38cb5d71485bbf Mon Sep 17 00:00:00 2001 From: Aaron Michael Lamb Date: Mon, 22 Nov 2021 12:49:39 -0800 Subject: [PATCH 2/2] fix: change putDimensions to update/sort existing dimension sets when duplicate This change ensures new dimension key-values are used for the EMF root node by removing duplicate dimension sets and adding input dimension set to the end of the collection. Example: ``` [ { "keyA": "value1" }, { "keyA": "value2" }, ] // expected EMF target member: { "keyA": "value2 } ``` [TESTING] Updated unit tests to check for this chase wherein putDimensions may be triggered using various dimension set lengths, values, and order. --- src/logger/MetricsContext.ts | 15 +++----- src/logger/__tests__/MetricsContext.test.ts | 42 +++++++++++++++++++-- 2 files changed, 44 insertions(+), 13 deletions(-) diff --git a/src/logger/MetricsContext.ts b/src/logger/MetricsContext.ts index dd9599f..79143ac 100644 --- a/src/logger/MetricsContext.ts +++ b/src/logger/MetricsContext.ts @@ -111,23 +111,20 @@ export class MetricsContext { * @param dimensions */ public putDimensions(incomingDimensionSet: Record): void { - const incomingDimensionSetKeys = Object.keys(incomingDimensionSet); - + // Duplicate dimensions sets are removed before being added to the end of the collection. + // This ensures the latest dimension key-value is used as a target member on the root EMF node. // This operation is O(n^2), but acceptable given sets are capped at 10 dimensions - const doesDimensionSetExist = this.dimensions.some(existingDimensionSet => { + const incomingDimensionSetKeys = Object.keys(incomingDimensionSet); + this.dimensions = this.dimensions.filter(existingDimensionSet => { const existingDimensionSetKeys = Object.keys(existingDimensionSet); if (existingDimensionSetKeys.length !== incomingDimensionSetKeys.length) { - return false; + return true; } - return existingDimensionSetKeys.every(existingDimensionSetKey => + return !existingDimensionSetKeys.every(existingDimensionSetKey => incomingDimensionSetKeys.includes(existingDimensionSetKey), ); }); - if (doesDimensionSetExist) { - return; - } - this.dimensions.push(incomingDimensionSet); } diff --git a/src/logger/__tests__/MetricsContext.test.ts b/src/logger/__tests__/MetricsContext.test.ts index 3484aea..ee70a02 100644 --- a/src/logger/__tests__/MetricsContext.test.ts +++ b/src/logger/__tests__/MetricsContext.test.ts @@ -50,25 +50,59 @@ test('putDimensions will not duplicate dimensions', () => { const context = MetricsContext.empty(); const dimension1 = faker.random.word(); const dimension2 = faker.random.word(); - const expectedDimension1 = { dimension1 }; - const expectedDimension2 = { dimension1, dimension2 }; - const expectedDimension3 = { dimension2 }; + const expectedDimension1 = {}; + const expectedDimension2 = { dimension1 }; + const expectedDimension3 = { dimension2, dimension1 }; + const expectedDimension4 = { dimension2 }; // act + context.putDimensions({}); context.putDimensions({ dimension1 }); context.putDimensions({ dimension1, dimension2 }); context.putDimensions({ dimension2, dimension1 }); context.putDimensions({ dimension2 }); + context.putDimensions({}); context.putDimensions({ dimension1 }); context.putDimensions({ dimension1, dimension2 }); context.putDimensions({ dimension2, dimension1 }); context.putDimensions({ dimension2 }); // assert - expect(context.getDimensions().length).toBe(3); + expect(context.getDimensions().length).toBe(4); expect(context.getDimensions()[0]).toStrictEqual(expectedDimension1); expect(context.getDimensions()[1]).toStrictEqual(expectedDimension2); expect(context.getDimensions()[2]).toStrictEqual(expectedDimension3); + expect(context.getDimensions()[3]).toStrictEqual(expectedDimension4); +}); + +test('putDimensions will sort dimensions correctly', () => { + // arrange + const context = MetricsContext.empty(); + const dimension1 = faker.random.word(); + const dimension2 = faker.random.word(); + const expectedDimension1 = { dimension2, dimension1 }; + const expectedDimension2 = { dimension2 }; + const expectedDimension3 = { dimension1 }; + const expectedDimension4 = {}; + + // act + context.putDimensions({}); + context.putDimensions({ dimension1 }); + context.putDimensions({ dimension1, dimension2 }); + context.putDimensions({ dimension2, dimension1 }); + context.putDimensions({ dimension2 }); + context.putDimensions({ dimension1, dimension2 }); + context.putDimensions({ dimension2, dimension1 }); + context.putDimensions({ dimension2 }); + context.putDimensions({ dimension1 }); + context.putDimensions({}); + + // assert + expect(context.getDimensions().length).toBe(4); + expect(context.getDimensions()[0]).toStrictEqual(expectedDimension1); + expect(context.getDimensions()[1]).toStrictEqual(expectedDimension2); + expect(context.getDimensions()[2]).toStrictEqual(expectedDimension3); + expect(context.getDimensions()[3]).toStrictEqual(expectedDimension4); }); test('getDimensions returns default dimensions if custom dimensions not set', () => {