-
Notifications
You must be signed in to change notification settings - Fork 825
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
fix(sdk-metrics): ignore NaN value recordings for histograms #4455
fix(sdk-metrics): ignore NaN value recordings for histograms #4455
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4455 +/- ##
==========================================
+ Coverage 90.28% 92.41% +2.12%
==========================================
Files 155 330 +175
Lines 3603 9518 +5915
Branches 787 2030 +1243
==========================================
+ Hits 3253 8796 +5543
- Misses 350 722 +372
|
Specification: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#handle-all-normal-values
|
Ah. Should've checked that first. Thanks for pointing that out. I'll convert to draft and change it to fit the specification. |
4c7ca1a
to
7898d31
Compare
Changed it to ignore |
I think it would be better to handle |
@legendecas I think that section fo the spec it only intended for Histogram Aggregations. The behavior for counters seems to be unspecified as per this section of the spec: I agree that allowing |
…lemetry#4455) * fix(sdk-metrics): ignore NaN value recordings * fix(changelog): add changelog entry * test(exporter-prometheus): adapt tests * fix(sdk-metrics): ignore in accumulation instead * fix(changelog): update changelog
Which problem is this PR solving?
Currently you can add
NaN
to your instruments, which currently can cause two problems:NaN
foreverNaN
is added to the total count, but not to any bucketThis PR changes the Histogram aggregations so that
NaN
is dropped.Possibly relates to #4450(cause for that was something entirely different)Type of change
How Has This Been Tested?