-
Notifications
You must be signed in to change notification settings - Fork 440
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 Aggregation: Fix bucket detection logic, performance improvements, and benchmark tests #1869
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1869 +/- ##
==========================================
+ Coverage 85.71% 86.21% +0.51%
==========================================
Files 171 171
Lines 5252 5257 +5
==========================================
+ Hits 4501 4532 +31
+ Misses 751 725 -26
|
sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h
Outdated
Show resolved
Hide resolved
Can I merge this PR, as this has few trivial bug fixes, and performance improvement for histogram aggregation, don't want to keep this on hold for long. Though it is approved, I realize it is still not meeting our merge policy, so checking if there are any issue :) I can incorporate any later comments as separate PR if required.
p.s. - As special case, we have been merging ETW exporter related PRs and few smaller PRs with one approval from same company, but that should be acceptable. |
Thanks for raising this. @esigo could you please help take a look when you have time? For ETW exporter, it seems more appropriate to move it to contrib repo. |
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.
LGTM
Thanks for the PR. Just had a suggestion :)
Sorry for the delay.
sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h
Outdated
Show resolved
Hide resolved
…gregation.h Co-authored-by: Ehsan Saei <71217171+esigo@users.noreply.github.com>
No issues. Thanks for reviewing :) |
Fixes #1864, #1865, #1866
Changes
Benchmark result for histogram aggregation (prior to changes in point 5 above):
Benchmark result for histogram aggregation (after changes in point 5 above):
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes