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

Histogram buckets should be 1 more than boundaries #614

Merged
merged 2 commits into from
Aug 28, 2023

Conversation

albertored
Copy link
Contributor

@albertored albertored commented Aug 27, 2023

Histogram buckets should be 1 more than histogram boundaries. At the moment values greater than the last boundary (let's call it T(n)) are accumulated in the boundary (T(n-1), T(n)] instead of (T(n), +infinity)

@albertored albertored requested a review from a team August 27, 2023 12:47
@codecov
Copy link

codecov bot commented Aug 27, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (3581046) 72.49% compared to head (57b6748) 72.49%.

❗ Current head 57b6748 differs from pull request most recent head 04effcc. Consider uploading reports for the commit 04effcc to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #614   +/-   ##
=======================================
  Coverage   72.49%   72.49%           
=======================================
  Files          61       61           
  Lines        1923     1923           
=======================================
  Hits         1394     1394           
  Misses        529      529           
Flag Coverage Δ
api 69.05% <ø> (ø)
elixir 17.98% <ø> (ø)
erlang 73.79% <ø> (ø)
exporter 66.66% <ø> (ø)
sdk 77.96% <ø> (ø)
zipkin 54.16% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@albertored
Copy link
Contributor Author

albertored commented Aug 27, 2023

Also, as per the spec it seems that our default boundaries are missing some values (2500, 5000, 7500, 10000), I can add them in this PR

@tsloughter
Copy link
Member

Thanks! And yea, the spec changed since this was written.

@albertored albertored force-pushed the fix-histo-buckets branch 3 times, most recently from be9194e to 5950dd4 Compare August 28, 2023 07:00
@albertored
Copy link
Contributor Author

@tsloughter default boundaries aligned with the specification, it should be ready to be merged

@tsloughter
Copy link
Member

Needs a changelog update.

Adpat default boundaries to last version of the specs
@albertored
Copy link
Contributor Author

@tsloughter done

@tsloughter tsloughter merged commit efc87f3 into open-telemetry:main Aug 28, 2023
@albertored albertored deleted the fix-histo-buckets branch August 28, 2023 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants