fix(metrics): warnings when there are default_dimensions - #8404
Conversation
This fixes and reorganizes the code in a few ways:
* Make it clearer that _metrics, _dimensions, _metadata and
_default_dimensions are class attributes. They don't need to be also
set as instance attributes when constructing Metrics, as they're only
meant to be used to construct the provider with shared data.
* Expose metric_set, dimension_set, metadata_set and default_dimensions
as attributes just to keep backwards compatibility. Don't expose setters
for these as previously setting them would have no side effect. Now
users will get an error, which is a small breaking change but arguably
for something they should never be doing anyway.
* Fix setting the default_dimensions in AmazonCloudWatchEMFProvider, as
`default_dimensions or {}` was setting a new dict instance when the
given default_dimensions was empty and we want to share the given dict.
|
|
Disclaimer: the code changes and description here were not produced using any AI. |
|
Nice catch on I did find two behavior gaps while testing this branch though.
def handler():
metrics = Metrics(namespace="MyNamespace", service="MyService")
metrics.set_default_dimensions(Environment="dev", Resource="MyJob")
metrics.add_metric(name="MyJobDuration", unit=MetricUnit.Seconds, value=42)
metrics.flush_metrics()
On the read-only properties: agreed nobody should be assigning those, but it is still a breaking change on a public attribute, and this file's own maintenance note says breaking customers before v3 is off the table, so that part is probably a maintainer call. |
|
Thanks for testing this so thoroughly. On both points you raised, I checked them against the code as it stood before this PR (commit 1. Repeat invocation warnings — before this PR, the same repeated-invocation scenario from #8402 produced 4 warnings on the very first invocation (from the double 2. Type reverts to non-str — same story. 3. Read-only properties — agreed this is ultimately a maintainer call under the pre-v3 policy. Worth noting the previous behavior wasn't a clean no-op either: reassigning Given all three points trace back to pre-existing behavior rather than regressions introduced here, I'd like to keep this PR as-is and track the two remaining gaps as follow-ups. |
|
On the str cast I have to push back, because I tested this on develop (8db13c7) before commenting. With |
|
@vishwakt our AIs are discussing here over a PR and corresponding issue that were not even triaged by the project maintainers yet. Please I want to wait for the project maintainers feedback first before proceeding further with any other conversation. |



Issue number: closes #8402
Summary
Changes
This fixes and reorganizes the code in a few ways:
default_dimensions or {}was setting a new dict instance when the given default_dimensions was empty and we want to share the given dict.User experience
This fix will not produce the following warnings anymore as it makes sure default dimensions are only set once when using metrics.add_metric and metrics.flush_metrics methods.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.