Binary search for large bucket count histograms #3252

mic-max · 2022-05-04T22:38:02Z

Changes

When using Metric.DefaultHistogramCountForBinarySearch or more buckets for a histogram, it will use binary search instead of linear search.
- 1.0x faster for ~50 buckets
- 3.7x faster for 1000 buckets no labels

Histogram Benchmark results from main with same the same parameters.

Method	BoundCount	Mean	Error	StdDev
HistogramHotPath	10	49.47 ns	1.016 ns	1.725 ns
HistogramHotPath	49	60.95 ns	1.217 ns	1.016 ns
HistogramHotPath	50	69.81 ns	2.435 ns	7.180 ns
HistogramHotPath	1000	358.74 ns	6.944 ns	14.340 ns

from mic-max:hist-binary - shows slightly slower performance for small bound count and great speedup for large bound counts.

Method	BoundCount	Mean	Error	StdDev
HistogramHotPath	10	57.19 ns	1.139 ns	1.481 ns
HistogramHotPath	49	70.11 ns	1.103 ns	0.978 ns
HistogramHotPath	50	71.75 ns	1.450 ns	1.286 ns
HistogramHotPath	1000	95.72 ns	1.932 ns	2.644 ns

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

Appropriate CHANGELOG.md updated for non-trivial changes
Design discussion issue #
Changes in public API reviewed

Additional Notes

To find the point at which binary search would be faster I performed only the HistogramHotPath benchmark with 2 similar BoundCount values, one above and one below the threshold value. I continued until the time taken matched for both linear and binary search which happened to be at an array size of ~50 in this scenario.

Method	BoundCount	Mean	Error	StdDev
HistogramHotPath	49	68.81 ns	1.356 ns	1.268 ns
HistogramHotPath	50	68.46 ns	0.973 ns	0.910 ns

codecov · 2022-05-04T23:00:27Z

Codecov Report

Merging #3252 (2bb1212) into main (db0918e) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3252      +/-   ##
==========================================
- Coverage   87.10%   87.10%   -0.01%     
==========================================
  Files         280      280              
  Lines       10081    10112      +31     
==========================================
+ Hits         8781     8808      +27     
- Misses       1300     1304       +4

Impacted Files	Coverage Δ
src/OpenTelemetry/Metrics/HistogramBuckets.cs	`100.00% <100.00%> (ø)`
src/OpenTelemetry/Metrics/MetricPoint.cs	`82.99% <100.00%> (-0.12%)`	⬇️
src/OpenTelemetry/ProviderExtensions.cs	`81.81% <0.00%> (-9.10%)`	⬇️
...Telemetry/Metrics/PeriodicExportingMetricReader.cs	`72.72% <0.00%> (-5.46%)`	⬇️
src/OpenTelemetry/Logs/Pool/LogRecordSharedPool.cs	`97.36% <0.00%> (-2.64%)`	⬇️
src/OpenTelemetry/Logs/OpenTelemetryLogger.cs	`86.66% <0.00%> (-2.23%)`	⬇️
....Prometheus.HttpListener/PrometheusHttpListener.cs	`81.33% <0.00%> (-1.34%)`	⬇️
...tation/OpenTelemetryProtocolExporterEventSource.cs	`85.00% <0.00%> (+10.00%)`	⬆️
...Propagators/OpenTelemetryPropagatorsEventSource.cs	`100.00% <0.00%> (+12.50%)`	⬆️

github-actions · 2022-05-12T03:31:04Z

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

Binary search for large bucket count histograms Update CHANGELOG.md netcoreapp3.1 was complaining ci rerun ci rerun Update MetricTestData.cs use 400 buckets

src/OpenTelemetry/Metrics/MetricPoint.cs

src/OpenTelemetry/Metrics/Metric.cs

- Remove conversion to float from `FindHistogramBucketIndexBinary` - Make `DefaultHistogramCountForBinarySearch` a `const` instead of a `static readonly`

…y-dotnet into hist-binary

src/OpenTelemetry/Metrics/MetricPoint.cs

github-actions · 2022-05-25T03:34:20Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@CodeBlanch

Thanks to @CodeBlanch !!

github-actions · 2022-07-23T03:40:32Z

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

…y-dotnet into hist-binary

src/OpenTelemetry/Metrics/HistogramBuckets.cs

xiang17 · 2022-07-27T20:55:05Z

I also had a question on how to evaluate whether it's worth the preprocessing time. There needs to be a balance. If we preprocess O(n) just for optimizing a couple of queries which has an O(log(n)) complexity for 10% improvement, it might not be worth it. So I think it's crucial to have an idea on (1) how often the query happens and (2) how long the preprocessing takes.

The issue #3248 mentioned that some customers like to have faster search, which makes sense. But is it worth to add the O(n) memory and CPU burden from the preprocessing in construction of all the MetricPoint instances to all customers?

…y-dotnet into hist-binary

test/Benchmarks/Metrics/HistogramBenchmarks.cs

github-actions · 2022-08-17T03:50:48Z

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

github-actions · 2022-08-24T03:58:38Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

cijothomas · 2022-09-02T17:33:43Z

test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs

@@ -104,6 +104,46 @@ public void HistogramDistributeToAllBucketsCustom()
            Assert.Equal(boundaries.Length + 1, actualCount);
        }

+        [Fact]
+        public void HistogramBinaryBucketTest()


I'd suggest to add e2e test for Histogram targeting the edges (default bounds, bound.length >50 etc.)

alanwest

LGTM. Merging, to unblock @mic-max's min-max work #2735.

mic-max added 4 commits May 4, 2022 15:35

Binary search for large bucket count histograms

a69121c

Update CHANGELOG.md

