-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Implement WithExplicitBucketBoundaries option in the metric SDK #4605
Conversation
30aecd6
to
1ee3f46
Compare
Codecov Report
@@ 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
|
1ee3f46
to
51dd529
Compare
5fd9764
to
2c1bc22
Compare
2c1bc22
to
b6ed43a
Compare
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
There was a problem hiding this 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?
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. |
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: