From 5d0a771b8b929edab4dfe6405084b758b8bfa821 Mon Sep 17 00:00:00 2001 From: Ruslan Nigmatullin Date: Thu, 23 Jan 2025 03:00:42 -0800 Subject: [PATCH] deltatocumulative: Fix selection of the target scale for exponential histograms (#37432) #### Description While addressing comments a bug was added to the logic of calculating of the desired scale and it slipped through tests. Fix the bug (use `min` instead of `max`) and update tests to avoid regressions in the future. #### Link to tracking issue Fixes #37416 #### Testing Update tests to separately cover positive and negative buckets. #### Documentation n/a --- .chloggen/expo-histogram-fix-downscaling.yaml | 27 ++++++++++++ .../internal/data/add.go | 2 +- .../internal/data/expo_test.go | 42 +++++++++++++++---- 3 files changed, 61 insertions(+), 10 deletions(-) create mode 100644 .chloggen/expo-histogram-fix-downscaling.yaml diff --git a/.chloggen/expo-histogram-fix-downscaling.yaml b/.chloggen/expo-histogram-fix-downscaling.yaml new file mode 100644 index 000000000000..0e4e89e0ea36 --- /dev/null +++ b/.chloggen/expo-histogram-fix-downscaling.yaml @@ -0,0 +1,27 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: bug_fix + +# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) +component: deltatocumulativeprocessor + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: In order to cap number of histogram buckets take the min of desired scale across negative and positive buckets instead of the max + +# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. +issues: [37416] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: + +# If your change doesn't affect end users or the exported elements of any package, +# you should instead start your pull request title with [chore] or use the "Skip Changelog" label. +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [] diff --git a/processor/deltatocumulativeprocessor/internal/data/add.go b/processor/deltatocumulativeprocessor/internal/data/add.go index 13b9a8151106..c1a1ee6ad8f7 100644 --- a/processor/deltatocumulativeprocessor/internal/data/add.go +++ b/processor/deltatocumulativeprocessor/internal/data/add.go @@ -80,7 +80,7 @@ func (dp ExpHistogram) Add(in ExpHistogram) ExpHistogram { // Downscale if an expected number of buckets after the merge is too large. from := expo.Scale(dp.Scale()) - to := max( + to := min( expo.Limit(maxBuckets, from, dp.Positive(), in.Positive()), expo.Limit(maxBuckets, from, dp.Negative(), in.Negative()), ) diff --git a/processor/deltatocumulativeprocessor/internal/data/expo_test.go b/processor/deltatocumulativeprocessor/internal/data/expo_test.go index 970eda2b67c7..768f1f39b1cf 100644 --- a/processor/deltatocumulativeprocessor/internal/data/expo_test.go +++ b/processor/deltatocumulativeprocessor/internal/data/expo_test.go @@ -27,10 +27,11 @@ func TestExpoAdd(t *testing.T) { defer func() { maxBuckets = prevMaxBuckets }() cases := []struct { - name string - dp, in expdp - want expdp - flip bool + name string + dp, in expdp + want expdp + flip bool + alsoTryEachSign bool }{{ name: "noop", dp: expdp{PosNeg: bins{0, 0, 0, 0, 0, 0, 0, 0}.Into(), Count: 0}, @@ -108,6 +109,7 @@ func TestExpoAdd(t *testing.T) { PosNeg: bins{3, 3, 3, 3, 3, 3, 3, 3}.Into(), Count: 24, }, + alsoTryEachSign: true, }, { name: "scale/downscale_once_exceeds_limit", dp: expdp{ @@ -125,6 +127,7 @@ func TestExpoAdd(t *testing.T) { PosNeg: rawbs([]uint64{2, 2, 2, 6, 4, 4, 4}, 0), Count: 24, }, + alsoTryEachSign: true, }, { name: "scale/downscale_multiple_times_until_within_limit", dp: expdp{ @@ -142,6 +145,7 @@ func TestExpoAdd(t *testing.T) { PosNeg: rawbs([]uint64{2, 4, 2, 4, 8, 4}, -2), Count: 24, }, + alsoTryEachSign: true, }, { name: "scale/ignore_leading_trailing_zeros_in_bucket_count", dp: expdp{ @@ -159,6 +163,7 @@ func TestExpoAdd(t *testing.T) { PosNeg: rawbs([]uint64{1, 7, 7, 4, 3, 2, 2}, 0), Count: 26, }, + alsoTryEachSign: true, }, { name: "scale/downscale_with_leading_trailing_zeros", dp: expdp{ @@ -176,17 +181,18 @@ func TestExpoAdd(t *testing.T) { PosNeg: rawbs([]uint64{11, 11, 0, 0, 12, 12}, -1), Count: 46, }, + alsoTryEachSign: true, }} for _, cs := range cases { - run := func(dp, in expdp) func(t *testing.T) { + run := func(dp, in, want expdp) func(t *testing.T) { return func(t *testing.T) { is := datatest.New(t) var ( dp = ExpHistogram{dp.Into()} in = ExpHistogram{in.Into()} - want = ExpHistogram{cs.want.Into()} + want = ExpHistogram{want.Into()} ) dp.SetTimestamp(0) @@ -199,14 +205,32 @@ func TestExpoAdd(t *testing.T) { } if cs.flip { - t.Run(cs.name+"-dp", run(cs.dp, cs.in)) - t.Run(cs.name+"-in", run(cs.in, cs.dp)) + t.Run(cs.name+"-dp", run(cs.dp, cs.in, cs.want)) + t.Run(cs.name+"-in", run(cs.in, cs.dp, cs.want)) continue } - t.Run(cs.name, run(cs.dp, cs.in)) + if cs.alsoTryEachSign { + t.Run(cs.name+"-pos", run(clonePosExpdp(cs.dp), clonePosExpdp(cs.in), clonePosExpdp(cs.want))) + t.Run(cs.name+"-neg", run(cloneNegExpdp(cs.dp), cloneNegExpdp(cs.in), cloneNegExpdp(cs.want))) + } + t.Run(cs.name, run(cs.dp, cs.in, cs.want)) } } +func cloneNegExpdp(dp expotest.Histogram) expotest.Histogram { + dp.Neg = pmetric.NewExponentialHistogramDataPointBuckets() + dp.PosNeg.CopyTo(dp.Neg) + dp.PosNeg = expo.Buckets{} + return dp +} + +func clonePosExpdp(dp expotest.Histogram) expotest.Histogram { + dp.Pos = pmetric.NewExponentialHistogramDataPointBuckets() + dp.PosNeg.CopyTo(dp.Pos) + dp.PosNeg = expo.Buckets{} + return dp +} + func rawbs(data []uint64, offset int32) expo.Buckets { bs := pmetric.NewExponentialHistogramDataPointBuckets() bs.BucketCounts().FromRaw(data)