Use sync.Map for exponential histogram aggregations - #8077
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8077 +/- ##
=======================================
- Coverage 84.0% 84.0% -0.1%
=======================================
Files 329 329
Lines 26068 26219 +151
=======================================
+ Hits 21918 22039 +121
- Misses 3769 3787 +18
- Partials 381 393 +12
🚀 New features to boost your workflow:
|
4a958eb to
fc3a34b
Compare
3054c75 to
d5f3996
Compare
|
One part if this that is challenging to resolve is dealing with underflow for cumulative metrics. The current design mirrors how the histogram implementation works: Collection swaps hot and cold, reads the cold, and then merges the cold back into the hot point. The issue comes during the merge process. It is possible that an observation made to the hot point should underflow, but we don't find that out until we try to merge the cold point into the hot one. For example: attrs := attribute.NewSet()
maxSize := 2
h := newExpoHistogram(maxsize, ...)
h.measure(ctx, math.MaxFloat64, attrs, ...)
go h.collect(...)
h.measure(ctx, math.SmallestNonzeroFloat64, attrs)
// assume collect() finishes after measure, and tries to merge
// an exp histogram with math.MaxFloat64 into an exp histogram
// with math.SmallestNonzeroFloat64. This will underflow, but we
// can't remove the underflowed measurement after it has been
// aggregated.This is an extremely rare case: Underflow is only possible with maxSize <= 2, and when making measurements where one is 2^1024 times greater than the other. Some options i've come up with to deal with it:
I'm planning to implement the proper fix (option 1.i), but I wanted to document this in-case it comes up later. Option 2.i is also appealing given how extremely rare this should be in-practice. |
fa32a82 to
906aa6e
Compare
Some small testing improvements forked from #8077. This also fixes a flake where the order in which sums are added can change the resulting sum. Use assertSumEqual to handle this similar to other places in the test. Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
906aa6e to
5ad698a
Compare
|
I had an idea for how to solve the underflow: Just don't optimize the locking for exponential histograms of size 1 or 2! Use a standard lock around everything. Give that is an odd case, we don't need to optimize for amazing performance there. |
e387744 to
7a6afcc
Compare
|
Finally cleaned this up. Ready for review, but no rush. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the exponential histogram aggregators in the metric SDK to reduce contention by replacing a single mutex-protected map with sync.Map-based structures and hot/cold swapping, aligning the implementation approach with the earlier fixed-bucket histogram optimization work.
Changes:
- Replaces delta exponential histogram aggregation’s single locked map with a double-buffered hot/cold
[2]limitedSyncMapcoordinated byhotColdWaitGroup. - Introduces a new cumulative exponential histogram aggregator that keeps a stable
limitedSyncMapof series keys while swapping per-series hot/cold delta points and merging into a persistent cumulative point. - Adds/updates tests to account for the new aggregator types and includes coverage for scale-underflow behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| sdk/metric/internal/aggregate/exponential_histogram.go | Reworks delta + cumulative exponential histogram implementations to use limitedSyncMap and hot/cold synchronization; adds per-point merge and bucket merge helpers. |
| sdk/metric/internal/aggregate/exponential_histogram_test.go | Updates tests for new constructors/collection paths and adds underflow test coverage for both delta and cumulative variants. |
| sdk/metric/internal/aggregate/aggregate.go | Updates the Builder to construct delta vs cumulative exponential histogram aggregators explicitly and wire both to collect. |
512090c to
579d74c
Compare
…rflowSet in limitedSyncMap (open-telemetry#8077)
MrAlias
left a comment
There was a problem hiding this comment.
The double-buffered design is headed in the right direction. There is one cumulative exemplar collection boundary to tighten up before this lands.
80403e9 to
b65a333
Compare
…xponential histograms
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…ive exemplar reservoir (open-telemetry#8077)
…rflowSet in limitedSyncMap (open-telemetry#8077)
b65a333 to
cb1ec3e
Compare
cb1ec3e to
8057bcb
Compare
|
Converting to a draft until #8711 is resolved. |
Part of #7796
This applies the same approach as I did for fixed-bucket histograms (#7474) to exponential histograms.
Changes
mapprotected by async.Mutex. Collection blocked writers while it iterated over the map and cleared it.[2]limitedSyncMap(a specializedsync.Mapwith size limits). Writers record to a "hot" map, whilecollectreads from a "cold" map. A customhotColdWaitGrouporchestrates wait-free swapping of these maps, ensuring collection does not block active writers.limitedSyncMapmapping to acumulativePointstruct. This struct contains two active data points (hot and cold) and one persistent cumulative point. Writers use the hot point, andcollectswaps them and merges the cold point into the cumulative point.measure) and collection (collect).sync.Maphandles concurrent lookups, and a mutex is only acquired at the individual data point level (expoHistogramDataPoint) when recording a value that requires bucket expansion or downscaling.This does not make the buckets concurrent-safe. That will be done in subsequent PRs.
This does not try to prevent concurrent underflow when <=2 maxSize is used. We will just keep the lock around buckets for that case. It will have worse performance, but that's OK.