fix(metrics): detect key collisions between dimensions, metrics, and metadata

packages/metrics/src/Metrics.ts

@@ -36,6 +36,7 @@ import { MetricsStore } from './MetricsStore.js';
 import type {
   ConfigServiceInterface,
   Dimensions,
+  EmfKeySource,
   EmfOutput,
   ExtraOptions,
   MetricDefinition,
@@ -758,6 +759,14 @@ class Metrics extends Utility implements MetricsInterface {
       dimensionNames.push(Object.keys(defaultDimensions));
     }
 
+    this.#checkKeyCollisions(
+      defaultDimensions,
+      dimensions,
+      dimensionSets,
+      metricValues,
+      this.#metadataStore.getAll()
+    );
+
     return {
       _aws: {
         Timestamp: this.#metricsStore.getTimestamp() ?? Date.now(),
@@ -769,6 +778,10 @@ class Metrics extends Utility implements MetricsInterface {
           },
         ],
       },
+      // Metadata is spread first so that any colliding key from a dimension
+      // source overrides it: dimensions are load-bearing for EMF aggregation,
+      // metadata is incidental context.
+      ...this.#metadataStore.getAll(),
       ...defaultDimensions,
       ...dimensions,
       // Merge all dimension sets efficiently by mutating the accumulator
@@ -779,10 +792,48 @@ class Metrics extends Utility implements MetricsInterface {
         return acc;
       }, {}),
       ...metricValues,
-      ...this.#metadataStore.getAll(),
     };
   }
 
+  #checkKeyCollisions(
+    defaultDimensions: Dimensions,
+    dimensions: Dimensions,
+    dimensionSets: Dimensions[],
+    metricValues: Record<string, number | number[]>,
+    metadata: Record<string, string>
+  ): void {
+    const seenKeys = new Map<string, EmfKeySource>();
+
+    const setKey = (k: string, source: EmfKeySource) => {
+      const existing = seenKeys.get(k);
+      if (existing !== undefined) {
+        this.#logger.warn(
+          `EMF key "${k}" is defined as both a ${existing} and ${source}; the ${source} value will take precedence in the serialized output`
+        );
+      }
+      seenKeys.set(k, source);
+    };
+
+    // Seed in precedence order (lowest to highest). The spread in
+    // serializeMetrics() applies the same order, so the source named in the
+    // warning genuinely wins in the output.
+    for (const k of Object.keys(metadata)) setKey(k, 'metadata');
+    for (const k of Object.keys(defaultDimensions))
+      setKey(k, 'default dimension');
+    for (const k of Object.keys(dimensions)) setKey(k, 'dimension');
+    for (const set of dimensionSets)
+      for (const k of Object.keys(set)) setKey(k, 'dimension set');
+
+    for (const name of Object.keys(metricValues)) {
+      const src = seenKeys.get(name);
+      if (src !== undefined) {
+        throw new Error(
+          `EMF key collision on "${name}": registered as both a metric (number) and a ${src} (string)`
+        );
+      }
+    }
+  }
+
   /**
    * Set default dimensions that will be added to all metrics.
    *

packages/metrics/src/types/Metrics.ts

@@ -13,6 +13,16 @@ import type { ConfigServiceInterface } from './ConfigServiceInterface.js';
  */
 type Dimensions = Record<string, string>;
 
