[OpenTelemetry] Move BucketLookup tree to AggregatorStore #6715
Conversation
Reduces memory usage by caching HistogramBuckets.BucketLookupNode
github-actions
bot
added
the
pkg:OpenTelemetry
iliar-turdushev
changed the title
iliar-turdushev
changed the title
iliar-turdushev
changed the title
martincostello
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6715 +/- ##
==========================================
- Coverage 86.87% 86.73% -0.14%
==========================================
Files 258 259 +1
Lines 12057 12066 +9
==========================================
- Hits 10474 10465 -9
- Misses 1583 1601 +18
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Co-authored-by: Martin Costello <[email protected]>
|
@martincostello @Kielek @cijothomas Codecov says that one changed line is not covered with tests, this is the line (you need to expand the |
|
@martincostello @Kielek @cijothomas @rajkumar-rangaraj Gentle ping on this comment. Can we proceed with the PR and merge it? Thank you. |
rajkumar-rangaraj
left a comment
rajkumar-rangaraj
left a comment
There was a problem hiding this comment.
LGTM
@iliar-turdushev Thanks for your contributions!
There was a problem hiding this comment.
Pull request overview
This PR improves the performance and memory efficiency of OpenTelemetry's histogram metrics by centralizing the BucketLookup tree structure. The bucket lookup logic (binary search tree and linear search) has been moved from individual MetricPoint instances to a shared HistogramExplicitBounds class stored in AggregatorStore, reducing memory consumption from ~750-800MB to ~170-180MB while maintaining or improving performance.
Key Changes
- Created new
HistogramExplicitBoundsclass to encapsulate bucket boundary lookup logic and BST construction - Modified
AggregatorStoreto maintain a single sharedHistogramExplicitBoundsinstance for all metric points - Refactored
HistogramBucketsto delegate bucket index lookups toHistogramExplicitBounds
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/OpenTelemetry/Metrics/HistogramExplicitBounds.cs |
New class encapsulating bucket boundary lookup with binary search tree for ≥50 boundaries |
src/OpenTelemetry/Metrics/AggregatorStore.cs |
Centralizes HistogramExplicitBounds instance and passes it to all MetricPoint creations |
src/OpenTelemetry/Metrics/MetricPoint/MetricPoint.cs |
Updated constructor parameter from double[] to HistogramExplicitBounds |
src/OpenTelemetry/Metrics/MetricPoint/HistogramBuckets.cs |
Removed bucket lookup logic and delegated to HistogramExplicitBounds |
test/OpenTelemetry.Tests/Metrics/AggregatorTests.cs |
Updated tests to instantiate HistogramExplicitBounds objects |
src/OpenTelemetry/CHANGELOG.md |
Documented performance and memory improvements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (var i = 0; i < bounds.Length; i++) | ||
| { | ||
| if (double.IsNegativeInfinity(bounds[i]) || double.IsPositiveInfinity(bounds[i])) | ||
| { | ||
| return bounds | ||
| .Where(b => !double.IsNegativeInfinity(b) && !double.IsPositiveInfinity(b)) | ||
| .ToArray(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The CleanUpInfinitiesFromExplicitBounds method performs two passes over the array when infinities are found: first to detect them, then LINQ operations (Where + ToArray) to filter them. For better performance, consider collecting filtered values in a single pass using a List or ArrayPool, especially since this runs in the hot path during construction.
| for (var i = 0; i < bounds.Length; i++) | |
| { | |
| if (double.IsNegativeInfinity(bounds[i]) || double.IsPositiveInfinity(bounds[i])) | |
| { | |
| return bounds | |
| .Where(b => !double.IsNegativeInfinity(b) && !double.IsPositiveInfinity(b)) | |
| .ToArray(); | |
| } | |
| } | |
| List<double>? filtered = null; | |
| for (var i = 0; i < bounds.Length; i++) | |
| { | |
| var b = bounds[i]; | |
| if (double.IsNegativeInfinity(b) || double.IsPositiveInfinity(b)) | |
| { | |
| if (filtered == null) | |
| { | |
| filtered = new List<double>(bounds.Length - 1); | |
| for (var j = 0; j < i; j++) | |
| { | |
| filtered.Add(bounds[j]); | |
| } | |
| } | |
| // skip this value | |
| } | |
| else | |
| { | |
| if (filtered != null) | |
| { | |
| filtered.Add(b); | |
| } | |
| } | |
| } | |
| if (filtered != null) | |
| { | |
| return filtered.ToArray(); | |
| } |
Kielek
left a comment
Kielek
left a comment
There was a problem hiding this comment.
LGTM.
@iliar-turdushev, I left one comment opened from Copilot for your consideration. If you thin that it will be beneficial, please consider creating follow up PR,
Adding to merge queue.
5 participants
Fixes #5976
Changes
The PR moves
BucketLookuptree toAggregateStore. A logic that buildsBucketLookuptree and decides whether a linear or binary search should be used has been moved to a class calledHistogramExplicitBounds.Benchmark results
HistogramBenchmarks measure performance of histograms with explicit bucket boundaries. Below are sample benchmark results comparing performance "before" and "after".
BEFORE:
AFTER:
The benchmark tests were run several times and the results were the same as above:
BoundsCount) less than 50 the performance "before" and "after" is more or less the same;BoundsCount>= 50 the performance "after" is better than "before", and the higher the cardinality of a metric the better performance.Memory consumption
A test described in this comment was used to compare memory consumption "before" and "after":
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changesChanges in public API reviewed (if applicable) -NOT APPLICABLE