From 5ae7b4190e0de99e4a58364b4a86eebf5edb0bc5 Mon Sep 17 00:00:00 2001 From: arnabrahman Date: Thu, 14 Nov 2024 21:05:03 +0600 Subject: [PATCH 1/4] fix: invalid dimension values will not be added --- packages/metrics/src/Metrics.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/metrics/src/Metrics.ts b/packages/metrics/src/Metrics.ts index 94eef05c8b..b2a58ce16e 100644 --- a/packages/metrics/src/Metrics.ts +++ b/packages/metrics/src/Metrics.ts @@ -218,6 +218,7 @@ class Metrics extends Utility implements MetricsInterface { * Add a dimension to metrics. * * A dimension is a key-value pair that is used to group metrics, and it is included in all metrics emitted after it is added. + * Invalid dimension values are skipped and a warning is logged. * * When calling the {@link Metrics.publishStoredMetrics | `publishStoredMetrics()`} method, the dimensions are cleared. This type of * dimension is useful when you want to add request-specific dimensions to your metrics. If you want to add dimensions that are @@ -227,6 +228,12 @@ class Metrics extends Utility implements MetricsInterface { * @param value - The value of the dimension */ public addDimension(name: string, value: string): void { + if (!value) { + this.#logger.warn( + `Dimension value is invalid for ${name} and will be skipped.` + ); + return; + } if (MAX_DIMENSION_COUNT <= this.getCurrentDimensionsCount()) { throw new RangeError( `The number of metric dimensions must be lower than ${MAX_DIMENSION_COUNT}` From c5b3444a192dcaceedc0ea8c119f26155cbd5036 Mon Sep 17 00:00:00 2001 From: arnabrahman Date: Thu, 14 Nov 2024 21:54:24 +0600 Subject: [PATCH 2/4] fix: validate dimension values for `addDimensions` --- packages/metrics/src/Metrics.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/metrics/src/Metrics.ts b/packages/metrics/src/Metrics.ts index b2a58ce16e..be94758429 100644 --- a/packages/metrics/src/Metrics.ts +++ b/packages/metrics/src/Metrics.ts @@ -246,6 +246,7 @@ class Metrics extends Utility implements MetricsInterface { * Add multiple dimensions to the metrics. * * This method is useful when you want to add multiple dimensions to the metrics at once. + * Invalid dimension values are skipped and a warning is logged. * * When calling the {@link Metrics.publishStoredMetrics | `publishStoredMetrics()`} method, the dimensions are cleared. This type of * dimension is useful when you want to add request-specific dimensions to your metrics. If you want to add dimensions that are @@ -256,7 +257,14 @@ class Metrics extends Utility implements MetricsInterface { public addDimensions(dimensions: Dimensions): void { const newDimensions = { ...this.dimensions }; for (const dimensionName of Object.keys(dimensions)) { - newDimensions[dimensionName] = dimensions[dimensionName]; + const value = dimensions[dimensionName]; + if (value) { + newDimensions[dimensionName] = value; + } else { + this.#logger.warn( + `Dimension value is invalid for ${dimensionName} and will be skipped.` + ); + } } if (Object.keys(newDimensions).length > MAX_DIMENSION_COUNT) { throw new RangeError( From 86638e0941df0806b4509a60462a0e8ef140ba4e Mon Sep 17 00:00:00 2001 From: arnabrahman Date: Thu, 14 Nov 2024 21:54:52 +0600 Subject: [PATCH 3/4] test: `addDimension` & `addDimensions` for invalid values --- packages/metrics/tests/unit/Metrics.test.ts | 86 +++++++++++++++++++++ 1 file changed, 86 insertions(+) diff --git a/packages/metrics/tests/unit/Metrics.test.ts b/packages/metrics/tests/unit/Metrics.test.ts index b65ebab212..dde89a87a5 100644 --- a/packages/metrics/tests/unit/Metrics.test.ts +++ b/packages/metrics/tests/unit/Metrics.test.ts @@ -431,6 +431,45 @@ describe('Class: Metrics', () => { `The number of metric dimensions must be lower than ${MAX_DIMENSION_COUNT}` ); }); + + describe('invalid values should not be added as dimensions', () => { + const testCases = [ + { value: undefined as unknown as string, description: 'undefined' }, + { value: null as unknown as string, description: 'null' }, + { value: '', description: 'empty string' }, + ]; + + for (const { value, description } of testCases) { + it(`it should not add dimension with ${description} value and log a warning`, () => { + // Prepare + const customLogger = { + warn: jest.fn(), + debug: jest.fn(), + error: jest.fn(), + info: jest.fn(), + }; + const metrics: Metrics = new Metrics({ + namespace: TEST_NAMESPACE, + logger: customLogger, + }); + const consoleWarnSpy = jest.spyOn(customLogger, 'warn'); + const testDimensionName = 'test-dimension'; + + // Act + metrics.addDimension(testDimensionName, value); + + // Assess + expect(consoleWarnSpy).toHaveBeenCalledWith( + `Dimension value is invalid for ${testDimensionName} and will be skipped.` + ); + expect(metrics).toEqual( + expect.objectContaining({ + dimensions: {}, + }) + ); + }); + } + }); }); describe('Method: addDimensions', () => { @@ -520,6 +559,53 @@ describe('Class: Metrics', () => { `Unable to add 1 dimensions: the number of metric dimensions must be lower than ${MAX_DIMENSION_COUNT}` ); }); + + describe('invalid values should not be added as dimensions', () => { + const testCases = [ + { value: undefined as unknown as string, description: 'undefined' }, + { value: null as unknown as string, description: 'null' }, + { value: '', description: 'empty string' }, + ]; + + for (const { value, description } of testCases) { + it(`it should not add dimension with ${description} value and log a warning`, () => { + // Prepare + const customLogger = { + warn: jest.fn(), + debug: jest.fn(), + error: jest.fn(), + info: jest.fn(), + }; + const metrics: Metrics = new Metrics({ + namespace: TEST_NAMESPACE, + logger: customLogger, + }); + const consoleWarnSpy = jest.spyOn(customLogger, 'warn'); + const dimensionsToBeAdded: LooseObject = { + 'test-dimension-1': 'test-value-1', + 'test-dimension-2': 'test-value-2', + }; + const testDimensionName = 'test-dimension'; + + // Act + metrics.addDimensions(dimensionsToBeAdded); + metrics.addDimensions({ [testDimensionName]: value }); + + // Assess + expect(consoleWarnSpy).toHaveBeenCalledWith( + `Dimension value is invalid for ${testDimensionName} and will be skipped.` + ); + expect(metrics).toEqual( + expect.objectContaining({ + dimensions: { + 'test-dimension-1': 'test-value-1', + 'test-dimension-2': 'test-value-2', + }, + }) + ); + }); + } + }); }); describe('Method: addMetadata', () => { From 188e18ed04040b159b31d778add61689bceb77e7 Mon Sep 17 00:00:00 2001 From: arnabrahman Date: Tue, 19 Nov 2024 09:32:05 +0600 Subject: [PATCH 4/4] refactor: make warning message similar to python implementation --- packages/metrics/src/Metrics.ts | 4 ++-- packages/metrics/tests/unit/Metrics.test.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/metrics/src/Metrics.ts b/packages/metrics/src/Metrics.ts index be94758429..ea4fba2270 100644 --- a/packages/metrics/src/Metrics.ts +++ b/packages/metrics/src/Metrics.ts @@ -230,7 +230,7 @@ class Metrics extends Utility implements MetricsInterface { public addDimension(name: string, value: string): void { if (!value) { this.#logger.warn( - `Dimension value is invalid for ${name} and will be skipped.` + `The dimension ${name} doesn't meet the requirements and won't be added. Ensure the dimension name and value are non empty strings` ); return; } @@ -262,7 +262,7 @@ class Metrics extends Utility implements MetricsInterface { newDimensions[dimensionName] = value; } else { this.#logger.warn( - `Dimension value is invalid for ${dimensionName} and will be skipped.` + `The dimension ${dimensionName} doesn't meet the requirements and won't be added. Ensure the dimension name and value are non empty strings` ); } } diff --git a/packages/metrics/tests/unit/Metrics.test.ts b/packages/metrics/tests/unit/Metrics.test.ts index dde89a87a5..f555e91515 100644 --- a/packages/metrics/tests/unit/Metrics.test.ts +++ b/packages/metrics/tests/unit/Metrics.test.ts @@ -460,7 +460,7 @@ describe('Class: Metrics', () => { // Assess expect(consoleWarnSpy).toHaveBeenCalledWith( - `Dimension value is invalid for ${testDimensionName} and will be skipped.` + `The dimension ${testDimensionName} doesn't meet the requirements and won't be added. Ensure the dimension name and value are non empty strings` ); expect(metrics).toEqual( expect.objectContaining({ @@ -593,7 +593,7 @@ describe('Class: Metrics', () => { // Assess expect(consoleWarnSpy).toHaveBeenCalledWith( - `Dimension value is invalid for ${testDimensionName} and will be skipped.` + `The dimension ${testDimensionName} doesn't meet the requirements and won't be added. Ensure the dimension name and value are non empty strings` ); expect(metrics).toEqual( expect.objectContaining({