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 Aggregation: Fix bucket detection logic, performance improvements, and benchmark tests #1869

Merged
merged 24 commits into from
Jan 12, 2023

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Dec 15, 2022

Fixes #1864, #1865, #1866

Changes

  1. Fixed the bucket detection logic
    • Lower bound value is exclusive, Upper bound value is inclusive ( as mentioned in specs )
    • The last bucket was not getting populated.
  2. The default boundaries for UINT64 was incorrect, changed it to use the values as specified in above specs link
  3. Added separate tests for histogram buckets with default and custom boundaries (shamelessly taken from the issue).
  4. Pass custom boundaries config data while merging and diff-ing Histogram aggregation.
  5. Change boundaries data type from std::list to std::vector. And use binary search for searching the right bucket for a measurement.
  6. Add histogram performance benchmark tests.

Benchmark result for histogram aggregation (prior to changes in point 5 above):

$ ./histogram_aggregation_benchmark
2022-12-16 15:18:01
Running ./histogram_aggregation_benchmark
Run on (20 X 2808.01 MHz CPU s)
CPU Caches:
  L1 Data 32K (x10)
  L1 Instruction 32K (x10)
  L2 Unified 256K (x10)
  L3 Unified 20480K (x1)
Load Average: 2.90, 1.64, 0.61
------------------------------------------------------------------
Benchmark                        Time             CPU   Iterations
------------------------------------------------------------------
BM_HistogramAggregation        740 ns          740 ns       936807

Benchmark result for histogram aggregation (after changes in point 5 above):

$ ./histogram_aggregation_benchmark
2022-12-16 15:17:58
Running ./histogram_aggregation_benchmark
Run on (20 X 2808.01 MHz CPU s)
CPU Caches:
  L1 Data 32K (x10)
  L1 Instruction 32K (x10)
  L2 Unified 256K (x10)
  L3 Unified 20480K (x1)
Load Average: 2.90, 1.64, 0.61
------------------------------------------------------------------
Benchmark                        Time             CPU   Iterations
------------------------------------------------------------------
BM_HistogramAggregation        552 ns          552 ns      1297628

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@lalitb lalitb requested a review from a team December 15, 2022 07:16
@codecov
Copy link

codecov bot commented Dec 15, 2022

Codecov Report

Merging #1869 (779a840) into main (57d1a47) will increase coverage by 0.51%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
...metry/sdk/metrics/aggregation/aggregation_config.h 100.00% <ø> (ø)
...nclude/opentelemetry/sdk/metrics/data/point_data.h 100.00% <ø> (ø)
exporters/ostream/test/ostream_metric_test.cc 100.00% <100.00%> (ø)
...ry/sdk/metrics/aggregation/histogram_aggregation.h 91.31% <100.00%> (+1.31%) ⬆️
...k/src/metrics/aggregation/histogram_aggregation.cc 88.47% <100.00%> (+0.23%) ⬆️
sdk/src/trace/batch_span_processor.cc 91.48% <0.00%> (+0.78%) ⬆️
ext/src/http/client/curl/http_client_curl.cc 81.44% <0.00%> (+1.14%) ⬆️
sdk/src/metrics/meter.cc 54.22% <0.00%> (+8.44%) ⬆️
...ntelemetry/sdk/metrics/state/sync_metric_storage.h 87.10% <0.00%> (+25.81%) ⬆️

@lalitb lalitb changed the title Fix histogram bucket detection Fix histogram bucket detection, performance improvements, and benchmark tests Dec 16, 2022
@lalitb lalitb changed the title Fix histogram bucket detection, performance improvements, and benchmark tests Histogram Aggregation: Fix bucket detection logic, performance improvements, and benchmark tests Dec 16, 2022
@esigo esigo self-assigned this Jan 2, 2023
@lalitb
Copy link
Member Author

lalitb commented Jan 5, 2023

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.

A pull request opened by an Approver / Maintainer can be merged with only one approval from Approver / Maintainer (at different company).

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.

@ThomsonTan
Copy link
Contributor

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.

A pull request opened by an Approver / Maintainer can be merged with only one approval from Approver / Maintainer (at different company).

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.

@lalitb lalitb mentioned this pull request Jan 9, 2023
3 tasks
@lalitb lalitb enabled auto-merge (squash) January 11, 2023 00:09
@lalitb lalitb disabled auto-merge January 11, 2023 00:10
Copy link
Member

@esigo esigo left a 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.

lalitb and others added 2 commits January 11, 2023 15:23
…gregation.h

Co-authored-by: Ehsan Saei <71217171+esigo@users.noreply.github.com>
@lalitb
Copy link
Member Author

lalitb commented Jan 11, 2023

LGTM Thanks for the PR. Just had a suggestion :) Sorry for the delay.

No issues. Thanks for reviewing :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Why boundaries is std::list
4 participants