+/**
+ * The source of a key when emitting EMF output, used by the collision
+ * detection in {@link MetricsInterface.serializeMetrics | `serializeMetrics()`}.
+ */
+type EmfKeySource =
+  | 'default dimension'
+  | 'dimension'
+  | 'dimension set'
+  | 'metadata';
+
 /**
  * Options to configure the Metrics class.
  *
@@ -540,6 +550,7 @@ interface MetricsInterface {
 
 export type {
   Dimensions,
+  EmfKeySource,
   EmfOutput,
   ExtraOptions,
   MetricDefinition,

packages/metrics/src/types/index.ts

@@ -1,6 +1,7 @@
 export type { ConfigServiceInterface } from './ConfigServiceInterface.js';
 export type {
   Dimensions,
+  EmfKeySource,
   EmfOutput,
   ExtraOptions,
   MetricDefinition,

packages/metrics/tests/unit/dimensions.test.ts

@@ -728,4 +728,127 @@ describe('Working with dimensions', () => {
     // Assess
     expect(() => metrics.publishStoredMetrics()).not.toThrow();
   });
+
+  it('throws when a metric name conflicts with an existing dimension key', () => {
+    // Prepare
+    const metrics = new Metrics({
+      singleMetric: true,
+    });
+    metrics.addDimension('environment', 'prod');
+
+    // Act & Assess
+    expect(() =>
+      metrics.addMetric('environment', MetricUnit.Count, 1)
+    ).toThrowError(
+      'EMF key collision on "environment": registered as both a metric (number) and a dimension (string)'
+    );
+  });
+
+  it('throws when a metric name conflicts with an existing default dimension key', () => {
+    // Prepare
+    const metrics = new Metrics({
+      singleMetric: true,
+      defaultDimensions: { environment: 'prod' },
+    });
+
+    // Act & Assess
+    expect(() =>
+      metrics.addMetric('environment', MetricUnit.Count, 1)
+    ).toThrowError(
+      'EMF key collision on "environment": registered as both a metric (number) and a default dimension (string)'
+    );
+  });
+
+  it('throws when a metric name conflicts with the built-in service dimension', () => {
+    // Prepare
+    const metrics = new Metrics({
+      singleMetric: true,
+    });
+
+    // Act & Assess
+    expect(() =>
+      metrics.addMetric('service', MetricUnit.Count, 1)
+    ).toThrowError(
+      'EMF key collision on "service": registered as both a metric (number) and a default dimension (string)'
+    );
+  });
+
+  it('throws when a metric name conflicts with a key added via addDimensions', () => {
+    // Prepare
+    const metrics = new Metrics({
+      singleMetric: true,
+    });
+    metrics.addDimensions({ environment: 'prod' });
+
+    // Act & Assess
+    expect(() =>
+      metrics.addMetric('environment', MetricUnit.Count, 1)
+    ).toThrowError(
+      'EMF key collision on "environment": registered as both a metric (number) and a dimension set (string)'
+    );
+  });
+
+  it('throws on serialize when a dimension key is added after a metric with the same name', () => {
+    // Prepare
+    const metrics = new Metrics({ namespace: 'test' });
+    metrics.addMetric('environment', MetricUnit.Count, 1);
+    metrics.addDimension('environment', 'prod');
+
+    // Act & Assess
+    expect(() => metrics.serializeMetrics()).toThrowError(
+      'EMF key collision on "environment": registered as both a metric (number) and a dimension (string)'
+    );
+  });
+
+  it('warns on serialize when a dimension key overwrites a default dimension key', () => {
+    // Prepare
+    const metrics = new Metrics({
+      namespace: 'test',
+      defaultDimensions: { environment: 'prod' },
+    });
+    metrics.addDimension('environment', 'staging');
+    metrics.addMetric('test', MetricUnit.Count, 1);
+
+    // Act
+    metrics.serializeMetrics();
+
+    // Assess
+    expect(console.warn).toHaveBeenCalledWith(
+      'EMF key "environment" is defined as both a default dimension and dimension; the dimension value will take precedence in the serialized output'
+    );
+  });
+
+  it('warns on serialize when a dimension set key overwrites a regular dimension key', () => {
+    // Prepare
+    const metrics = new Metrics({ namespace: 'test' });
+    metrics.addDimension('environment', 'prod');
+    metrics.addDimensions({ environment: 'staging' });
+    metrics.addMetric('test', MetricUnit.Count, 1);
+
+    // Act
+    metrics.serializeMetrics();
+
+    // Assess
+    expect(console.warn).toHaveBeenCalledWith(
+      'EMF key "environment" is defined as both a dimension and dimension set; the dimension set value will take precedence in the serialized output'
+    );
+  });
+
+  it('warns on serialize when a dimension set key overwrites a default dimension key', () => {
+    // Prepare
+    const metrics = new Metrics({
+      namespace: 'test',
+      defaultDimensions: { environment: 'prod' },
+    });
+    metrics.addDimensions({ environment: 'staging' });
+    metrics.addMetric('test', MetricUnit.Count, 1);
+
+    // Act
+    metrics.serializeMetrics();
+
+    // Assess
+    expect(console.warn).toHaveBeenCalledWith(
+      'EMF key "environment" is defined as both a default dimension and dimension set; the dimension set value will take precedence in the serialized output'
+    );
+  });
 });

packages/metrics/tests/unit/metadata.test.ts

@@ -86,4 +86,98 @@ describe('Working with metadata', () => {
       expect.not.objectContaining({ 'cost-center': '1234' })
     );
   });
+
+  it('throws on serialize when metadata key and metric name collide (metric first)', () => {
+    // Prepare
+    const metrics = new Metrics({ namespace: 'test' });
+    metrics.addMetric('request_count', MetricUnit.Count, 42);
+    metrics.addMetadata('request_count', 'not-a-number');
+
+    // Act & Assess
+    expect(() => metrics.serializeMetrics()).toThrowError(
+      'EMF key collision on "request_count": registered as both a metric (number) and a metadata (string)'
+    );
+  });
+
+  it('throws on serialize when metadata key and metric name collide (metadata first)', () => {
+    // Prepare
+    const metrics = new Metrics({ namespace: 'test' });
+    metrics.addMetadata('request_count', 'not-a-number');
+    metrics.addMetric('request_count', MetricUnit.Count, 42);
+
+    // Act & Assess
+    expect(() => metrics.serializeMetrics()).toThrowError(
+      'EMF key collision on "request_count": registered as both a metric (number) and a metadata (string)'
+    );
+  });
+
+  it('warns on serialize when a metadata key matches a dimension key, and the dimension wins', () => {
+    // Prepare
+    const metrics = new Metrics({ namespace: 'test' });
+    metrics.addDimension('environment', 'prod');
+    metrics.addMetadata('environment', 'metadata-value');
+    metrics.addMetric('test', MetricUnit.Count, 1);
+
+    // Act
+    const serialized = metrics.serializeMetrics();
+
+    // Assess
+    expect(console.warn).toHaveBeenCalledWith(
+      'EMF key "environment" is defined as both a metadata and dimension; the dimension value will take precedence in the serialized output'
+    );
+    expect(serialized).toMatchObject({ environment: 'prod' });
+  });
+
+  it('warns on serialize when a metadata key matches a default dimension key, and the default dimension wins', () => {
+    // Prepare
+    const metrics = new Metrics({
+      namespace: 'test',
+      defaultDimensions: { environment: 'prod' },
+    });
+    metrics.addMetadata('environment', 'metadata-value');
+    metrics.addMetric('test', MetricUnit.Count, 1);
+
+    // Act
+    const serialized = metrics.serializeMetrics();
+
+    // Assess
+    expect(console.warn).toHaveBeenCalledWith(
+      'EMF key "environment" is defined as both a metadata and default dimension; the default dimension value will take precedence in the serialized output'
+    );
+    expect(serialized).toMatchObject({ environment: 'prod' });
+  });
+
+  it('warns on serialize when a metadata key matches a dimension set key, and the dimension set wins', () => {
+    // Prepare
+    const metrics = new Metrics({ namespace: 'test' });
+    metrics.addDimensions({ environment: 'prod' });
+    metrics.addMetadata('environment', 'metadata-value');
+    metrics.addMetric('test', MetricUnit.Count, 1);
+
+    // Act
+    const serialized = metrics.serializeMetrics();
+
+    // Assess
+    expect(console.warn).toHaveBeenCalledWith(
+      'EMF key "environment" is defined as both a metadata and dimension set; the dimension set value will take precedence in the serialized output'
+    );
+    expect(serialized).toMatchObject({ environment: 'prod' });
+  });
+
+  it('still lets the dimension win and warns even when addMetadata is called before addDimension', () => {
+    // Prepare
+    const metrics = new Metrics({ namespace: 'test' });
+    metrics.addMetadata('environment', 'metadata-value');
+    metrics.addDimension('environment', 'prod');
+    metrics.addMetric('test', MetricUnit.Count, 1);
+
+    // Act
+    const serialized = metrics.serializeMetrics();
+
+    // Assess
+    expect(console.warn).toHaveBeenCalledWith(
+      'EMF key "environment" is defined as both a metadata and dimension; the dimension value will take precedence in the serialized output'
+    );
+    expect(serialized).toMatchObject({ environment: 'prod' });
+  });
 });