-
Notifications
You must be signed in to change notification settings - Fork 162
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
base: main
Are you sure you want to change the base?
improv(metrics): consistent duplicate dimension handling #4235
Conversation
…ws-powertools#4222) Co-authored-by: Swopnil Dangol <[email protected]>
Co-authored-by: Andrea Amorosi <[email protected]>
if (Object.hasOwn(this.defaultDimensions, key)) { | ||
const currentValue = this.defaultDimensions[key]; | ||
const suppressOverwriteWarning = | ||
key === 'service' && currentValue === this.defaultServiceName; |
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@@ -1058,8 +1090,6 @@ class Metrics extends Utility implements MetricsInterface { | |||
functionName, | |||
} = options; | |||
|
|||
this.setEnvConfig(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@dreamorosi @sdangol I’d like to get your thoughts on how best to solve this. |
@uttam282005 I think having the flag to maintain the internal calls would probably add some complexity to this. Let me think this through for some more time, discuss with the team and get back to you |
private static supressWarnings = false;
private static supressSetDefaultDimensionsWarnings<T>(fn: () => T): T {
Metrics.supressWarnings = true;
try {
return fn();
} finally {
Metrics.supressWarnings = false;
}
} How is this approach? |
Summary
This PR restores and improves the warning mechanism for overwritten default dimensions in the
Metrics
utility:Changes
captureColdStartMetric()
would emit duplicate warnings for theservice
dimension.service
dimension is different from the internal default (i.e., truly user-defined).Additional Fix
Previously, passing
defaultDimensions
with aservice
key in the constructor triggered unnecessary warnings, beacauseservice
was set internally as a default dimension viasetService()
:Now, this behavior is handled correctly. No warning is emitted if the
service
indefaultDimensions
is the same as the internal default service name.Final Fix Summary
setDefaultDimensions()
insidecaptureColdStartMetric()
, which previously caused a second warning.Metrics
withservice
value indefaultDimensions
.Issue number: #4134
Note
Instantiating the object like this will still emit a warning as user is trying to overwrite previously passed
serviceName
, even if they are equal. This behaviour is consistent with other methods likeaddDimensions
andaddDimension
.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.