From 602da083e7beb4e22e2131b755f50ba833fd1240 Mon Sep 17 00:00:00 2001 From: Zelys Date: Mon, 11 May 2026 14:01:03 -0500 Subject: [PATCH 1/6] fix(metrics): detect key collisions between dimensions, metrics, and metadata --- packages/metrics/src/Metrics.ts | 14 +++++ .../metrics/tests/unit/dimensions.test.ts | 59 +++++++++++++++++++ packages/metrics/tests/unit/metadata.test.ts | 13 ++++ 3 files changed, 86 insertions(+) diff --git a/packages/metrics/src/Metrics.ts b/packages/metrics/src/Metrics.ts index 64f1540945..be34372f70 100644 --- a/packages/metrics/src/Metrics.ts +++ b/packages/metrics/src/Metrics.ts @@ -320,6 +320,10 @@ class Metrics extends Utility implements MetricsInterface { * @param value - The value of the metadata */ public addMetadata(key: string, value: string): this { + if (this.#metricsStore.getMetric(key) !== undefined) + throw new Error( + `Metadata key "${key}" conflicts with an existing metric name and would overwrite it in the EMF output` + ); this.#metadataStore.set(key, value); return this; } @@ -1067,6 +1071,16 @@ class Metrics extends Utility implements MetricsInterface { ).join(',')}` ); + const dimensionKeys = new Set([ + ...Object.keys(this.#dimensionsStore.getDimensions()), + ...Object.keys(this.#dimensionsStore.getDefaultDimensions()), + ...this.#dimensionsStore.getDimensionSets().flatMap(Object.keys), + ]); + if (dimensionKeys.has(name)) + throw new Error( + `Metric name "${name}" conflicts with an existing dimension key and would overwrite it in the EMF output` + ); + if (this.#metricsStore.getMetricsCount() >= MAX_METRICS_SIZE) { this.publishStoredMetrics(); } diff --git a/packages/metrics/tests/unit/dimensions.test.ts b/packages/metrics/tests/unit/dimensions.test.ts index 774a748795..6edefacf21 100644 --- a/packages/metrics/tests/unit/dimensions.test.ts +++ b/packages/metrics/tests/unit/dimensions.test.ts @@ -647,4 +647,63 @@ describe('Working with dimensions', () => { expect.not.objectContaining({ [name]: value }) ); }); + + 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( + 'Metric name "environment" conflicts with an existing dimension key' + ); + }); + + 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( + 'Metric name "environment" conflicts with an existing dimension key' + ); + }); + + 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( + 'Metric name "service" conflicts with an existing dimension key' + ); + }); + + 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( + 'Metric name "environment" conflicts with an existing dimension key' + ); + }); }); diff --git a/packages/metrics/tests/unit/metadata.test.ts b/packages/metrics/tests/unit/metadata.test.ts index 5b0353f0c5..a8ee12120e 100644 --- a/packages/metrics/tests/unit/metadata.test.ts +++ b/packages/metrics/tests/unit/metadata.test.ts @@ -86,4 +86,17 @@ describe('Working with metadata', () => { expect.not.objectContaining({ 'cost-center': '1234' }) ); }); + + it('throws when a metadata key conflicts with an existing metric name', () => { + // Prepare + const metrics = new Metrics({ namespace: 'test' }); + metrics.addMetric('request_count', MetricUnit.Count, 42); + + // Act & Assess + expect(() => + metrics.addMetadata('request_count', 'not-a-number') + ).toThrowError( + 'Metadata key "request_count" conflicts with an existing metric name' + ); + }); }); From 345315ac626ac291fa427fc366a9b830495f9232 Mon Sep 17 00:00:00 2001 From: Zelys Date: Sun, 17 May 2026 12:00:22 -0500 Subject: [PATCH 2/6] fix(metrics): centralize EMF key collision check in serializeMetrics() Per reviewer feedback: move the type-change collision guard from individual mutators (storeMetric, addMetadata) to serializeMetrics(), where all string and numeric sources are in scope simultaneously. The prior approach only caught two of the four collision directions (metric-after-dimension and metadata-after-metric). The centralized check catches all four regardless of call order, and throws only on type-change collisions (metric number vs. string source) not same-type overwrites, which CloudWatch handles silently. New error format: 'EMF key collision on "": registered as both a metric (number) and a (string)' where source is 'dimension', 'default dimension', 'dimension set', or 'metadata'. Tests updated to assert at serializeMetrics() time; two reverse-direction tests added (dimension-after-metric, metadata-before-metric). Co-Authored-By: Claude Sonnet 4.6 --- packages/metrics/src/Metrics.ts | 36 +++++++++++-------- .../metrics/tests/unit/dimensions.test.ts | 20 ++++++++--- packages/metrics/tests/unit/metadata.test.ts | 21 ++++++++--- 3 files changed, 54 insertions(+), 23 deletions(-) diff --git a/packages/metrics/src/Metrics.ts b/packages/metrics/src/Metrics.ts index be34372f70..fc8dafe00e 100644 --- a/packages/metrics/src/Metrics.ts +++ b/packages/metrics/src/Metrics.ts @@ -320,10 +320,6 @@ class Metrics extends Utility implements MetricsInterface { * @param value - The value of the metadata */ public addMetadata(key: string, value: string): this { - if (this.#metricsStore.getMetric(key) !== undefined) - throw new Error( - `Metadata key "${key}" conflicts with an existing metric name and would overwrite it in the EMF output` - ); this.#metadataStore.set(key, value); return this; } @@ -749,6 +745,28 @@ class Metrics extends Utility implements MetricsInterface { dimensionNames.push(Object.keys(defaultDimensions)); } + type StringSource = + | 'dimension' + | 'default dimension' + | 'dimension set' + | 'metadata'; + const stringKeys = new Map(); + for (const k of Object.keys(defaultDimensions)) + stringKeys.set(k, 'default dimension'); + for (const k of Object.keys(dimensions)) stringKeys.set(k, 'dimension'); + for (const set of dimensionSets) + for (const k of Object.keys(set)) stringKeys.set(k, 'dimension set'); + for (const k of Object.keys(this.#metadataStore.getAll())) + stringKeys.set(k, 'metadata'); + for (const name of Object.keys(metricValues)) { + const source = stringKeys.get(name); + if (source !== undefined) { + throw new Error( + `EMF key collision on "${name}": registered as both a metric (number) and a ${source} (string)` + ); + } + } + return { _aws: { Timestamp: this.#metricsStore.getTimestamp() ?? Date.now(), @@ -1071,16 +1089,6 @@ class Metrics extends Utility implements MetricsInterface { ).join(',')}` ); - const dimensionKeys = new Set([ - ...Object.keys(this.#dimensionsStore.getDimensions()), - ...Object.keys(this.#dimensionsStore.getDefaultDimensions()), - ...this.#dimensionsStore.getDimensionSets().flatMap(Object.keys), - ]); - if (dimensionKeys.has(name)) - throw new Error( - `Metric name "${name}" conflicts with an existing dimension key and would overwrite it in the EMF output` - ); - if (this.#metricsStore.getMetricsCount() >= MAX_METRICS_SIZE) { this.publishStoredMetrics(); } diff --git a/packages/metrics/tests/unit/dimensions.test.ts b/packages/metrics/tests/unit/dimensions.test.ts index 6edefacf21..d5dffb4ac0 100644 --- a/packages/metrics/tests/unit/dimensions.test.ts +++ b/packages/metrics/tests/unit/dimensions.test.ts @@ -659,7 +659,7 @@ describe('Working with dimensions', () => { expect(() => metrics.addMetric('environment', MetricUnit.Count, 1) ).toThrowError( - 'Metric name "environment" conflicts with an existing dimension key' + 'EMF key collision on "environment": registered as both a metric (number) and a dimension (string)' ); }); @@ -674,7 +674,7 @@ describe('Working with dimensions', () => { expect(() => metrics.addMetric('environment', MetricUnit.Count, 1) ).toThrowError( - 'Metric name "environment" conflicts with an existing dimension key' + 'EMF key collision on "environment": registered as both a metric (number) and a default dimension (string)' ); }); @@ -688,7 +688,7 @@ describe('Working with dimensions', () => { expect(() => metrics.addMetric('service', MetricUnit.Count, 1) ).toThrowError( - 'Metric name "service" conflicts with an existing dimension key' + 'EMF key collision on "service": registered as both a metric (number) and a default dimension (string)' ); }); @@ -703,7 +703,19 @@ describe('Working with dimensions', () => { expect(() => metrics.addMetric('environment', MetricUnit.Count, 1) ).toThrowError( - 'Metric name "environment" conflicts with an existing dimension key' + '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)' ); }); }); diff --git a/packages/metrics/tests/unit/metadata.test.ts b/packages/metrics/tests/unit/metadata.test.ts index a8ee12120e..7d8d185667 100644 --- a/packages/metrics/tests/unit/metadata.test.ts +++ b/packages/metrics/tests/unit/metadata.test.ts @@ -87,16 +87,27 @@ describe('Working with metadata', () => { ); }); - it('throws when a metadata key conflicts with an existing metric name', () => { + 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.addMetadata('request_count', 'not-a-number') - ).toThrowError( - 'Metadata key "request_count" conflicts with an existing metric name' + 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)' ); }); }); From 973d3228c45d3ac1b621784ac1ef04415f5a275c Mon Sep 17 00:00:00 2001 From: Zelys Date: Thu, 21 May 2026 19:10:09 -0500 Subject: [PATCH 3/6] fix(metrics): warn on metadata/dimension key overwrites; extract collision check - Pull collision detection out of serializeMetrics() into a private #checkKeyCollisions() method, addressing the cyclomatic complexity warning flagged by SonarCloud - Extend the check to warn (via the logger) when a metadata key would silently overwrite a dimension, default dimension, or dimension set value in the serialized EMF output; metric collisions still throw - Add three regression tests in metadata.test.ts covering the new metadata-vs-dimension-source warning path Co-Authored-By: Claude Sonnet 4.6 --- packages/metrics/src/Metrics.ts | 68 ++++++++++++++------ packages/metrics/tests/unit/metadata.test.ts | 50 ++++++++++++++ 2 files changed, 97 insertions(+), 21 deletions(-) diff --git a/packages/metrics/src/Metrics.ts b/packages/metrics/src/Metrics.ts index fc8dafe00e..17c73ddac5 100644 --- a/packages/metrics/src/Metrics.ts +++ b/packages/metrics/src/Metrics.ts @@ -745,27 +745,13 @@ class Metrics extends Utility implements MetricsInterface { dimensionNames.push(Object.keys(defaultDimensions)); } - type StringSource = - | 'dimension' - | 'default dimension' - | 'dimension set' - | 'metadata'; - const stringKeys = new Map(); - for (const k of Object.keys(defaultDimensions)) - stringKeys.set(k, 'default dimension'); - for (const k of Object.keys(dimensions)) stringKeys.set(k, 'dimension'); - for (const set of dimensionSets) - for (const k of Object.keys(set)) stringKeys.set(k, 'dimension set'); - for (const k of Object.keys(this.#metadataStore.getAll())) - stringKeys.set(k, 'metadata'); - for (const name of Object.keys(metricValues)) { - const source = stringKeys.get(name); - if (source !== undefined) { - throw new Error( - `EMF key collision on "${name}": registered as both a metric (number) and a ${source} (string)` - ); - } - } + this.#checkKeyCollisions( + defaultDimensions, + dimensions, + dimensionSets, + metricValues, + this.#metadataStore.getAll() + ); return { _aws: { @@ -792,6 +778,46 @@ class Metrics extends Utility implements MetricsInterface { }; } + #checkKeyCollisions( + defaultDimensions: Dimensions, + dimensions: Dimensions, + dimensionSets: Dimensions[], + metricValues: Record, + metadata: Record + ): void { + type Source = + | 'default dimension' + | 'dimension' + | 'dimension set' + | 'metadata'; + const seenKeys = new Map(); + + for (const k of Object.keys(defaultDimensions)) + seenKeys.set(k, 'default dimension'); + for (const k of Object.keys(dimensions)) seenKeys.set(k, 'dimension'); + for (const set of dimensionSets) + for (const k of Object.keys(set)) seenKeys.set(k, 'dimension set'); + + for (const k of Object.keys(metadata)) { + const src = seenKeys.get(k); + if (src !== undefined) { + this.#logger.warn( + `EMF key "${k}" is defined as both a ${src} and metadata; the metadata value will take precedence in the serialized output` + ); + } + seenKeys.set(k, 'metadata'); + } + + 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. * diff --git a/packages/metrics/tests/unit/metadata.test.ts b/packages/metrics/tests/unit/metadata.test.ts index 7d8d185667..9971753bee 100644 --- a/packages/metrics/tests/unit/metadata.test.ts +++ b/packages/metrics/tests/unit/metadata.test.ts @@ -110,4 +110,54 @@ describe('Working with metadata', () => { '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', () => { + // Prepare + const metrics = new Metrics({ namespace: 'test' }); + metrics.addDimension('environment', 'prod'); + metrics.addMetadata('environment', 'metadata-value'); + metrics.addMetric('test', MetricUnit.Count, 1); + + // Act + metrics.serializeMetrics(); + + // Assess + expect(console.warn).toHaveBeenCalledWith( + 'EMF key "environment" is defined as both a dimension and metadata; the metadata value will take precedence in the serialized output' + ); + }); + + it('warns on serialize when a metadata key matches a default dimension key', () => { + // Prepare + const metrics = new Metrics({ + namespace: 'test', + defaultDimensions: { environment: 'prod' }, + }); + metrics.addMetadata('environment', 'metadata-value'); + 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 metadata; the metadata value will take precedence in the serialized output' + ); + }); + + it('warns on serialize when a metadata key matches a dimension set key', () => { + // Prepare + const metrics = new Metrics({ namespace: 'test' }); + metrics.addDimensions({ environment: 'prod' }); + metrics.addMetadata('environment', 'metadata-value'); + metrics.addMetric('test', MetricUnit.Count, 1); + + // Act + metrics.serializeMetrics(); + + // Assess + expect(console.warn).toHaveBeenCalledWith( + 'EMF key "environment" is defined as both a dimension set and metadata; the metadata value will take precedence in the serialized output' + ); + }); }); From b2a61faf7c0abad6489f51969060ad26edc4e2e7 Mon Sep 17 00:00:00 2001 From: Zelys Date: Fri, 22 May 2026 03:02:15 -0500 Subject: [PATCH 4/6] fix(metrics): warn on all key overwrites; sync dimension tests with upstream Extends #checkKeyCollisions to warn whenever any two string sources share a key (dimension overwriting default dimension, dimension set overwriting dimension or default dimension), not just when metadata overlaps with a dimension. Folds the repeated get+check+set pattern into a single setKey helper. Also reconciles dimensions.test.ts with upstream #5229 (MAX_DIMENSION_COUNT loop bounds, toThrow vs toThrowError, three new upstream test cases) and adds regression tests for the three new cross-source overwrite warn paths. Co-Authored-By: Claude Sonnet 4.6 --- packages/metrics/src/Metrics.ts | 25 +-- .../metrics/tests/unit/dimensions.test.ts | 165 ++++++++++++++++-- 2 files changed, 162 insertions(+), 28 deletions(-) diff --git a/packages/metrics/src/Metrics.ts b/packages/metrics/src/Metrics.ts index 17c73ddac5..11107e3535 100644 --- a/packages/metrics/src/Metrics.ts +++ b/packages/metrics/src/Metrics.ts @@ -792,21 +792,22 @@ class Metrics extends Utility implements MetricsInterface { | 'metadata'; const seenKeys = new Map(); - for (const k of Object.keys(defaultDimensions)) - seenKeys.set(k, 'default dimension'); - for (const k of Object.keys(dimensions)) seenKeys.set(k, 'dimension'); - for (const set of dimensionSets) - for (const k of Object.keys(set)) seenKeys.set(k, 'dimension set'); - - for (const k of Object.keys(metadata)) { - const src = seenKeys.get(k); - if (src !== undefined) { + const setKey = (k: string, source: Source) => { + const existing = seenKeys.get(k); + if (existing !== undefined) { this.#logger.warn( - `EMF key "${k}" is defined as both a ${src} and metadata; the metadata value will take precedence in the serialized output` + `EMF key "${k}" is defined as both a ${existing} and ${source}; the ${source} value will take precedence in the serialized output` ); } - seenKeys.set(k, 'metadata'); - } + seenKeys.set(k, source); + }; + + for (const k of Object.keys(defaultDimensions)) + seenKeys.set(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 k of Object.keys(metadata)) setKey(k, 'metadata'); for (const name of Object.keys(metricValues)) { const src = seenKeys.get(name); diff --git a/packages/metrics/tests/unit/dimensions.test.ts b/packages/metrics/tests/unit/dimensions.test.ts index d5dffb4ac0..64635fa72c 100644 --- a/packages/metrics/tests/unit/dimensions.test.ts +++ b/packages/metrics/tests/unit/dimensions.test.ts @@ -338,7 +338,7 @@ describe('Working with dimensions', () => { } // Assess - expect(() => metrics.addDimension('extra', 'test')).toThrowError( + expect(() => metrics.addDimension('extra', 'test')).toThrow( `The number of metric dimensions must be lower than ${MAX_DIMENSION_COUNT}` ); }); @@ -351,12 +351,12 @@ describe('Working with dimensions', () => { // Act // We start with 1 dimension because service name is already added - for (let i = 1; i < MAX_DIMENSION_COUNT - 1; i++) { + for (let i = 1; i < MAX_DIMENSION_COUNT; i++) { metrics.setDefaultDimensions({ [`dimension-${i}`]: 'test' }); } // Assess - expect(() => metrics.setDefaultDimensions({ extra: 'test' })).toThrowError( + expect(() => metrics.setDefaultDimensions({ extra: 'test' })).toThrow( 'The number of metric dimensions must be lower than 29' ); }); @@ -368,7 +368,7 @@ describe('Working with dimensions', () => { }); // Act - for (let i = 1; i < MAX_DIMENSION_COUNT - 1; i++) { + for (let i = 1; i < MAX_DIMENSION_COUNT; i++) { metrics.addDimension(`regular-${i}`, 'test'); } @@ -380,6 +380,27 @@ describe('Working with dimensions', () => { ); }); + it('throws when setDefaultDimensions would exceed the limit with existing dimension sets', () => { + // Prepare + const metrics = new Metrics({ + singleMetric: true, + }); + + // Act + const newDimensionSet: Record = {}; + for (let i = 0; i < 28; i++) { + newDimensionSet[`dimension-extra-${i}`] = 'test'; + } + metrics.addDimensions(newDimensionSet); + + // Assess + expect(() => + metrics.setDefaultDimensions({ 'new-default': 'test' }) + ).toThrow( + `The number of metric dimensions must be lower than ${MAX_DIMENSION_COUNT}` + ); + }); + it('allows overriding existing default dimension keys without triggering the limit', () => { // Prepare const metrics = new Metrics({ @@ -387,7 +408,7 @@ describe('Working with dimensions', () => { }); // Act - for (let i = 1; i < MAX_DIMENSION_COUNT - 1; i++) { + for (let i = 1; i < MAX_DIMENSION_COUNT; i++) { metrics.setDefaultDimensions({ [`dimension-${i}`]: 'test' }); } @@ -397,34 +418,79 @@ describe('Working with dimensions', () => { ).not.toThrow(); }); - it('throws when adding dimension sets would exceed the limit', () => { + it('allows overriding existing regular dimensions via addDimension without triggering the limit', () => { // Prepare const metrics = new Metrics({ singleMetric: true, - defaultDimensions: { - environment: 'test', - }, }); // Act - // We start with 2 dimensions because the default dimension & service name are already added - for (let i = 2; i < MAX_DIMENSION_COUNT; i++) { + for (let i = 1; i < MAX_DIMENSION_COUNT; i++) { metrics.addDimension(`dimension-${i}`, 'test'); } // Assess - // Adding a dimension set with 3 dimensions would exceed the limit + expect(() => metrics.addDimension('dimension-1', 'updated')).not.toThrow(); + }); + + it('allows addDimensions to override existing default dimension keys without triggering the limit', () => { + // Prepare + const metrics = new Metrics({ + singleMetric: true, + }); + + // Act + for (let i = 1; i < MAX_DIMENSION_COUNT; i++) { + metrics.setDefaultDimensions({ [`dimension-${i}`]: 'test' }); + } + + // Assess expect(() => metrics.addDimensions({ - 'dimension-extra-1': 'test', - 'dimension-extra-2': 'test', - 'dimension-extra-3': 'test', + 'dimension-1': 'updated', + 'dimension-2': 'updated', }) - ).toThrowError( + ).not.toThrow(); + }); + + it('throws when adding dimension sets would exceed the limit', () => { + // Prepare + const metrics = new Metrics({ + singleMetric: true, + defaultDimensions: { + environment: 'test', + }, + }); + + // Act + const newDimensionSet: Record = {}; + for (let i = 0; i < 28; i++) { + newDimensionSet[`dimension-extra-${i}`] = 'test'; + } + + // Assess + // Adding a dimension set that results in > 29 dimensions should exceed the limit + expect(() => metrics.addDimensions(newDimensionSet)).toThrow( `The number of metric dimensions must be lower than ${MAX_DIMENSION_COUNT}` ); }); + it('allows setDefaultDimensions to evaluate maxProjectedSize correctly when dimensionSets are smaller', () => { + // Prepare + const metrics = new Metrics({ singleMetric: true }); + + // Act + metrics.addDimension('dim1', 'val1'); + metrics.addDimension('dim2', 'val2'); + metrics.addDimensions({ setDim1: 'val3' }); + + // Assess + // This triggers setDefaultDimensions logic where setSize (3) < maxProjectedSize (4), covering the false branch. + expect(() => + metrics.setDefaultDimensions({ newDefault: 'val4' }) + ).not.toThrow(); + }); + it('handles dimension overrides across multiple dimension sets', () => { // Prepare const metrics = new Metrics({ @@ -647,6 +713,21 @@ describe('Working with dimensions', () => { expect.not.objectContaining({ [name]: value }) ); }); + it('allows adding multiple dimension sets as long as no single set exceeds MAX_DIMENSION_COUNT', () => { + // Prepare + const metrics = new Metrics({ namespace: 'test' }); + + // Act + // Add 30 dimension sets, each with 1 dimension + for (let i = 0; i < 30; i++) { + metrics.addDimensions({ [`dim-${i}`]: 'value' }); + } + + metrics.addMetric('test', MetricUnit.Count, 1); + + // Assess + expect(() => metrics.publishStoredMetrics()).not.toThrow(); + }); it('throws when a metric name conflicts with an existing dimension key', () => { // Prepare @@ -718,4 +799,56 @@ describe('Working with dimensions', () => { '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' + ); + }); }); From 3e92ae89ee2b2fc10ffc4bde3dadae9e5891ea61 Mon Sep 17 00:00:00 2001 From: Zelys Date: Sat, 23 May 2026 09:57:55 -0500 Subject: [PATCH 5/6] fix(metrics): move EmfKeySource to types module; make dimensions win over metadata Three changes addressing PR review feedback: 1. EmfKeySource extracted from the inline type alias in #checkKeyCollisions to packages/metrics/src/types/Metrics.ts. 2. Dimensions now take precedence over metadata in the serialized EMF. The prior spread order put metadata last, so a metadata value silently overrode any dimension on the same key, contradicting the warning that said the dimension would win. The fix flips the spread (metadata first, then dimensions) and reorders the seed in #checkKeyCollisions so the warning message matches what actually ends up in the payload. 3. Collision tests moved from dimensions.test.ts to a new keyCollisions.test.ts. dimensions.test.ts is now byte-identical to upstream/main, which clears the merge conflict GitHub flagged after #5229 merged. Also drops the now-unused DimensionsStore.getDimensionCount(), which #5229 stopped calling. Test count: 156 unit tests pass. --- packages/metrics/src/DimensionsStore.ts | 14 -- packages/metrics/src/Metrics.ts | 82 ++++++++--- packages/metrics/src/types/Metrics.ts | 11 ++ packages/metrics/src/types/index.ts | 1 + .../metrics/tests/unit/dimensions.test.ts | 123 ---------------- .../metrics/tests/unit/keyCollisions.test.ts | 138 ++++++++++++++++++ packages/metrics/tests/unit/metadata.test.ts | 38 +++-- 7 files changed, 237 insertions(+), 170 deletions(-) create mode 100644 packages/metrics/tests/unit/keyCollisions.test.ts diff --git a/packages/metrics/src/DimensionsStore.ts b/packages/metrics/src/DimensionsStore.ts index 4a665f1b5c..888d3b5d6a 100644 --- a/packages/metrics/src/DimensionsStore.ts +++ b/packages/metrics/src/DimensionsStore.ts @@ -92,20 +92,6 @@ class DimensionsStore { this.#defaultDimensions = {}; } - public getDimensionCount(): number { - const dimensions = this.#getDimensions(); - const dimensionSets = this.#getDimensionSets(); - const dimensionSetsCount = dimensionSets.reduce( - (total, dimensionSet) => total + Object.keys(dimensionSet).length, - 0 - ); - return ( - Object.keys(dimensions).length + - Object.keys(this.#defaultDimensions).length + - dimensionSetsCount - ); - } - public setDefaultDimensions(dimensions: Dimensions): void { this.#defaultDimensions = { ...dimensions }; } diff --git a/packages/metrics/src/Metrics.ts b/packages/metrics/src/Metrics.ts index 11107e3535..101f03474c 100644 --- a/packages/metrics/src/Metrics.ts +++ b/packages/metrics/src/Metrics.ts @@ -36,6 +36,7 @@ import { MetricsStore } from './MetricsStore.js'; import type { ConfigServiceInterface, Dimensions, + EmfKeySource, EmfOutput, ExtraOptions, MetricDefinition, @@ -247,13 +248,22 @@ class Metrics extends Utility implements MetricsInterface { ); return this; } - if (MAX_DIMENSION_COUNT <= this.#dimensionsStore.getDimensionCount()) { + const dimensions = this.#dimensionsStore.getDimensions(); + const defaultDimensions = this.#dimensionsStore.getDefaultDimensions(); + // addDimension only mutates the regular dimensions array, so we don't need to project against dimensionSets + // (each set is an independent published array). + const projectedSize = new Set([ + ...Object.keys(defaultDimensions), + ...Object.keys(dimensions), + name, + ]).size; + // MAX_DIMENSION_COUNT is 29 (EMF 30 dimension cap - 1 reserved for service). + // Thus exactly 29 custom dimensions are allowed, and we throw only if strictly greater. + if (projectedSize > MAX_DIMENSION_COUNT) { throw new RangeError( `The number of metric dimensions must be lower than ${MAX_DIMENSION_COUNT}` ); } - const dimensions = this.#dimensionsStore.getDimensions(); - const defaultDimensions = this.#dimensionsStore.getDefaultDimensions(); if ( Object.hasOwn(dimensions, name) || Object.hasOwn(defaultDimensions, name) @@ -280,9 +290,13 @@ class Metrics extends Utility implements MetricsInterface { */ public addDimensions(dimensions: Dimensions): this { const newDimensions = this.#sanitizeDimensions(dimensions); - const currentCount = this.#dimensionsStore.getDimensionCount(); - const newSetCount = Object.keys(newDimensions).length; - if (currentCount + newSetCount >= MAX_DIMENSION_COUNT) { + const defaultDimensions = this.#dimensionsStore.getDefaultDimensions(); + // addDimensions creates a new independent dimensionSet array, so we only project against defaultDimensions. + const projectedSize = new Set([ + ...Object.keys(defaultDimensions), + ...Object.keys(newDimensions), + ]).size; + if (projectedSize > MAX_DIMENSION_COUNT) { throw new RangeError( `The number of metric dimensions must be lower than ${MAX_DIMENSION_COUNT}` ); @@ -764,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 @@ -774,7 +792,6 @@ class Metrics extends Utility implements MetricsInterface { return acc; }, {}), ...metricValues, - ...this.#metadataStore.getAll(), }; } @@ -785,14 +802,9 @@ class Metrics extends Utility implements MetricsInterface { metricValues: Record, metadata: Record ): void { - type Source = - | 'default dimension' - | 'dimension' - | 'dimension set' - | 'metadata'; - const seenKeys = new Map(); - - const setKey = (k: string, source: Source) => { + const seenKeys = new Map(); + + const setKey = (k: string, source: EmfKeySource) => { const existing = seenKeys.get(k); if (existing !== undefined) { this.#logger.warn( @@ -802,12 +814,15 @@ class Metrics extends Utility implements MetricsInterface { 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)) - seenKeys.set(k, 'default dimension'); + 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 k of Object.keys(metadata)) setKey(k, 'metadata'); for (const name of Object.keys(metricValues)) { const src = seenKeys.get(name); @@ -847,13 +862,32 @@ class Metrics extends Utility implements MetricsInterface { const newDimensions = this.#sanitizeDimensions(dimensions); const currentDefaultDimensions = this.#dimensionsStore.getDefaultDimensions(); - const newKeysCount = Object.keys(newDimensions).filter( - (key) => !Object.hasOwn(currentDefaultDimensions, key) - ).length; - if ( - this.#dimensionsStore.getDimensionCount() + newKeysCount >= - MAX_DIMENSION_COUNT - ) { + const currentDimensions = this.#dimensionsStore.getDimensions(); + const dimensionSets = this.#dimensionsStore.getDimensionSets(); + + const combinedDefaultKeys = [ + ...Object.keys(currentDefaultDimensions), + ...Object.keys(newDimensions), + ]; + const currentDimensionsKeys = Object.keys(currentDimensions); + // The main array is only emitted if currentDimensions has keys. + // When empty, maxProjectedSize safely defaults to just the combinedDefaultKeys length to guard against phantom arrays. + let maxProjectedSize = + currentDimensionsKeys.length > 0 + ? new Set([...combinedDefaultKeys, ...currentDimensionsKeys]).size + : new Set(combinedDefaultKeys).size; + + for (const dimensionSet of dimensionSets) { + const setSize = new Set([ + ...combinedDefaultKeys, + ...Object.keys(dimensionSet), + ]).size; + if (setSize > maxProjectedSize) { + maxProjectedSize = setSize; + } + } + + if (maxProjectedSize > MAX_DIMENSION_COUNT) { throw new RangeError( `The number of metric dimensions must be lower than ${MAX_DIMENSION_COUNT}` ); diff --git a/packages/metrics/src/types/Metrics.ts b/packages/metrics/src/types/Metrics.ts index 8430c65a61..33748283c8 100644 --- a/packages/metrics/src/types/Metrics.ts +++ b/packages/metrics/src/types/Metrics.ts @@ -13,6 +13,16 @@ import type { ConfigServiceInterface } from './ConfigServiceInterface.js'; */ type Dimensions = Record; +/** + * 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, diff --git a/packages/metrics/src/types/index.ts b/packages/metrics/src/types/index.ts index ced7349ffd..299f9dfd73 100644 --- a/packages/metrics/src/types/index.ts +++ b/packages/metrics/src/types/index.ts @@ -1,6 +1,7 @@ export type { ConfigServiceInterface } from './ConfigServiceInterface.js'; export type { Dimensions, + EmfKeySource, EmfOutput, ExtraOptions, MetricDefinition, diff --git a/packages/metrics/tests/unit/dimensions.test.ts b/packages/metrics/tests/unit/dimensions.test.ts index 64635fa72c..ace544da0f 100644 --- a/packages/metrics/tests/unit/dimensions.test.ts +++ b/packages/metrics/tests/unit/dimensions.test.ts @@ -728,127 +728,4 @@ 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' - ); - }); }); diff --git a/packages/metrics/tests/unit/keyCollisions.test.ts b/packages/metrics/tests/unit/keyCollisions.test.ts new file mode 100644 index 0000000000..596d0265f8 --- /dev/null +++ b/packages/metrics/tests/unit/keyCollisions.test.ts @@ -0,0 +1,138 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { Metrics, MetricUnit } from '../../src/index.js'; + +describe('EMF key collision detection', () => { + const ENVIRONMENT_VARIABLES = process.env; + + beforeEach(() => { + process.env = { + ...ENVIRONMENT_VARIABLES, + POWERTOOLS_DEV: 'true', + POWERTOOLS_METRICS_DISABLED: 'false', + }; + vi.clearAllMocks(); + }); + + 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' + ); + }); +}); diff --git a/packages/metrics/tests/unit/metadata.test.ts b/packages/metrics/tests/unit/metadata.test.ts index 9971753bee..44fbd58b40 100644 --- a/packages/metrics/tests/unit/metadata.test.ts +++ b/packages/metrics/tests/unit/metadata.test.ts @@ -111,7 +111,7 @@ describe('Working with metadata', () => { ); }); - it('warns on serialize when a metadata key matches a dimension key', () => { + 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'); @@ -119,15 +119,16 @@ describe('Working with metadata', () => { metrics.addMetric('test', MetricUnit.Count, 1); // Act - metrics.serializeMetrics(); + const serialized = metrics.serializeMetrics(); // Assess expect(console.warn).toHaveBeenCalledWith( - 'EMF key "environment" is defined as both a dimension and metadata; the metadata value will take precedence in the serialized output' + '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', () => { + 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', @@ -137,15 +138,16 @@ describe('Working with metadata', () => { metrics.addMetric('test', MetricUnit.Count, 1); // Act - metrics.serializeMetrics(); + const serialized = metrics.serializeMetrics(); // Assess expect(console.warn).toHaveBeenCalledWith( - 'EMF key "environment" is defined as both a default dimension and metadata; the metadata value will take precedence in the serialized output' + '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', () => { + 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' }); @@ -153,11 +155,29 @@ describe('Working with metadata', () => { metrics.addMetric('test', MetricUnit.Count, 1); // Act - metrics.serializeMetrics(); + const serialized = metrics.serializeMetrics(); // Assess expect(console.warn).toHaveBeenCalledWith( - 'EMF key "environment" is defined as both a dimension set and metadata; the metadata value will take precedence in the serialized output' + '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' }); }); }); From 9ecbc7b2752ab4efbe62cfdfbe6f3e8faac6dcb1 Mon Sep 17 00:00:00 2001 From: Zelys Date: Sun, 24 May 2026 07:30:14 -0500 Subject: [PATCH 6/6] test(metrics): move collision tests into dimensions.test.ts Deleted keyCollisions.test.ts and moved all 8 tests into dimensions.test.ts, where they belong alongside the existing dimension behavior tests. metadata.test.ts owns the metadata-side collision tests; dimensions.test.ts now owns the dimension-side. Co-authored-by: Claude Signed-off-by: Zelys-DFKH --- .../metrics/tests/unit/dimensions.test.ts | 123 ++++++++++++++++ .../metrics/tests/unit/keyCollisions.test.ts | 138 ------------------ 2 files changed, 123 insertions(+), 138 deletions(-) delete mode 100644 packages/metrics/tests/unit/keyCollisions.test.ts diff --git a/packages/metrics/tests/unit/dimensions.test.ts b/packages/metrics/tests/unit/dimensions.test.ts index ace544da0f..64635fa72c 100644 --- a/packages/metrics/tests/unit/dimensions.test.ts +++ b/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' + ); + }); }); diff --git a/packages/metrics/tests/unit/keyCollisions.test.ts b/packages/metrics/tests/unit/keyCollisions.test.ts deleted file mode 100644 index 596d0265f8..0000000000 --- a/packages/metrics/tests/unit/keyCollisions.test.ts +++ /dev/null @@ -1,138 +0,0 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest'; -import { Metrics, MetricUnit } from '../../src/index.js'; - -describe('EMF key collision detection', () => { - const ENVIRONMENT_VARIABLES = process.env; - - beforeEach(() => { - process.env = { - ...ENVIRONMENT_VARIABLES, - POWERTOOLS_DEV: 'true', - POWERTOOLS_METRICS_DISABLED: 'false', - }; - vi.clearAllMocks(); - }); - - 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' - ); - }); -});