Skip to content
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

Ignore +/- Inf and NaN for exponential histogram measurement #4446

Merged
merged 3 commits into from Aug 16, 2023

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Aug 14, 2023

Fix #4445

Similar to Java, follow the specification1 and do not record these values.

No changelog entry is made as the exponential histogram has not yet been released.

Footnotes

  1. https://github.com/open-telemetry/opentelemetry-specification/blob/v1.24.0/specification/metrics/sdk.md#handle-all-normal-values

@MrAlias MrAlias added pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics Skip Changelog PRs that do not require a CHANGELOG.md entry labels Aug 14, 2023
@MrAlias MrAlias added this to the v1.17.0/v0.40.0 milestone Aug 14, 2023
@codecov
Copy link

codecov bot commented Aug 15, 2023

Codecov Report

Merging #4446 (959fdab) into main (d78820e) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #4446     +/-   ##
=======================================
- Coverage   78.9%   78.9%   -0.1%     
=======================================
  Files        254     254             
  Lines      20646   20650      +4     
=======================================
+ Hits       16301   16303      +2     
- Misses      4000    4002      +2     
  Partials     345     345             
Files Changed Coverage Δ
...metric/internal/aggregate/exponential_histogram.go 96.3% <100.0%> (+<0.1%) ⬆️

... and 1 file with indirect coverage changes

@MrAlias MrAlias merged commit a5ff7af into open-telemetry:main Aug 16, 2023
22 checks passed
@MrAlias MrAlias deleted the ignore-nan-inf branch August 16, 2023 14:11
MrAlias added a commit to MrAlias/opentelemetry-go that referenced this pull request Aug 16, 2023
A guard was added in open-telemetry#4446 to prevent non-normal float64 from being
recorded. This was added in the low-level `record` method meaning that
the higher-level `measure` method will still keep a record of the
invalid value measurement, just with a zero-value.

This fixes that issue by moving the guard to the `measure` method.
MrAlias added a commit that referenced this pull request Aug 18, 2023
A guard was added in #4446 to prevent non-normal float64 from being
recorded. This was added in the low-level `record` method meaning that
the higher-level `measure` method will still keep a record of the
invalid value measurement, just with a zero-value.

This fixes that issue by moving the guard to the `measure` method.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics pkg:SDK Related to an SDK package Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
No open projects
4 participants