Repository: awslabs/aws-embedded-metrics-node
Pull request #104 (2021-11-22 19:27:15)

fix: prevent putDimensions from storing duplicate dimensions
## Issue

https://github.com/awslabs/aws-embedded-metrics-node/issues/20 (previous, related issue)

## Description of changes

It appears a prior change addressing this issue may be allowing both variably-sized dimensions sets, and dimension sets with different keys, to be added, even if they are true duplicates of existing sets in the collection. Examples:

```
[
  ['A', 'B'],
  ['A', 'B', C'],
  ['A', 'B'], // this would be considered acceptable
]

// Output: 3 dimension sets (should be 2)
```

```
[
  ['A', 'B'],
  ['B', 'C'],
  ['A', 'B'], // this would be considered acceptable
]

// Output: 3 dimension sets (should be 2)
```

This change fixes that aforementioned behavior and adds an additional behavior:
1. duplicate dimension sets replace (and are sorted after) existing dimension sets such
that newer/latest dimension values are used by the root EMF node

Example:

```
putDimensions({ "keyA": "valueA1" })
putDimensions({ "keyA": "valueA2" })
putDimensions({ "keyA": "valueA3", "keyB": "valueB1" })
putDimensions({ "keyB": "valueB2", "keyA": "valueA4" })

// expected MetricsDirective dimensions:
[
  { "keyA": "valueA2" },
  { "keyB": "valueB2", "keyA": "valueA4" },
]

// expected EMF target members:
{
  "keyA": "valueA4",
  "keyB": "valueB2",
}
```

## Description of how you validated changes

Unit tests were updated to check for these two edge-cases.

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.


markkuhn: Another way to solve this would have been to use a `Map` for dimensions instead of `Record`.

Patch:
diff --git a/src/logger/MetricsContext.ts b/src/logger/MetricsContext.ts
index 1cbbe03..461c82d 100644
--- a/src/logger/MetricsContext.ts
+++ b/src/logger/MetricsContext.ts
@@ -124,35 +124,23 @@ export class MetricsContext {
    * @param dimensions
    */
   public putDimensions(incomingDimensionSet: Record<string, string>): void {
-    MetricsContext.validateDimensionSet(incomingDimensionSet)
+    MetricsContext.validateDimensionSet(incomingDimensionSet);
 
-    if (this.dimensions.length === 0) {
-      this.dimensions.push(incomingDimensionSet);
-      return;
-    }
-
-    for (let i = 0; i < this.dimensions.length; i++) {
-      const existingDimensionSet = this.dimensions[i];
-
-      // 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.
+    // 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 incomingDimensionSetKeys = Object.keys(incomingDimensionSet);
+    this.dimensions = this.dimensions.filter(existingDimensionSet => {
       const existingDimensionSetKeys = Object.keys(existingDimensionSet);
-      const incomingDimensionSetKeys = Object.keys(incomingDimensionSet);
       if (existingDimensionSetKeys.length !== incomingDimensionSetKeys.length) {
-        this.dimensions.push(incomingDimensionSet);
-        return;
+        return true;
       }
+      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;
-        }
-      }
-    }
+    this.dimensions.push(incomingDimensionSet);
   }
 
   /**


Test patch:
diff --git a/src/logger/__tests__/MetricsContext.test.ts b/src/logger/__tests__/MetricsContext.test.ts
index 5b09d83..a01dc91 100644
--- a/src/logger/__tests__/MetricsContext.test.ts
+++ b/src/logger/__tests__/MetricsContext.test.ts
@@ -31,6 +31,7 @@ test('setDimensions allows 30 dimensions', () => {
 });
 
 test('putDimension adds key to dimension and sets the dimension as a property', () => {
+
   // arrange
   const context = MetricsContext.empty();
   const dimension = faker.random.word();
@@ -43,51 +44,80 @@ 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 = {};
+  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(1);
-  expect(context.getDimensions()[0]).toStrictEqual(expectedDimension);
+  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('putDimension accepts multiple unique dimension sets', () => {
+test('putDimensions will sort dimensions correctly', () => {
   // 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() };
+  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(expectedDimension1);
-  context.putDimensions(expectedDimension2);
+  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(2);
+  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', () => {