From a0fdbee0f193ac82ce091957d3aab4982d5a488f Mon Sep 17 00:00:00 2001 From: Uttam Raj Date: Tue, 29 Jul 2025 14:42:19 +0530 Subject: [PATCH 1/4] fix(metrics): emit warning when default dimensions are overwritten (#4222) Co-authored-by: Swopnil Dangol --- packages/metrics/src/Metrics.ts | 40 ++++++- .../tests/unit/customTimestamp.test.ts | 8 +- .../metrics/tests/unit/dimensions.test.ts | 100 ++++++++++++++++++ .../tests/unit/initializeMetrics.test.ts | 4 +- 4 files changed, 144 insertions(+), 8 deletions(-) diff --git a/packages/metrics/src/Metrics.ts b/packages/metrics/src/Metrics.ts index a309186f0..f34c6a159 100644 --- a/packages/metrics/src/Metrics.ts +++ b/packages/metrics/src/Metrics.ts @@ -1,7 +1,9 @@ import { Console } from 'node:console'; import { isIntegerNumber, + isNullOrUndefined, isNumber, + isRecord, isString, isStringUndefinedNullEmpty, Utility, @@ -240,8 +242,10 @@ class Metrics extends Utility implements MetricsInterface { super(); this.dimensions = {}; - this.setOptions(options); + this.setEnvConfig(); + this.setConsole(); this.#logger = options.logger || this.console; + this.setOptions(options); } /** @@ -824,13 +828,41 @@ class Metrics extends Utility implements MetricsInterface { * @param dimensions - The dimensions to be added to the default dimensions object */ public setDefaultDimensions(dimensions: Dimensions | undefined): void { + if (isNullOrUndefined(dimensions) || !isRecord(dimensions)) { + return; + } + + const cleanedDimensions: Dimensions = {}; + + for (const [key, value] of Object.entries(dimensions)) { + if ( + isStringUndefinedNullEmpty(key) || + isStringUndefinedNullEmpty(value) + ) { + this.#logger.warn( + `The dimension ${key} doesn't meet the requirements and won't be added. Ensure the dimension name and value are non empty strings` + ); + continue; + } + + if (Object.hasOwn(this.defaultDimensions, key)) { + this.#logger.warn( + `Dimension "${key}" has already been added. The previous value will be overwritten.` + ); + } + + cleanedDimensions[key] = value; + } + const targetDimensions = { ...this.defaultDimensions, - ...dimensions, + ...cleanedDimensions, }; - if (MAX_DIMENSION_COUNT <= Object.keys(targetDimensions).length) { + + if (Object.keys(targetDimensions).length >= MAX_DIMENSION_COUNT) { throw new Error('Max dimension count hit'); } + this.defaultDimensions = targetDimensions; } @@ -1058,8 +1090,6 @@ class Metrics extends Utility implements MetricsInterface { functionName, } = options; - this.setEnvConfig(); - this.setConsole(); this.setCustomConfigService(customConfigService); this.setDisabled(); this.setNamespace(namespace); diff --git a/packages/metrics/tests/unit/customTimestamp.test.ts b/packages/metrics/tests/unit/customTimestamp.test.ts index 61e3ffa22..01d6589eb 100644 --- a/packages/metrics/tests/unit/customTimestamp.test.ts +++ b/packages/metrics/tests/unit/customTimestamp.test.ts @@ -48,7 +48,9 @@ describe('Setting custom timestamp', () => { it('logs a warning when the provided timestamp is too far in the past', () => { // Prepare - const metrics = new Metrics({ singleMetric: true }); + const metrics = new Metrics({ + singleMetric: true, + }); // Act metrics.setTimestamp(Date.now() - EMF_MAX_TIMESTAMP_PAST_AGE - 1000); @@ -63,7 +65,9 @@ describe('Setting custom timestamp', () => { it('logs a warning when the provided timestamp is too far in the future', () => { // Prepare - const metrics = new Metrics({ singleMetric: true }); + const metrics = new Metrics({ + singleMetric: true, + }); // Act metrics.setTimestamp(Date.now() + EMF_MAX_TIMESTAMP_FUTURE_AGE + 1000); diff --git a/packages/metrics/tests/unit/dimensions.test.ts b/packages/metrics/tests/unit/dimensions.test.ts index 1a874ec02..d9d39f471 100644 --- a/packages/metrics/tests/unit/dimensions.test.ts +++ b/packages/metrics/tests/unit/dimensions.test.ts @@ -552,4 +552,104 @@ describe('Working with dimensions', () => { }) ); }); + + it('warns when setDefaultDimensions overwrites existing dimensions', () => { + // Prepare + const metrics = new Metrics({ + namespace: DEFAULT_NAMESPACE, + defaultDimensions: { environment: 'prod' }, + }); + + // Act + metrics.setDefaultDimensions({ region: 'us-east-1' }); + metrics.setDefaultDimensions({ + environment: 'staging', // overwrites default dimension + }); + + // Assess + expect(console.warn).toHaveBeenCalledOnce(); + expect(console.warn).toHaveBeenCalledWith( + 'Dimension "environment" has already been added. The previous value will be overwritten.' + ); + }); + + it('returns immediately if dimensions is undefined', () => { + // Prepare + const metrics = new Metrics({ + singleMetric: true, + namespace: DEFAULT_NAMESPACE, + }); + + // Act + metrics.addMetric('myMetric', MetricUnit.Count, 1); + + // Assert + expect(console.warn).not.toHaveBeenCalled(); + + expect(console.log).toHaveEmittedEMFWith( + expect.objectContaining({ + service: 'hello-world', + }) + ); + }); + + it.each([ + { value: undefined, name: 'valid-name' }, + { value: null, name: 'valid-name' }, + { value: '', name: 'valid-name' }, + { value: 'valid-value', name: '' }, + ])( + 'skips invalid default dimension values in setDefaultDimensions ($name)', + ({ value, name }) => { + // Arrange + const metrics = new Metrics({ + singleMetric: true, + namespace: DEFAULT_NAMESPACE, + }); + + // Act + metrics.setDefaultDimensions({ + validDimension: 'valid', + [name as string]: value as string, + }); + + metrics.addMetric('test', MetricUnit.Count, 1); + metrics.publishStoredMetrics(); + + // Assert + expect(console.warn).toHaveBeenCalledWith( + `The dimension ${name} doesn't meet the requirements and won't be added. Ensure the dimension name and value are non empty strings` + ); + + expect(console.log).toHaveEmittedEMFWith( + expect.objectContaining({ validDimension: 'valid' }) + ); + + expect(console.log).toHaveEmittedEMFWith( + expect.not.objectContaining({ [name]: value }) + ); + } + ); + it('returns immediately without logging if dimensions is not a plain object', () => { + // Prepare + const metrics = new Metrics({ + singleMetric: true, + namespace: DEFAULT_NAMESPACE, + }); + + // Act + // @ts-expect-error – simulate runtime misuse + metrics.setDefaultDimensions('not-an-object'); + + // Assert + expect(console.warn).not.toHaveBeenCalled(); + + metrics.addMetric('someMetric', MetricUnit.Count, 1); + + expect(console.log).toHaveEmittedEMFWith( + expect.objectContaining({ + service: 'hello-world', + }) + ); + }); }); diff --git a/packages/metrics/tests/unit/initializeMetrics.test.ts b/packages/metrics/tests/unit/initializeMetrics.test.ts index b1193e99d..7bb1ab5c7 100644 --- a/packages/metrics/tests/unit/initializeMetrics.test.ts +++ b/packages/metrics/tests/unit/initializeMetrics.test.ts @@ -65,7 +65,9 @@ describe('Initialize Metrics', () => { it('uses the default namespace when none is provided', () => { // Prepare - const metrics = new Metrics({ singleMetric: true }); + const metrics = new Metrics({ + singleMetric: true, + }); // Act metrics.addMetric('test', MetricUnit.Count, 1); From 75989d111645ee65fdbe26cb908b85fde5d577db Mon Sep 17 00:00:00 2001 From: uttam282005 Date: Wed, 30 Jul 2025 17:47:53 +0530 Subject: [PATCH 2/4] fix(metrics): warn on default dimension overwrite and avoid redundant service warning --- packages/metrics/src/Metrics.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/metrics/src/Metrics.ts b/packages/metrics/src/Metrics.ts index f34c6a159..43e597a81 100644 --- a/packages/metrics/src/Metrics.ts +++ b/packages/metrics/src/Metrics.ts @@ -437,11 +437,6 @@ class Metrics extends Utility implements MetricsInterface { if (!this.getColdStart()) return; const singleMetric = this.singleMetric(); - if (this.defaultDimensions.service) { - singleMetric.setDefaultDimensions({ - service: this.defaultDimensions.service, - }); - } const value = this.functionName?.trim() ?? functionName?.trim(); if (value && value.length > 0) { singleMetric.addDimension('function_name', value); @@ -846,9 +841,14 @@ class Metrics extends Utility implements MetricsInterface { } if (Object.hasOwn(this.defaultDimensions, key)) { - this.#logger.warn( - `Dimension "${key}" has already been added. The previous value will be overwritten.` - ); + const currentValue = this.defaultDimensions[key]; + const suppressOverwriteWarning = + key === 'service' && currentValue === this.defaultServiceName; + if (!suppressOverwriteWarning) { + this.#logger.warn( + `Dimension "${key}" has already been added. The previous value will be overwritten.` + ); + } } cleanedDimensions[key] = value; From 277b42263d4f6a4d62fcbbd071278b02de63e19f Mon Sep 17 00:00:00 2001 From: Uttam Raj Date: Wed, 30 Jul 2025 20:27:52 +0530 Subject: [PATCH 3/4] Replaces Assert with Assess Co-authored-by: Andrea Amorosi --- packages/metrics/tests/unit/dimensions.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/metrics/tests/unit/dimensions.test.ts b/packages/metrics/tests/unit/dimensions.test.ts index d9d39f471..82c66e159 100644 --- a/packages/metrics/tests/unit/dimensions.test.ts +++ b/packages/metrics/tests/unit/dimensions.test.ts @@ -583,7 +583,7 @@ describe('Working with dimensions', () => { // Act metrics.addMetric('myMetric', MetricUnit.Count, 1); - // Assert + // Assess expect(console.warn).not.toHaveBeenCalled(); expect(console.log).toHaveEmittedEMFWith( From 256d9e9fb83c6b4970acdb2ea5d7c78d1b8d3e2c Mon Sep 17 00:00:00 2001 From: uttam282005 Date: Wed, 30 Jul 2025 20:30:32 +0530 Subject: [PATCH 4/4] replaced assert with assess --- packages/metrics/tests/unit/dimensions.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/metrics/tests/unit/dimensions.test.ts b/packages/metrics/tests/unit/dimensions.test.ts index 82c66e159..731ca544e 100644 --- a/packages/metrics/tests/unit/dimensions.test.ts +++ b/packages/metrics/tests/unit/dimensions.test.ts @@ -581,7 +581,7 @@ describe('Working with dimensions', () => { }); // Act - metrics.addMetric('myMetric', MetricUnit.Count, 1); + metrics.addMetric('test', MetricUnit.Count, 1); // Assess expect(console.warn).not.toHaveBeenCalled(); @@ -616,7 +616,7 @@ describe('Working with dimensions', () => { metrics.addMetric('test', MetricUnit.Count, 1); metrics.publishStoredMetrics(); - // Assert + // Assess expect(console.warn).toHaveBeenCalledWith( `The dimension ${name} doesn't meet the requirements and won't be added. Ensure the dimension name and value are non empty strings` ); @@ -641,7 +641,7 @@ describe('Working with dimensions', () => { // @ts-expect-error – simulate runtime misuse metrics.setDefaultDimensions('not-an-object'); - // Assert + // Assess expect(console.warn).not.toHaveBeenCalled(); metrics.addMetric('someMetric', MetricUnit.Count, 1);