fix(metrics): warnings when there are default_dimensions

[ericbn]

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 not be propagated and actually used by the provider. 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.
• Properly cast dimension value to str and do other validations in all code that updates dimension_set.
• No need to check if isinstance(value, str) before casting to str. This is already optimized in CPython, which reuses the same instance if value is already an str. This is unnecessary overengineering.

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.

[vishwakt]

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()

2. 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]

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]

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]

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

[ericbn]

This comment seems outdated, as we're already on v3.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[leandrodamascena]

Hi Eric, thank you for taking the time to investigate this and for going deeper into the state-sharing problem. Your finding around default_dimensions or {} is important, and I have asked for that fix to be incorporated into #8403, with credit to you.

After comparing both approaches, I am leaning towards continuing with #8403 for the specific bug reported in #8402. It keeps the change focused and also avoids warnings when the same default dimensions are registered again during warm invocations. In this PR, that scenario can still warn because the existing dimension is treated as an overwrite even when its value has not changed.

The read-only properties are the one part that makes me hesitant. Even if assigning to those attributes was not very useful before, changing them to raise an error could still affect someone. I think we should look at that separately.

Would you be comfortable with us continuing with #8403 for #8402 and handling the broader cleanup separately? I do not want to close this PR before hearing your thoughts.

Thank you again for the investigation and especially for catching the empty dictionary issue.

[ericbn]

Oi @leandrodamascena! Thanks and I agree there's too much going on in this PR. Also agree replacing the direct references to the shared data by read-only attributes is not worth it -- there are other ways to break these references. Let me revert that, but also break this PR into the smaller different changes, if you agree. I'll list them below so we can discuss first.

Regarding #8403, and since I was the (apparently first) one complaining about the warnings in #8402, for your consideration: I don't think it's a good idea to start hiding the warnings when the key is being set twice but with the same value. The warnings are currently alerting to places where potentially code is being called twice and thanks to them I was able to dig deeper into issues that currently exist. Hiding these warnings when the value is the same seems more of a shortcut to try to fix a symptom, and it's not a fix for root causes, which are the scenarios where code is being called twice.

And here's the break down of the different changes I'm proposing:

1. Fix the dimension values cast to str and other verifications only being done in the add_dimension method, but not from the constructor or from clear_metrics in the provider.
2. Simplify value if isinstance(value, str) else str(value) to str(value). This is just a small refactoring to some unnecessary complicated code. I always enjoy being able to do these small quality of life contributions and hopefully you'll like this one. This is code surrounding the fix above, but definitely can be separated on its own PR.
3. Fix duplicated code in Metrics methods (set_default_dimensions and clear_default_dimensions) that does both self.provider.x.y() and self.x.y(), as this is just doing the same operation twice in the same x instance that has duplicated references. and This is what was leading me to also remove the duplicated references altogether, replacing the self.x by a read-only property that uses the self.provider.x reference (which I won't anymore).
4. Two other small fixes:
   1. Change check for dimension being set twice in add_dimension from if name in self.dimension_set or name in self.default_dimensions: to just if name in self.dimension_set:. Checking also if name in self.default_dimensions in unnecessary and leads to warnings when trying to reset data in clear_metrics (related to fix in 1).
   2. Fix setting self.default_dimensions with default_dimensions if default_dimensions is not None else {} in provider. The current default_dimensions or {} breaks the chain of using the shared data mentioned in item 3 when the given default_dimensions is empty.

Item 3 and 4 are what fixes #8402.

[leandrodamascena]

Thanks @ericbn for the patience here. I spent more time on both PRs because I did not want to lose the valid problems you found.

With #8403, the original warning from #8402 is covered, and your fix for the empty default_dimensions dictionary is included with co-authorship.

I would like to keep this PR for the other problem you found: non-string default dimensions can return to their original type when another Metrics instance is created.

For that, could we reduce this PR to only that fix? I would remove the read-only properties, the direct use of Metrics._metrics and the unrelated changes in base.py.

There are also two cases to handle in the new implementation. Calling add_dimension() from the provider constructor warns when a second Metrics instance is created, and with 29 default dimensions it raises SchemaValidationError because the limit is checked before checking whether the dimension already exists.

I think we should cover these cases here:

• non-string defaults remain strings in a second Metrics instance;
• creating another instance does not emit warnings;
• creating another instance with 29 defaults does not raise;
• existing custom provider and subclass behavior remains unchanged.

We should also remove closes #8402 from this PR since #8403 will close it.

Thank you for going deeper into this. The normalization issue is real and worth fixing; I just want to keep it separate from the public state changes.

[codecov]

Codecov Report

❌ Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 96.65%. Comparing base (a8df37d) to head (fca26c7).
⚠️ Report is 33 commits behind head on develop.

Files with missing lines	Patch %	Lines
aws_lambda_powertools/metrics/base.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #8404      +/-   ##
===========================================
+ Coverage    96.64%   96.65%   +0.01%     
===========================================
  Files          296      296              
  Lines        14767    14767              
  Branches      1246     1245       -1     
===========================================
+ Hits         14271    14273       +2     
+ Misses         361      359       -2     
  Partials       135      135              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.