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

Implement WithExplicitBucketBoundaries option in the metric SDK #4605

Merged
merged 7 commits into from Oct 31, 2023

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Oct 11, 2023

Fixes #4094

I looked at both #4341 and #4340 when implementing this, but mostly relied on #4340. I omitted changes to the existing lookup() and aggs() functions (made in #4340) to minimize the diff. If we still want those changes, I'd prefer refactoring as a follow-up.

The precedence of bucket boundaries is implemented as follows:

  1. Boundaries specified in the View
  2. Boundaries specified on the instrument
  3. Boundaries provided by the reader, or the default boundaries if the reader uses the DefaultAggregation

@dashpole dashpole added area:metrics Part of OpenTelemetry Metrics enhancement New feature or request labels Oct 11, 2023
@dashpole dashpole force-pushed the explicit_bucket_advisory_sdk branch 2 times, most recently from 30aecd6 to 1ee3f46 Compare October 11, 2023 19:04
@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

Merging #4605 (5d0cea9) into main (5ec67e8) will increase coverage by 0.0%.
The diff coverage is 88.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #4605   +/-   ##
=====================================
  Coverage   81.6%   81.7%           
=====================================
  Files        225     225           
  Lines      17991   18047   +56     
=====================================
+ Hits       14692   14753   +61     
+ Misses      2999    2996    -3     
+ Partials     300     298    -2     
Files Coverage Δ
sdk/metric/meter.go 89.0% <100.0%> (+3.1%) ⬆️
sdk/metric/pipeline.go 86.3% <77.2%> (+0.4%) ⬆️

@dashpole dashpole added the pkg:SDK Related to an SDK package label Oct 11, 2023
@MrAlias MrAlias added this to the v1.20.0 milestone Oct 24, 2023
sdk/metric/meter.go Outdated Show resolved Hide resolved
sdk/metric/meter.go Outdated Show resolved Hide resolved
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@MadVikingGod MadVikingGod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with this overall.

How much do you think would need to change if there was a WithExponential() option?

@dashpole
Copy link
Contributor Author

How much do you think would need to change if there was a WithExponential() option?

You would basically just change HistogramAggregators. It would probably take the full config instead of buckets as a separate argument. But overall, not much change I think.

@pellared pellared merged commit b2bb2ad into open-telemetry:main Oct 31, 2023
25 checks passed
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 enhancement New feature or request pkg:SDK Related to an SDK package
Projects
Development

Successfully merging this pull request may close these issues.

Add experimental histogram advice API
4 participants