Skip to content

improv(metrics): consistent duplicate dimension handling #4235

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

Closed
Show file tree
Hide file tree
Changes from all 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
50 changes: 40 additions & 10 deletions packages/metrics/src/Metrics.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import { Console } from 'node:console';
import {
isIntegerNumber,
isNullOrUndefined,
isNumber,
isRecord,
isString,
isStringUndefinedNullEmpty,
Utility,
Expand Down Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -433,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);
Expand Down Expand Up @@ -824,13 +823,46 @@ 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)) {
const currentValue = this.defaultDimensions[key];
const suppressOverwriteWarning =
key === 'service' && currentValue === this.defaultServiceName;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@uttam282005 I tried to run the e2e tests against the PR, and one of the tests is failing. It seems like there's still one warning that gets logged if we use captureColdStartMetric.
I'm not sure, but I think it has to do with this.defaultServiceName. We're doing this in setService to get the service, do we need something similar?

service ||
this.getCustomConfigService()?.getServiceName() ||
this.#envConfig.serviceName ||
this.defaultServiceName;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the warning occurs because the service dimension is initially set via setService(), this value can come from a user-provided serviceName, an environment variable, or a custom config service. Later, when setDefaultDimensions() is called (and includes a service key), we get a warning indicating the service dimension is being overwritten.

In many cases, this warning is correct, it's a user attempting to override a previously set user-defined service value, which we want to surface. However, the problem arises when setDefaultDimensions() is called internally (e.g., within captureColdStartMetric() or during initialization like singleMetric() ). In these cases, the overwrite is intentional and should not trigger a warning.

To address this, the cleanest solution I can think of is introducing a private boolean flag that marks such internal calls. This flag would let us suppress the warning when setDefaultDimensions() is invoked by internal logic rather than user input, preserving the intent of the warning without polluting logs in expected flows.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The root cause is the call to single metric which is emitting the same behaviour that I have written in the note section of the PR.

if (!suppressOverwriteWarning) {
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;
}

Expand Down Expand Up @@ -1058,8 +1090,6 @@ class Metrics extends Utility implements MetricsInterface {
functionName,
} = options;

this.setEnvConfig();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why were these two removed and extracted in the constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setEnvConfig() and setConsole() calls from setOptions() were moved to the constructor to ensure logger is initialized before any method (eg. setDefaultDimensions) uses it.

this.setConsole();
this.setCustomConfigService(customConfigService);
this.setDisabled();
this.setNamespace(namespace);
Expand Down
8 changes: 6 additions & 2 deletions packages/metrics/tests/unit/customTimestamp.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down
100 changes: 100 additions & 0 deletions packages/metrics/tests/unit/dimensions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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('test', MetricUnit.Count, 1);

// Assess
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();

// 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`
);

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');

// Assess
expect(console.warn).not.toHaveBeenCalled();

metrics.addMetric('someMetric', MetricUnit.Count, 1);

expect(console.log).toHaveEmittedEMFWith(
expect.objectContaining({
service: 'hello-world',
})
);
});
});
4 changes: 3 additions & 1 deletion packages/metrics/tests/unit/initializeMetrics.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down