6e446cd

netcoreapp3.1 was complaining

e420a15

ci rerun

e8913b2

mic-max added 2 commits May 4, 2022 16:35

ci rerun

4ade35a

Update MetricTestData.cs

52e2c43

github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label May 12, 2022

use 400 buckets

f7e88af

mic-max marked this pull request as ready for review May 12, 2022 19:18

mic-max requested a review from a team May 12, 2022 19:18

mic-max added 2 commits May 12, 2022 12:49

Update ExplicitBucketHistogramConfiguration.cs

51d7219

Binary search for large bucket count histograms Update CHANGELOG.md netcoreapp3.1 was complaining ci rerun ci rerun Update MetricTestData.cs use 400 buckets

separate nan part

1bb61e9

mic-max mentioned this pull request May 12, 2022

Histogram handle of double.NaN bounds and values #3279

Closed

3 tasks

reyang reviewed May 12, 2022

View reviewed changes

src/OpenTelemetry/Metrics/MetricPoint.cs Outdated Show resolved Hide resolved

utpilla reviewed May 12, 2022

View reviewed changes

src/OpenTelemetry/Metrics/Metric.cs Outdated Show resolved Hide resolved

mic-max added 2 commits May 12, 2022 15:12

Address PR comments

685b46e

- Remove conversion to float from `FindHistogramBucketIndexBinary` - Make `DefaultHistogramCountForBinarySearch` a `const` instead of a `static readonly`

Merge branch 'hist-binary' of https://github.com/mic-max/opentelemetr…

773fefd

…y-dotnet into hist-binary

mic-max marked this pull request as draft May 12, 2022 23:05

mic-max added 2 commits May 12, 2022 16:39

update to 140

de60b0e

remove double.nan from invalid hist bounds

020c42e

mic-max marked this pull request as ready for review May 12, 2022 23:59

CodeBlanch reviewed May 13, 2022

View reviewed changes

src/OpenTelemetry/Metrics/MetricPoint.cs Outdated Show resolved Hide resolved

mic-max marked this pull request as draft May 17, 2022 21:32

github-actions bot closed this May 25, 2022

mic-max added 3 commits May 25, 2022 15:27

Refactor and perf to histogram bucket index find

daf1a80

Thanks to @CodeBlanch !!

fine tune bound limit to switch to binary search

5bc4998

included benchmark results in comment

e84127a

mic-max and others added 2 commits July 12, 2022 14:44

Merge branch 'main' into hist-binary

4fe19f9

Merge branch 'main' into hist-binary

4d97b4e

github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Jul 23, 2022

mic-max added 3 commits July 26, 2022 11:04

Merge branch 'main' into hist-binary

8307bf8

CI

3b2f762

Merge branch 'hist-binary' of https://github.com/mic-max/opentelemetr…

2030736

…y-dotnet into hist-binary

xiang17 reviewed Jul 27, 2022

View reviewed changes

src/OpenTelemetry/Metrics/HistogramBuckets.cs Show resolved Hide resolved

Merge branch 'main' into hist-binary

42b1def

github-actions bot removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Jul 28, 2022

utpilla and others added 2 commits July 28, 2022 10:24

Merge branch 'main' into hist-binary

adb6945

Merge branch 'hist-binary' of https://github.com/mic-max/opentelemetr…

90a95e6

…y-dotnet into hist-binary

reyang reviewed Aug 2, 2022

View reviewed changes

test/Benchmarks/Metrics/HistogramBenchmarks.cs Show resolved Hide resolved

mic-max added 2 commits August 3, 2022 11:03

Merge branch 'main' into hist-binary

21de441

Merge branch 'main' into hist-binary

7806dbc

github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Aug 17, 2022

github-actions bot closed this Aug 24, 2022

utpilla reopened this Aug 24, 2022

github-actions bot removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Aug 25, 2022

mic-max added 2 commits August 25, 2022 10:00

Merge branch 'main' into hist-binary

e7212ac

Merge branch 'main' into hist-binary

9a3025c

cijothomas reviewed Sep 2, 2022

View reviewed changes

cijothomas approved these changes Sep 2, 2022

View reviewed changes

Merge branch 'main' into hist-binary

2bb1212

alanwest approved these changes Sep 2, 2022

View reviewed changes

alanwest merged commit 9c3d1b1 into open-telemetry:main Sep 2, 2022

mic-max deleted the hist-binary branch September 2, 2022 20:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Binary search for large bucket count histograms #3252

Binary search for large bucket count histograms #3252

mic-max commented May 4, 2022 •

edited

Loading

codecov bot commented May 4, 2022 •

edited

Loading

github-actions bot commented May 12, 2022

github-actions bot commented May 25, 2022

github-actions bot commented Jul 23, 2022

xiang17 commented Jul 27, 2022

github-actions bot commented Aug 17, 2022

github-actions bot commented Aug 24, 2022

cijothomas Sep 2, 2022

alanwest left a comment

Binary search for large bucket count histograms #3252

Binary search for large bucket count histograms #3252

Conversation

mic-max commented May 4, 2022 • edited Loading

Changes

Additional Notes

codecov bot commented May 4, 2022 • edited Loading

Codecov Report

github-actions bot commented May 12, 2022

github-actions bot commented May 25, 2022

github-actions bot commented Jul 23, 2022

xiang17 commented Jul 27, 2022

github-actions bot commented Aug 17, 2022

github-actions bot commented Aug 24, 2022

cijothomas Sep 2, 2022

Choose a reason for hiding this comment

alanwest left a comment

Choose a reason for hiding this comment

mic-max commented May 4, 2022 •

edited

Loading

codecov bot commented May 4, 2022 •

edited

Loading