Repository navigation
fix(metrics): skip non-finite metric values with a warning - #8797
anishmehta24 wants to merge 2 commits into
Conversation
boring-cyborg
Bot
added
documentation
powertools-for-aws-oss-automation
Bot
added
the
size/M
blacksoxx
left a comment
blacksoxx
left a comment
There was a problem hiding this comment.
Comment: one P3 inline on the rewritten finally in single_metric; the skip itself reads right. The isfinite check sits before the float(value) conversion that already raised for out-of-range ints and signalling-NaN decimals, so it adds no new failure mode. I did not assess the raise_on_empty_metrics=True question in the description; that is the maintainers' call.
| metric_set = metric.serialize_metric_set() | ||
| finally: | ||
| print(json.dumps(metric_set, separators=(",", ":"))) | ||
| if publish: |
There was a problem hiding this comment.
[P3] publish only turns false on the normal exit path. When the with body raises, metric_set is still None and this prints the literal null that 3.40.0 prints today, so one non-EMF line lands on stdout under every exception:
with single_metric(name="x", unit=MetricUnit.Count, value=1, namespace="ns"):
raise RuntimeError("boom")
# stdout: 'null\n' on aws-lambda-powertools 3.40.0Pre-existing, not introduced here. Since this block is being rewritten anyway, if publish and metric_set is not None: covers both cases.
There was a problem hiding this comment.
Done in 287b8c4: the finally now prints only when a metric set was serialized, so the raising body prints nothing. I dropped publish along the way. test_single_metric_body_raises covers it, and it fails on develop with null.
There was a problem hiding this comment.
Thanks, checked 287b8c4: the finally now gates on metric_set is not None, so neither the raising body nor the skipped-value path prints anything, and test_single_metric_body_raises pins the empty stdout. Nothing further from my side.
|
leandrodamascena
left a comment
leandrodamascena
left a comment
There was a problem hiding this comment.
Thanks @anishmehta24, that was quick! Both add_metric paths covered, and I hadn't noticed the null line when the with body raises, good catch. On raise_on_empty_metrics=True with every value skipped, raising is right: the user asked to be told about an empty flush, and this is one. Goes out in 3.41.0, no need to rebase, you'll be in the release notes.
Issue number: closes #8796
Summary
Changes
add_metricin bothAmazonCloudWatchEMFProviderandMetricManager(used bysingle_metric) now checksmath.isfinite(value)after the unit and resolution checks, so a bad unit still raises as before. ANaNor infinite value is skipped with aPowertoolsUserWarning, per the discussion in the issue.single_metricdoesn't flush when its only value was skipped. Otherwise the empty metric set would raiseSchemaValidationError: Must contain at least one metric., where today it raises nothing.It also no longer prints a
nullline when thewithbody raises. 3.40.0 does that too, but this block is rewritten here anyway (suggested in review).Added a docs note under "Creating metrics" and the regenerated agent skill reference for it (I left out the unrelated layer version bump the script also picked up).
User experience
Before:
metrics.add_metric(name="Latency", unit=MetricUnit.Milliseconds, value=float("nan"))writes a bareNaNinto the EMF line, so CloudWatch drops every metric in that flush without an error. After: a warning names the metric, it's left out, and the rest of the flush is published.One thing to check: with
raise_on_empty_metrics=True, a flush where every value was skipped now raisesSchemaValidationError, like any empty flush. Happy to change that if you'd rather it didn't.Verification
Added
test_add_metric_with_non_finite_value(nan, inf, -inf, parses the output withparse_constantso a bare constant fails) andtest_single_metric_with_non_finite_valueintests/functional/metrics/required_dependencies/test_metrics_cloudwatch_emf.py. Also addedtest_single_metric_body_raises. All 5 fail ondevelopand pass with the fix.pytest tests/unit/metrics tests/functional/metricson Python 3.12: 170 passed, 1 failed (test_datadog_write_to_log_with_env_variable, which fails the same way ondevelopwhen run with the rest of the folder and passes alone).ruff format,ruff checkandmypyare clean apart fromtime.tzsetin a logger example, which mypy flags on Windows. I didn't send the output to CloudWatch.I used an AI assistant (Claude Code) for this change.
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.