Skip to content

fix(metrics): skip empty string dimension values #3319

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 19, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion packages/metrics/src/Metrics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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}`
Expand All @@ -239,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
Expand All @@ -249,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(
Expand Down
86 changes: 86 additions & 0 deletions packages/metrics/tests/unit/Metrics.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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', () => {
Expand Down