Skip to content

fix(metrics): warnings when there are default_dimensions - #8404

Open
ericbn wants to merge 1 commit into
aws-powertools:developfrom
ericbn:fix_warnings_in_flush_metrics
Open

fix(metrics): warnings when there are default_dimensions#8404
ericbn wants to merge 1 commit into
aws-powertools:developfrom
ericbn:fix_warnings_in_flush_metrics

Conversation

@ericbn

@ericbn ericbn commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Issue number: closes #8402

Summary

Changes

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.

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.

[WARNING] aws_lambda_powertools/metrics/metrics.py:124: PowertoolsUserWarning: Dimension 'Key' has already been added. The previous value will be overwritten.
  self.provider.add_dimension(name=name, value=value)

[WARNING] aws_lambda_powertools/metrics/provider/cloudwatch_emf/cloudwatch.py:571: PowertoolsUserWarning: Dimension 'Key' has already been added. The previous value will be overwritten.
  self.add_dimension(name, value)

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.

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.
@ericbn
ericbn requested a review from a team as a code owner August 26, 2026 18:26
@ericbn
ericbn requested a review from hjgraca August 26, 2026 18:26
@powertools-for-aws-oss-automation powertools-for-aws-oss-automation Bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 26, 2026
@sonarqubecloud

Copy link
Copy Markdown

@ericbn

ericbn commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Disclaimer: the code changes and description here were not produced using any AI.

@vishwakt

Copy link
Copy Markdown
Contributor

Nice catch on default_dimensions or {}. An empty dict is falsy, so the provider was silently creating a new dict instead of sharing the one Metrics passed in, and the sharing only kicked in if a Metrics instance was constructed after defaults already existed. That one is worth fixing regardless of which approach lands.

I did find two behavior gaps while testing this branch though.

  1. the scenario from Bug: metrics.set_default_dimensions emits PowertoolsUserWarning: Dimension 'Key' has already been added. The previous value will be overwritten. #8402 still warns on warm invocations. provider.set_default_dimensions still goes through add_dimension with the original condition, so running the issue's own snippet per invocation (Metrics + set_default_dimensions + flush inside the handler) gives 0 warnings on the first invocation and 2 on every one after, since the keys are back in dimension_set (re-seeded by clear_metrics) and default_dimensions:
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()
  1. replacing set_default_dimensions with a plain dimension_set.update() in clear_metrics skips the str cast in add_dimension. default_dimensions holds the raw user values, so with set_default_dimensions(version=2) the first flush emits "version": "2" but every flush after emits "version": 2 as an int, which EMF does not accept for dimension values. The old code re-cast on every re-seed; going through add_dimension (or casting in set_default_dimensions) keeps that.

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.

@ericbn

ericbn commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for testing this so thoroughly.

On both points you raised, I checked them against the code as it stood before this PR (commit 8db13c761), and in both cases what you're describing is existing behavior, not something this PR introduces:

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 add_dimension call that #8402 is literally about) and 6 warnings on every invocation after that. With this PR, that's 0 on the first invocation and 2 on the ones after. So this PR doesn't fully eliminate warnings in the repeated-set_default_dimensions-per-invocation pattern, but it cuts them substantially in every case, including the exact case you tested. Given that, I'd treat the residual 2 warnings as a smaller follow-up rather than a blocker on this fix — happy to take a pass at eliminating those too, but it shouldn't hold up landing the bigger win here.

2. Type reverts to non-str — same story. __init__ already did self.dimension_set.update(**self._default_dimensions) before this PR, with no str() cast, so the "version": 2 int leaking through on the second+ invocation is pre-existing behavior — the clear_metrics line in this PR is doing exactly what __init__ already did, just relocated to run on every flush instead of only at construction. So this PR isn't regressing anything here; it's carrying forward a gap that already existed. Worth its own fix, but again, separable from this change.

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 metrics.metric_set etc. after construction used to silently decouple the Metrics instance from the provider's actual state with no error and no warning. Trading that for a loud AttributeError seems like the safer direction even if it's technically a breaking change in the strict sense.

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.

@vishwakt

Copy link
Copy Markdown
Contributor

On the str cast I have to push back, because I tested this on develop (8db13c7) before commenting. With set_default_dimensions(version=2) and a flush per invocation, develop serializes "version": "2" on every invocation, because both re-seed paths (clear_metrics and repeated set_default_dimensions) go through add_dimension's cast. This branch emits "2" on the first invocation and int 2 on every one after. You're right that __init__'s update was already uncast, and there is a narrow pre-existing leak (construct a second Metrics instance after defaults are set, then flush without touching defaults), but on develop the common single-instance patterns always re-cast. This PR makes the uncast update the steady-state re-seed on every flush, so the int output becomes the normal behavior rather than an edge case. That part I'd fix before landing

@ericbn

ericbn commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

metrics size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: metrics.set_default_dimensions emits PowertoolsUserWarning: Dimension 'Key' has already been added. The previous value will be overwritten.

2 participants