From 4ee29ff44807f3ec98a6b2db382d37357f3677c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Fri, 7 Jul 2023 08:18:31 +0200 Subject: [PATCH 01/16] promremotewrite: Fix histogram unit test observation count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: György Krajcsovits --- pkg/translator/prometheusremotewrite/histograms_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/translator/prometheusremotewrite/histograms_test.go b/pkg/translator/prometheusremotewrite/histograms_test.go index a5b9f47194eb..b23d7ed54438 100644 --- a/pkg/translator/prometheusremotewrite/histograms_test.go +++ b/pkg/translator/prometheusremotewrite/histograms_test.go @@ -141,7 +141,7 @@ func TestExponentialToNativeHistogram(t *testing.T) { pt := pmetric.NewExponentialHistogramDataPoint() pt.SetStartTimestamp(pcommon.NewTimestampFromTime(time.UnixMilli(100))) pt.SetTimestamp(pcommon.NewTimestampFromTime(time.UnixMilli(500))) - pt.SetCount(2) + pt.SetCount(4) pt.SetSum(10.1) pt.SetScale(1) pt.SetZeroCount(1) @@ -156,7 +156,7 @@ func TestExponentialToNativeHistogram(t *testing.T) { }, wantNativeHist: func() prompb.Histogram { return prompb.Histogram{ - Count: &prompb.Histogram_CountInt{CountInt: 2}, + Count: &prompb.Histogram_CountInt{CountInt: 4}, Sum: 10.1, Schema: 1, ZeroThreshold: defaultZeroThreshold, @@ -176,7 +176,7 @@ func TestExponentialToNativeHistogram(t *testing.T) { pt.SetStartTimestamp(pcommon.NewTimestampFromTime(time.UnixMilli(100))) pt.SetTimestamp(pcommon.NewTimestampFromTime(time.UnixMilli(500))) - pt.SetCount(2) + pt.SetCount(4) pt.SetScale(1) pt.SetZeroCount(1) @@ -190,7 +190,7 @@ func TestExponentialToNativeHistogram(t *testing.T) { }, wantNativeHist: func() prompb.Histogram { return prompb.Histogram{ - Count: &prompb.Histogram_CountInt{CountInt: 2}, + Count: &prompb.Histogram_CountInt{CountInt: 4}, Schema: 1, ZeroThreshold: defaultZeroThreshold, ZeroCount: &prompb.Histogram_ZeroCountInt{ZeroCountInt: 1}, From bf70fc80c177df1bc80a97b4dc189cd6ffec00d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Fri, 7 Jul 2023 13:31:54 +0200 Subject: [PATCH 02/16] Promremotewrite: test histogram downscaling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add unit test to check that upscaling is still rejected and downscaling happens above scale 8. Test edges 8 and 9. Further detailed tests to follow in TestConvertBucketsLayout. Signed-off-by: György Krajcsovits --- .../prometheusremotewrite/histograms_test.go | 68 ++++++++++++++++++- 1 file changed, 66 insertions(+), 2 deletions(-) diff --git a/pkg/translator/prometheusremotewrite/histograms_test.go b/pkg/translator/prometheusremotewrite/histograms_test.go index b23d7ed54438..6020d1ffda5b 100644 --- a/pkg/translator/prometheusremotewrite/histograms_test.go +++ b/pkg/translator/prometheusremotewrite/histograms_test.go @@ -203,14 +203,78 @@ func TestExponentialToNativeHistogram(t *testing.T) { }, }, { - name: "invalid scale", + name: "invalid negative scale", exponentialHist: func() pmetric.ExponentialHistogramDataPoint { pt := pmetric.NewExponentialHistogramDataPoint() pt.SetScale(-10) return pt }, wantErrMessage: "cannot convert exponential to native histogram." + - " Scale must be <= 8 and >= -4, was -10", + " Scale must be >= -4, was -10", + }, + { + name: "no downscaling at scale 8", + exponentialHist: func() pmetric.ExponentialHistogramDataPoint { + pt := pmetric.NewExponentialHistogramDataPoint() + pt.SetTimestamp(pcommon.NewTimestampFromTime(time.UnixMilli(500))) + pt.SetCount(6) + pt.SetSum(10.1) + pt.SetScale(8) + pt.SetZeroCount(1) + + pt.Positive().BucketCounts().FromRaw([]uint64{1, 1, 1}) + pt.Positive().SetOffset(1) + + pt.Negative().BucketCounts().FromRaw([]uint64{1, 1, 1}) + pt.Negative().SetOffset(2) + return pt + }, + wantNativeHist: func() prompb.Histogram { + return prompb.Histogram{ + Count: &prompb.Histogram_CountInt{CountInt: 6}, + Sum: 10.1, + Schema: 8, + ZeroThreshold: defaultZeroThreshold, + ZeroCount: &prompb.Histogram_ZeroCountInt{ZeroCountInt: 1}, + PositiveSpans: []prompb.BucketSpan{{Offset: 2, Length: 3}}, + PositiveDeltas: []int64{1, 0, 0}, // 1, 1, 1 + NegativeSpans: []prompb.BucketSpan{{Offset: 3, Length: 3}}, + NegativeDeltas: []int64{1, 0, 0}, // 1, 1, 1 + Timestamp: 500, + } + }, + }, + { + name: "downsample if scale is more than 8", + exponentialHist: func() pmetric.ExponentialHistogramDataPoint { + pt := pmetric.NewExponentialHistogramDataPoint() + pt.SetTimestamp(pcommon.NewTimestampFromTime(time.UnixMilli(500))) + pt.SetCount(6) + pt.SetSum(10.1) + pt.SetScale(9) + pt.SetZeroCount(1) + + pt.Positive().BucketCounts().FromRaw([]uint64{1, 1, 1}) + pt.Positive().SetOffset(1) + + pt.Negative().BucketCounts().FromRaw([]uint64{1, 1, 1}) + pt.Negative().SetOffset(2) + return pt + }, + wantNativeHist: func() prompb.Histogram { + return prompb.Histogram{ + Count: &prompb.Histogram_CountInt{CountInt: 6}, + Sum: 10.1, + Schema: 8, + ZeroThreshold: defaultZeroThreshold, + ZeroCount: &prompb.Histogram_ZeroCountInt{ZeroCountInt: 1}, + PositiveSpans: []prompb.BucketSpan{{Offset: 2, Length: 2}}, + PositiveDeltas: []int64{2, -1}, // 1+1, 1+0 = 2, 1 + NegativeSpans: []prompb.BucketSpan{{Offset: 2, Length: 2}}, + NegativeDeltas: []int64{1, 1}, // 0+1, 1+1 = 1, 2 + Timestamp: 500, + } + }, }, } for _, tt := range tests { From b8d41b668c6bb8f5abe87080e101136530792c6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Fri, 7 Jul 2023 13:38:03 +0200 Subject: [PATCH 03/16] Add changelog file. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: György Krajcsovits --- ...etheusremotewrite-downscale-histogram.yaml | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 .chloggen/prometheusremotewrite-downscale-histogram.yaml diff --git a/.chloggen/prometheusremotewrite-downscale-histogram.yaml b/.chloggen/prometheusremotewrite-downscale-histogram.yaml new file mode 100644 index 000000000000..1c36f4c0920a --- /dev/null +++ b/.chloggen/prometheusremotewrite-downscale-histogram.yaml @@ -0,0 +1,20 @@ +# Use this changelog template to create an entry for release notes. +# If your change doesn't affect end users, such as a test fix or a tooling change, +# you should instead start your pull request title with [chore] or use the "Skip Changelog" label. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: enhancement + +# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) +component: pkg/translator/prometheusremotewrite + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Downscale exponential histograms to fit prometheus native histograms. + +# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. +issues: [1756] + +# (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: From 7f2d6ae87b928ff1f28716d2ba062e479d7d2f62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Fri, 7 Jul 2023 16:43:02 +0200 Subject: [PATCH 04/16] Update convertBucketsLayout and its unit tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a new parameter that will be used to tell the function how many buckets to merge when converting the bucket layouts. Not use yet. Add disabled test cases. Signed-off-by: György Krajcsovits --- .../prometheusremotewrite/histograms.go | 8 +- .../prometheusremotewrite/histograms_test.go | 331 +++++++++++++++--- 2 files changed, 294 insertions(+), 45 deletions(-) diff --git a/pkg/translator/prometheusremotewrite/histograms.go b/pkg/translator/prometheusremotewrite/histograms.go index 293d318a3c58..34ce65729191 100644 --- a/pkg/translator/prometheusremotewrite/histograms.go +++ b/pkg/translator/prometheusremotewrite/histograms.go @@ -65,8 +65,8 @@ func exponentialToNativeHistogram(p pmetric.ExponentialHistogramDataPoint) (prom // TODO: downscale to 8 if scale > 8 } - pSpans, pDeltas := convertBucketsLayout(p.Positive()) - nSpans, nDeltas := convertBucketsLayout(p.Negative()) + pSpans, pDeltas := convertBucketsLayout(p.Positive(), 1) + nSpans, nDeltas := convertBucketsLayout(p.Negative(), 1) h := prompb.Histogram{ Schema: scale, @@ -104,7 +104,9 @@ func exponentialToNativeHistogram(p pmetric.ExponentialHistogramDataPoint) (prom // The bucket indexes conversion was adjusted, since OTel exp. histogram bucket // index 0 corresponds to the range (1, base] while Prometheus bucket index 0 // to the range (base 1]. -func convertBucketsLayout(buckets pmetric.ExponentialHistogramDataPointBuckets) ([]prompb.BucketSpan, []int64) { +// +// scaleMerge is the number of buckets to merge into a single bucket - must be power of 2 +func convertBucketsLayout(buckets pmetric.ExponentialHistogramDataPointBuckets, _ int32) ([]prompb.BucketSpan, []int64) { bucketCounts := buckets.BucketCounts() if bucketCounts.Len() == 0 { return nil, nil diff --git a/pkg/translator/prometheusremotewrite/histograms_test.go b/pkg/translator/prometheusremotewrite/histograms_test.go index 6020d1ffda5b..2cdf73811009 100644 --- a/pkg/translator/prometheusremotewrite/histograms_test.go +++ b/pkg/translator/prometheusremotewrite/histograms_test.go @@ -4,6 +4,7 @@ package prometheusremotewrite import ( + "fmt" "testing" "time" @@ -17,12 +18,16 @@ import ( prometheustranslator "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/prometheus" ) +type expectedBucketLayout struct { + wantSpans []prompb.BucketSpan + wantDeltas []int64 +} + func TestConvertBucketsLayout(t *testing.T) { tests := []struct { name string buckets func() pmetric.ExponentialHistogramDataPointBuckets - wantSpans []prompb.BucketSpan - wantDeltas []int64 + wantLayout map[int32]expectedBucketLayout }{ { name: "zero offset", @@ -32,13 +37,75 @@ func TestConvertBucketsLayout(t *testing.T) { b.BucketCounts().FromRaw([]uint64{4, 3, 2, 1}) return b }, - wantSpans: []prompb.BucketSpan{ - { - Offset: 1, - Length: 4, + wantLayout: map[int32]expectedBucketLayout{ + 1: { + wantSpans: []prompb.BucketSpan{ + { + Offset: 1, + Length: 4, + }, + }, + wantDeltas: []int64{4, -1, -1, -1}, + }, + 2: { + wantSpans: []prompb.BucketSpan{ + { + Offset: 1, + Length: 2, + }, + }, + // 4+3, 2+1 = 7, 3 =delta= 7, -4 + wantDeltas: []int64{4, -4}, + }, + 4: { + wantSpans: []prompb.BucketSpan{ + { + Offset: 1, + Length: 1, + }, + }, + // 4+3+2+1 = 10 =delta= 10 + wantDeltas: []int64{10}, + }, + }, + }, + { + name: "offset 1", + buckets: func() pmetric.ExponentialHistogramDataPointBuckets { + b := pmetric.NewExponentialHistogramDataPointBuckets() + b.SetOffset(1) + b.BucketCounts().FromRaw([]uint64{4, 3, 2, 1}) + return b + }, + wantLayout: map[int32]expectedBucketLayout{ + 1: { + wantSpans: []prompb.BucketSpan{ + { + Offset: 2, + Length: 4, + }, + }, + wantDeltas: []int64{4, -1, -1, -1}, + }, + 2: { + wantSpans: []prompb.BucketSpan{ + { + Offset: 1, + Length: 3, + }, + }, + wantDeltas: []int64{4, 1, -4}, // 0+4, 3+2, 1+0 = 4, 5, 1 + }, + 4: { + wantSpans: []prompb.BucketSpan{ + { + Offset: 1, + Length: 2, + }, + }, + wantDeltas: []int64{9, -8}, // 0+4+3+2, 1+0+0+0 = 9, 1 }, }, - wantDeltas: []int64{4, -1, -1, -1}, }, { name: "positive offset", @@ -48,17 +115,103 @@ func TestConvertBucketsLayout(t *testing.T) { b.BucketCounts().FromRaw([]uint64{4, 2, 0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}) return b }, - wantSpans: []prompb.BucketSpan{ - { - Offset: 5, - Length: 4, + wantLayout: map[int32]expectedBucketLayout{ + 1: { + wantSpans: []prompb.BucketSpan{ + { + Offset: 5, + Length: 4, + }, + { + Offset: 12, + Length: 1, + }, + }, + wantDeltas: []int64{4, -2, -2, 2, -1}, + }, + 2: { + wantSpans: []prompb.BucketSpan{ + { + Offset: 4, + Length: 2, + }, + { + Offset: 6, + Length: 1, + }, + }, + // Downscale: + // 4+2, 0+2, 0+0, 0+0, 0+0, 0+0, 0+0, 0+0, 1+0 = 6, 2, 0, 0, 0, 0, 0, 0, 1 + wantDeltas: []int64{6, -4, -1}, }, - { - Offset: 12, - Length: 1, + 4: { + wantSpans: []prompb.BucketSpan{ + { + Offset: 4, + Length: 1, + }, + { + Offset: 3, + Length: 1, + }, + }, + // Downscale: + // 4+2+0+2, 0+0+0+0, 0+0+0+0, 0+0+0+0, 1+0+0+0 = 8, 0, 0, 0, 1 + // Check from sclaing from previous: 6+2, 0+0, 0+0, 0+0, 1+0 = 8, 0, 0, 0, 1 + wantDeltas: []int64{8, -7}, + }, + }, + }, + { + name: "scaledown merges spans", + buckets: func() pmetric.ExponentialHistogramDataPointBuckets { + b := pmetric.NewExponentialHistogramDataPointBuckets() + b.SetOffset(4) + b.BucketCounts().FromRaw([]uint64{4, 2, 0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 1}) + return b + }, + wantLayout: map[int32]expectedBucketLayout{ + 1: { + wantSpans: []prompb.BucketSpan{ + { + Offset: 5, + Length: 4, + }, + { + Offset: 8, + Length: 1, + }, + }, + wantDeltas: []int64{4, -2, -2, 2, -1}, + }, + 2: { + wantSpans: []prompb.BucketSpan{ + { + Offset: 4, + Length: 2, + }, + { + Offset: 4, + Length: 1, + }, + }, + // Downscale: + // 4+2, 0+2, 0+0, 0+0, 0+0, 0+0, 1+0 = 6, 2, 0, 0, 0, 0, 1 + wantDeltas: []int64{6, -4, -1}, + }, + 4: { + wantSpans: []prompb.BucketSpan{ + { + Offset: 4, + Length: 4, + }, + }, + // Downscale: + // 4+2+0+2, 0+0+0+0, 0+0+0+0, 1+0+0+0 = 8, 0, 0, 1 + // Check from sclaing from previous: 6+2, 0+0, 0+0, 1+0 = 8, 0, 0, 1 + wantDeltas: []int64{8, -8, 0, 1}, }, }, - wantDeltas: []int64{4, -2, -2, 2, -1}, }, { name: "negative offset", @@ -68,17 +221,43 @@ func TestConvertBucketsLayout(t *testing.T) { b.BucketCounts().FromRaw([]uint64{3, 1, 0, 0, 0, 1}) return b }, - wantSpans: []prompb.BucketSpan{ - { - Offset: -1, - Length: 2, + wantLayout: map[int32]expectedBucketLayout{ + 1: { + wantSpans: []prompb.BucketSpan{ + { + Offset: -1, + Length: 2, + }, + { + Offset: 3, + Length: 1, + }, + }, + wantDeltas: []int64{3, -2, 0}, }, - { - Offset: 3, - Length: 1, + 2: { + wantSpans: []prompb.BucketSpan{ + { + Offset: -1, + Length: 3, + }, + }, + // Downscale: + // 3+1, 0+0, 0+1 = 4, 0, 1 + wantDeltas: []int64{4, -4, 1}, + }, + 4: { + wantSpans: []prompb.BucketSpan{ + { + Offset: -1, + Length: 2, + }, + }, + // Downscale: + // 0+0+3+1, 0+0+0+0 = 4, 1 + wantDeltas: []int64{4, -3}, }, }, - wantDeltas: []int64{3, -2, 0}, }, { name: "buckets with gaps of size 1", @@ -88,13 +267,39 @@ func TestConvertBucketsLayout(t *testing.T) { b.BucketCounts().FromRaw([]uint64{3, 1, 0, 1, 0, 1}) return b }, - wantSpans: []prompb.BucketSpan{ - { - Offset: -1, - Length: 6, + wantLayout: map[int32]expectedBucketLayout{ + 1: { + wantSpans: []prompb.BucketSpan{ + { + Offset: -1, + Length: 6, + }, + }, + wantDeltas: []int64{3, -2, -1, 1, -1, 1}, + }, + 2: { + wantSpans: []prompb.BucketSpan{ + { + Offset: -1, + Length: 3, + }, + }, + // Downscale: + // 3+1, 0+1, 0+1 = 4, 1, 1 + wantDeltas: []int64{4, -3, 0}, + }, + 4: { + wantSpans: []prompb.BucketSpan{ + { + Offset: -1, + Length: 2, + }, + }, + // Downscale: + // 0+0+3+1, 0+1+0+1 = 4, 2 + wantDeltas: []int64{4, -2}, }, }, - wantDeltas: []int64{3, -2, -1, 1, -1, 1}, }, { name: "buckets with gaps of size 2", @@ -104,27 +309,69 @@ func TestConvertBucketsLayout(t *testing.T) { b.BucketCounts().FromRaw([]uint64{3, 0, 0, 1, 0, 0, 1}) return b }, - wantSpans: []prompb.BucketSpan{ - { - Offset: -1, - Length: 7, + wantLayout: map[int32]expectedBucketLayout{ + 1: { + wantSpans: []prompb.BucketSpan{ + { + Offset: -1, + Length: 7, + }, + }, + wantDeltas: []int64{3, -3, 0, 1, -1, 0, 1}, + }, + 2: { + wantSpans: []prompb.BucketSpan{ + { + Offset: -1, + Length: 4, + }, + }, + // Downscale: + // 3+0, 0+1, 0+0, 0+1 = 3, 1, 0, 1 + wantDeltas: []int64{3, -2, -1, 1}, + }, + 4: { + wantSpans: []prompb.BucketSpan{ + { + Offset: -1, + Length: 3, + }, + }, + // Downscale: + // 0+0+3+0, 0+1+0+0, 1+0+0+0 = 3, 1, 1 + wantDeltas: []int64{3, -2, 0}, }, }, - wantDeltas: []int64{3, -3, 0, 1, -1, 0, 1}, }, { - name: "zero buckets", - buckets: pmetric.NewExponentialHistogramDataPointBuckets, - wantSpans: nil, - wantDeltas: nil, + name: "zero buckets", + buckets: pmetric.NewExponentialHistogramDataPointBuckets, + wantLayout: map[int32]expectedBucketLayout{ + 1: { + wantSpans: nil, + wantDeltas: nil, + }, + 2: { + wantSpans: nil, + wantDeltas: nil, + }, + 4: { + wantSpans: nil, + wantDeltas: nil, + }, + }, }, } for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - gotSpans, gotDeltas := convertBucketsLayout(tt.buckets()) - assert.Equal(t, tt.wantSpans, gotSpans) - assert.Equal(t, tt.wantDeltas, gotDeltas) - }) + for scaleDown, wantLayout := range tt.wantLayout { + if scaleDown == 1 { + t.Run(fmt.Sprintf("%s-scaleby-%d", tt.name, scaleDown), func(t *testing.T) { + gotSpans, gotDeltas := convertBucketsLayout(tt.buckets(), scaleDown) + assert.Equal(t, wantLayout.wantSpans, gotSpans) + assert.Equal(t, wantLayout.wantDeltas, gotDeltas) + }) + } + } } } From a7e0f926a7828fb1101f5490efc073663999b681 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Fri, 7 Jul 2023 17:07:03 +0200 Subject: [PATCH 05/16] Fix offsets and enable test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: György Krajcsovits --- .../prometheusremotewrite/histograms_test.go | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/pkg/translator/prometheusremotewrite/histograms_test.go b/pkg/translator/prometheusremotewrite/histograms_test.go index 2cdf73811009..94335b734bfd 100644 --- a/pkg/translator/prometheusremotewrite/histograms_test.go +++ b/pkg/translator/prometheusremotewrite/histograms_test.go @@ -132,7 +132,7 @@ func TestConvertBucketsLayout(t *testing.T) { 2: { wantSpans: []prompb.BucketSpan{ { - Offset: 4, + Offset: 3, Length: 2, }, { @@ -147,7 +147,7 @@ func TestConvertBucketsLayout(t *testing.T) { 4: { wantSpans: []prompb.BucketSpan{ { - Offset: 4, + Offset: 2, Length: 1, }, { @@ -187,7 +187,7 @@ func TestConvertBucketsLayout(t *testing.T) { 2: { wantSpans: []prompb.BucketSpan{ { - Offset: 4, + Offset: 2, Length: 2, }, { @@ -202,7 +202,7 @@ func TestConvertBucketsLayout(t *testing.T) { 4: { wantSpans: []prompb.BucketSpan{ { - Offset: 4, + Offset: 2, Length: 4, }, }, @@ -364,13 +364,11 @@ func TestConvertBucketsLayout(t *testing.T) { } for _, tt := range tests { for scaleDown, wantLayout := range tt.wantLayout { - if scaleDown == 1 { - t.Run(fmt.Sprintf("%s-scaleby-%d", tt.name, scaleDown), func(t *testing.T) { - gotSpans, gotDeltas := convertBucketsLayout(tt.buckets(), scaleDown) - assert.Equal(t, wantLayout.wantSpans, gotSpans) - assert.Equal(t, wantLayout.wantDeltas, gotDeltas) - }) - } + t.Run(fmt.Sprintf("%s-scaleby-%d", tt.name, scaleDown), func(t *testing.T) { + gotSpans, gotDeltas := convertBucketsLayout(tt.buckets(), scaleDown) + assert.Equal(t, wantLayout.wantSpans, gotSpans) + assert.Equal(t, wantLayout.wantDeltas, gotDeltas) + }) } } } @@ -515,10 +513,10 @@ func TestExponentialToNativeHistogram(t *testing.T) { Schema: 8, ZeroThreshold: defaultZeroThreshold, ZeroCount: &prompb.Histogram_ZeroCountInt{ZeroCountInt: 1}, - PositiveSpans: []prompb.BucketSpan{{Offset: 2, Length: 2}}, - PositiveDeltas: []int64{2, -1}, // 1+1, 1+0 = 2, 1 + PositiveSpans: []prompb.BucketSpan{{Offset: 1, Length: 2}}, + PositiveDeltas: []int64{1, 1}, // 0+1, 1+1 = 1, 2 NegativeSpans: []prompb.BucketSpan{{Offset: 2, Length: 2}}, - NegativeDeltas: []int64{1, 1}, // 0+1, 1+1 = 1, 2 + NegativeDeltas: []int64{2, -1}, // 1+1, 1+0 = 2, 1 Timestamp: 500, } }, From 79b9a1af605a6a90be069c4eb734ebf510f489da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Sat, 8 Jul 2023 01:41:55 +0200 Subject: [PATCH 06/16] Implement scaledown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: György Krajcsovits --- .../prometheusremotewrite/histograms.go | 69 ++++++++++++++----- .../prometheusremotewrite/histograms_test.go | 64 ++++++++--------- 2 files changed, 84 insertions(+), 49 deletions(-) diff --git a/pkg/translator/prometheusremotewrite/histograms.go b/pkg/translator/prometheusremotewrite/histograms.go index 34ce65729191..e2c1906afcea 100644 --- a/pkg/translator/prometheusremotewrite/histograms.go +++ b/pkg/translator/prometheusremotewrite/histograms.go @@ -58,15 +58,20 @@ func addSingleExponentialHistogramDataPoint( // to Prometheus Native Histogram. func exponentialToNativeHistogram(p pmetric.ExponentialHistogramDataPoint) (prompb.Histogram, error) { scale := p.Scale() - if scale < -4 || scale > 8 { + if scale < -4 { return prompb.Histogram{}, fmt.Errorf("cannot convert exponential to native histogram."+ - " Scale must be <= 8 and >= -4, was %d", scale) - // TODO: downscale to 8 if scale > 8 + " Scale must be >= -4, was %d", scale) } - pSpans, pDeltas := convertBucketsLayout(p.Positive(), 1) - nSpans, nDeltas := convertBucketsLayout(p.Negative(), 1) + var scaleDown int32 + if scale > 8 { + scaleDown = scale - 8 + scale = 8 + } + + pSpans, pDeltas := convertBucketsLayout(p.Positive(), scaleDown) + nSpans, nDeltas := convertBucketsLayout(p.Negative(), scaleDown) h := prompb.Histogram{ Schema: scale, @@ -105,8 +110,8 @@ func exponentialToNativeHistogram(p pmetric.ExponentialHistogramDataPoint) (prom // index 0 corresponds to the range (1, base] while Prometheus bucket index 0 // to the range (base 1]. // -// scaleMerge is the number of buckets to merge into a single bucket - must be power of 2 -func convertBucketsLayout(buckets pmetric.ExponentialHistogramDataPointBuckets, _ int32) ([]prompb.BucketSpan, []int64) { +// scaleDown is the factor by which the buckets are scaled down. In other words 2^scaleDown buckets will be merged into one. +func convertBucketsLayout(buckets pmetric.ExponentialHistogramDataPointBuckets, scaleDown int32) ([]prompb.BucketSpan, []int64) { bucketCounts := buckets.BucketCounts() if bucketCounts.Len() == 0 { return nil, nil @@ -115,6 +120,7 @@ func convertBucketsLayout(buckets pmetric.ExponentialHistogramDataPointBuckets, var ( spans []prompb.BucketSpan deltas []int64 + count int64 prevCount int64 nextBucketIdx int32 ) @@ -125,34 +131,63 @@ func convertBucketsLayout(buckets pmetric.ExponentialHistogramDataPointBuckets, prevCount = count } + // The offset is scaled and adjusted by 1 as described above. + bucketIdx := buckets.Offset()>>scaleDown + 1 + spans = append(spans, prompb.BucketSpan{ + Offset: bucketIdx, + Length: 0, + }) + for i := 0; i < bucketCounts.Len(); i++ { - count := int64(bucketCounts.At(i)) + // The offset is scaled and adjusted by 1 as described above. + nextBucketIdx = (int32(i)+buckets.Offset())>>scaleDown + 1 + if bucketIdx == nextBucketIdx { // we have not collected enough buckets to merge yet + count += int64(bucketCounts.At(i)) + continue + } if count == 0 { + count = int64(bucketCounts.At(i)) continue } - // The offset is adjusted by 1 as described above. - bucketIdx := int32(i) + buckets.Offset() + 1 - delta := bucketIdx - nextBucketIdx - if i == 0 || delta > 2 { - // We have to create a new span, either because we are - // at the very beginning, or because we have found a gap + gap := nextBucketIdx - bucketIdx - 1 + if gap > 2 { + // We have to create a new span, either because we have found a gap // of more than two buckets. The constant 2 is copied from the logic in // https://github.com/prometheus/client_golang/blob/27f0506d6ebbb117b6b697d0552ee5be2502c5f2/prometheus/histogram.go#L1296 spans = append(spans, prompb.BucketSpan{ - Offset: delta, + Offset: gap, Length: 0, }) } else { // We have found a small gap (or no gap at all). // Insert empty buckets as needed. - for j := int32(0); j < delta; j++ { + for j := int32(0); j < gap; j++ { appendDelta(0) } } appendDelta(count) - nextBucketIdx = bucketIdx + 1 + count = int64(bucketCounts.At(i)) + bucketIdx = nextBucketIdx + } + // nextBucketIdx is the last index, not the next one, hence no need to deduct 1 + gap := nextBucketIdx - bucketIdx + if gap > 2 { + // We have to create a new span, because we have found a gap + // of more than two buckets. The constant 2 is copied from the logic in + // https://github.com/prometheus/client_golang/blob/27f0506d6ebbb117b6b697d0552ee5be2502c5f2/prometheus/histogram.go#L1296 + spans = append(spans, prompb.BucketSpan{ + Offset: gap, + Length: 0, + }) + } else { + // We have found a small gap (or no gap at all). + // Insert empty buckets as needed. + for j := int32(0); j < gap; j++ { + appendDelta(0) + } } + appendDelta(count) return spans, deltas } diff --git a/pkg/translator/prometheusremotewrite/histograms_test.go b/pkg/translator/prometheusremotewrite/histograms_test.go index 94335b734bfd..31a4ee581fca 100644 --- a/pkg/translator/prometheusremotewrite/histograms_test.go +++ b/pkg/translator/prometheusremotewrite/histograms_test.go @@ -38,7 +38,7 @@ func TestConvertBucketsLayout(t *testing.T) { return b }, wantLayout: map[int32]expectedBucketLayout{ - 1: { + 0: { wantSpans: []prompb.BucketSpan{ { Offset: 1, @@ -47,7 +47,7 @@ func TestConvertBucketsLayout(t *testing.T) { }, wantDeltas: []int64{4, -1, -1, -1}, }, - 2: { + 1: { wantSpans: []prompb.BucketSpan{ { Offset: 1, @@ -55,9 +55,9 @@ func TestConvertBucketsLayout(t *testing.T) { }, }, // 4+3, 2+1 = 7, 3 =delta= 7, -4 - wantDeltas: []int64{4, -4}, + wantDeltas: []int64{7, -4}, }, - 4: { + 2: { wantSpans: []prompb.BucketSpan{ { Offset: 1, @@ -78,7 +78,7 @@ func TestConvertBucketsLayout(t *testing.T) { return b }, wantLayout: map[int32]expectedBucketLayout{ - 1: { + 0: { wantSpans: []prompb.BucketSpan{ { Offset: 2, @@ -87,7 +87,7 @@ func TestConvertBucketsLayout(t *testing.T) { }, wantDeltas: []int64{4, -1, -1, -1}, }, - 2: { + 1: { wantSpans: []prompb.BucketSpan{ { Offset: 1, @@ -96,7 +96,7 @@ func TestConvertBucketsLayout(t *testing.T) { }, wantDeltas: []int64{4, 1, -4}, // 0+4, 3+2, 1+0 = 4, 5, 1 }, - 4: { + 2: { wantSpans: []prompb.BucketSpan{ { Offset: 1, @@ -116,7 +116,7 @@ func TestConvertBucketsLayout(t *testing.T) { return b }, wantLayout: map[int32]expectedBucketLayout{ - 1: { + 0: { wantSpans: []prompb.BucketSpan{ { Offset: 5, @@ -129,7 +129,7 @@ func TestConvertBucketsLayout(t *testing.T) { }, wantDeltas: []int64{4, -2, -2, 2, -1}, }, - 2: { + 1: { wantSpans: []prompb.BucketSpan{ { Offset: 3, @@ -144,7 +144,7 @@ func TestConvertBucketsLayout(t *testing.T) { // 4+2, 0+2, 0+0, 0+0, 0+0, 0+0, 0+0, 0+0, 1+0 = 6, 2, 0, 0, 0, 0, 0, 0, 1 wantDeltas: []int64{6, -4, -1}, }, - 4: { + 2: { wantSpans: []prompb.BucketSpan{ { Offset: 2, @@ -171,7 +171,7 @@ func TestConvertBucketsLayout(t *testing.T) { return b }, wantLayout: map[int32]expectedBucketLayout{ - 1: { + 0: { wantSpans: []prompb.BucketSpan{ { Offset: 5, @@ -184,10 +184,10 @@ func TestConvertBucketsLayout(t *testing.T) { }, wantDeltas: []int64{4, -2, -2, 2, -1}, }, - 2: { + 1: { wantSpans: []prompb.BucketSpan{ { - Offset: 2, + Offset: 3, Length: 2, }, { @@ -199,7 +199,7 @@ func TestConvertBucketsLayout(t *testing.T) { // 4+2, 0+2, 0+0, 0+0, 0+0, 0+0, 1+0 = 6, 2, 0, 0, 0, 0, 1 wantDeltas: []int64{6, -4, -1}, }, - 4: { + 2: { wantSpans: []prompb.BucketSpan{ { Offset: 2, @@ -222,7 +222,7 @@ func TestConvertBucketsLayout(t *testing.T) { return b }, wantLayout: map[int32]expectedBucketLayout{ - 1: { + 0: { wantSpans: []prompb.BucketSpan{ { Offset: -1, @@ -235,10 +235,10 @@ func TestConvertBucketsLayout(t *testing.T) { }, wantDeltas: []int64{3, -2, 0}, }, - 2: { + 1: { wantSpans: []prompb.BucketSpan{ { - Offset: -1, + Offset: 0, Length: 3, }, }, @@ -246,10 +246,10 @@ func TestConvertBucketsLayout(t *testing.T) { // 3+1, 0+0, 0+1 = 4, 0, 1 wantDeltas: []int64{4, -4, 1}, }, - 4: { + 2: { wantSpans: []prompb.BucketSpan{ { - Offset: -1, + Offset: 0, Length: 2, }, }, @@ -268,7 +268,7 @@ func TestConvertBucketsLayout(t *testing.T) { return b }, wantLayout: map[int32]expectedBucketLayout{ - 1: { + 0: { wantSpans: []prompb.BucketSpan{ { Offset: -1, @@ -277,10 +277,10 @@ func TestConvertBucketsLayout(t *testing.T) { }, wantDeltas: []int64{3, -2, -1, 1, -1, 1}, }, - 2: { + 1: { wantSpans: []prompb.BucketSpan{ { - Offset: -1, + Offset: 0, Length: 3, }, }, @@ -288,10 +288,10 @@ func TestConvertBucketsLayout(t *testing.T) { // 3+1, 0+1, 0+1 = 4, 1, 1 wantDeltas: []int64{4, -3, 0}, }, - 4: { + 2: { wantSpans: []prompb.BucketSpan{ { - Offset: -1, + Offset: 0, Length: 2, }, }, @@ -310,7 +310,7 @@ func TestConvertBucketsLayout(t *testing.T) { return b }, wantLayout: map[int32]expectedBucketLayout{ - 1: { + 0: { wantSpans: []prompb.BucketSpan{ { Offset: -1, @@ -319,10 +319,10 @@ func TestConvertBucketsLayout(t *testing.T) { }, wantDeltas: []int64{3, -3, 0, 1, -1, 0, 1}, }, - 2: { + 1: { wantSpans: []prompb.BucketSpan{ { - Offset: -1, + Offset: 0, Length: 4, }, }, @@ -330,10 +330,10 @@ func TestConvertBucketsLayout(t *testing.T) { // 3+0, 0+1, 0+0, 0+1 = 3, 1, 0, 1 wantDeltas: []int64{3, -2, -1, 1}, }, - 4: { + 2: { wantSpans: []prompb.BucketSpan{ { - Offset: -1, + Offset: 0, Length: 3, }, }, @@ -347,15 +347,15 @@ func TestConvertBucketsLayout(t *testing.T) { name: "zero buckets", buckets: pmetric.NewExponentialHistogramDataPointBuckets, wantLayout: map[int32]expectedBucketLayout{ - 1: { + 0: { wantSpans: nil, wantDeltas: nil, }, - 2: { + 1: { wantSpans: nil, wantDeltas: nil, }, - 4: { + 2: { wantSpans: nil, wantDeltas: nil, }, From c1e8359f04367fb3b872cec290047413e29e2ee6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Sun, 9 Jul 2023 16:03:59 +0200 Subject: [PATCH 07/16] Benchmark convert buckets layout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: György Krajcsovits --- .../prometheusremotewrite/histograms_test.go | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/pkg/translator/prometheusremotewrite/histograms_test.go b/pkg/translator/prometheusremotewrite/histograms_test.go index 31a4ee581fca..18e2715c916f 100644 --- a/pkg/translator/prometheusremotewrite/histograms_test.go +++ b/pkg/translator/prometheusremotewrite/histograms_test.go @@ -373,6 +373,34 @@ func TestConvertBucketsLayout(t *testing.T) { } } +func BenchmarkConvertBucketLayout(b *testing.B) { + scenarios := []struct { + gap int + }{ + {gap: 0}, + {gap: 1}, + {gap: 2}, + {gap: 3}, + } + + for _, scenario := range scenarios { + buckets := pmetric.NewExponentialHistogramDataPointBuckets() + buckets.SetOffset(0) + for i := 0; i < 1000; i++ { + if i%(scenario.gap+1) == 0 { + buckets.BucketCounts().Append(10) + } else { + buckets.BucketCounts().Append(0) + } + } + b.Run(fmt.Sprintf("gap %d", scenario.gap), func(b *testing.B) { + for i := 0; i < b.N; i++ { + convertBucketsLayout(buckets) + } + }) + } +} + func TestExponentialToNativeHistogram(t *testing.T) { tests := []struct { name string From f9475bc009dd302035eac74dc41d8ce3e2785598 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Sun, 9 Jul 2023 18:26:27 +0200 Subject: [PATCH 08/16] Optimize: do not recalc length of input buckets. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Saves about 10%. Signed-off-by: György Krajcsovits --- pkg/translator/prometheusremotewrite/histograms.go | 12 ++++++++---- .../prometheusremotewrite/histograms_test.go | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/pkg/translator/prometheusremotewrite/histograms.go b/pkg/translator/prometheusremotewrite/histograms.go index e2c1906afcea..17ea4b96f897 100644 --- a/pkg/translator/prometheusremotewrite/histograms.go +++ b/pkg/translator/prometheusremotewrite/histograms.go @@ -131,6 +131,10 @@ func convertBucketsLayout(buckets pmetric.ExponentialHistogramDataPointBuckets, prevCount = count } + // Let the compiler figure out that this is const during this function by + // moving it into a local variable. + bucketsCount := bucketCounts.Len() + // The offset is scaled and adjusted by 1 as described above. bucketIdx := buckets.Offset()>>scaleDown + 1 spans = append(spans, prompb.BucketSpan{ @@ -138,10 +142,10 @@ func convertBucketsLayout(buckets pmetric.ExponentialHistogramDataPointBuckets, Length: 0, }) - for i := 0; i < bucketCounts.Len(); i++ { + for i := 0; i < bucketsCount; i++ { // The offset is scaled and adjusted by 1 as described above. nextBucketIdx = (int32(i)+buckets.Offset())>>scaleDown + 1 - if bucketIdx == nextBucketIdx { // we have not collected enough buckets to merge yet + if bucketIdx == nextBucketIdx { // We have not collected enough buckets to merge yet. count += int64(bucketCounts.At(i)) continue } @@ -152,7 +156,7 @@ func convertBucketsLayout(buckets pmetric.ExponentialHistogramDataPointBuckets, gap := nextBucketIdx - bucketIdx - 1 if gap > 2 { - // We have to create a new span, either because we have found a gap + // We have to create a new span, because we have found a gap // of more than two buckets. The constant 2 is copied from the logic in // https://github.com/prometheus/client_golang/blob/27f0506d6ebbb117b6b697d0552ee5be2502c5f2/prometheus/histogram.go#L1296 spans = append(spans, prompb.BucketSpan{ @@ -170,7 +174,7 @@ func convertBucketsLayout(buckets pmetric.ExponentialHistogramDataPointBuckets, count = int64(bucketCounts.At(i)) bucketIdx = nextBucketIdx } - // nextBucketIdx is the last index, not the next one, hence no need to deduct 1 + // nextBucketIdx is the last index, not the next one, hence no need to deduct 1. gap := nextBucketIdx - bucketIdx if gap > 2 { // We have to create a new span, because we have found a gap diff --git a/pkg/translator/prometheusremotewrite/histograms_test.go b/pkg/translator/prometheusremotewrite/histograms_test.go index 18e2715c916f..d6a29e2738f7 100644 --- a/pkg/translator/prometheusremotewrite/histograms_test.go +++ b/pkg/translator/prometheusremotewrite/histograms_test.go @@ -395,7 +395,7 @@ func BenchmarkConvertBucketLayout(b *testing.B) { } b.Run(fmt.Sprintf("gap %d", scenario.gap), func(b *testing.B) { for i := 0; i < b.N; i++ { - convertBucketsLayout(buckets) + convertBucketsLayout(buckets, 0) } }) } From e2a964f5602b3bba103522c5d9553262cf18e8e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Sun, 9 Jul 2023 19:03:45 +0200 Subject: [PATCH 09/16] Optimize: reduce variable scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We don't need to carry nextBucketIdx outside the loop since we can easily recalculate its value. Signed-off-by: György Krajcsovits --- .../prometheusremotewrite/histograms.go | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/pkg/translator/prometheusremotewrite/histograms.go b/pkg/translator/prometheusremotewrite/histograms.go index 17ea4b96f897..5b042c14836b 100644 --- a/pkg/translator/prometheusremotewrite/histograms.go +++ b/pkg/translator/prometheusremotewrite/histograms.go @@ -118,11 +118,10 @@ func convertBucketsLayout(buckets pmetric.ExponentialHistogramDataPointBuckets, } var ( - spans []prompb.BucketSpan - deltas []int64 - count int64 - prevCount int64 - nextBucketIdx int32 + spans []prompb.BucketSpan + deltas []int64 + count int64 + prevCount int64 ) appendDelta := func(count int64) { @@ -144,7 +143,7 @@ func convertBucketsLayout(buckets pmetric.ExponentialHistogramDataPointBuckets, for i := 0; i < bucketsCount; i++ { // The offset is scaled and adjusted by 1 as described above. - nextBucketIdx = (int32(i)+buckets.Offset())>>scaleDown + 1 + nextBucketIdx := (int32(i)+buckets.Offset())>>scaleDown + 1 if bucketIdx == nextBucketIdx { // We have not collected enough buckets to merge yet. count += int64(bucketCounts.At(i)) continue @@ -174,8 +173,8 @@ func convertBucketsLayout(buckets pmetric.ExponentialHistogramDataPointBuckets, count = int64(bucketCounts.At(i)) bucketIdx = nextBucketIdx } - // nextBucketIdx is the last index, not the next one, hence no need to deduct 1. - gap := nextBucketIdx - bucketIdx + // Need to use the last item's index. The offset is scaled and adjusted by 1 as described above. + gap := (int32(bucketsCount)+buckets.Offset()-1)>>scaleDown + 1 - bucketIdx if gap > 2 { // We have to create a new span, because we have found a gap // of more than two buckets. The constant 2 is copied from the logic in From af9e1600e7cddb9878d8b28608aef6d9132d2482 Mon Sep 17 00:00:00 2001 From: George Krajcsovits Date: Wed, 12 Jul 2023 12:14:51 +0200 Subject: [PATCH 10/16] Update .chloggen/prometheusremotewrite-downscale-histogram.yaml Co-authored-by: Ruslan Kovalov --- .chloggen/prometheusremotewrite-downscale-histogram.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.chloggen/prometheusremotewrite-downscale-histogram.yaml b/.chloggen/prometheusremotewrite-downscale-histogram.yaml index 1c36f4c0920a..01ca351e0de0 100644 --- a/.chloggen/prometheusremotewrite-downscale-histogram.yaml +++ b/.chloggen/prometheusremotewrite-downscale-histogram.yaml @@ -12,7 +12,7 @@ component: pkg/translator/prometheusremotewrite note: Downscale exponential histograms to fit prometheus native histograms. # Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. -issues: [1756] +issues: [17565] # (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. From 8486261a2a3d9832f2d7bd2bff48d2dd44f8af34 Mon Sep 17 00:00:00 2001 From: George Krajcsovits Date: Fri, 14 Jul 2023 06:37:48 +0200 Subject: [PATCH 11/16] Update .chloggen/prometheusremotewrite-downscale-histogram.yaml Co-authored-by: Anthony Mirabella --- .chloggen/prometheusremotewrite-downscale-histogram.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.chloggen/prometheusremotewrite-downscale-histogram.yaml b/.chloggen/prometheusremotewrite-downscale-histogram.yaml index 01ca351e0de0..e7d3721d5109 100644 --- a/.chloggen/prometheusremotewrite-downscale-histogram.yaml +++ b/.chloggen/prometheusremotewrite-downscale-histogram.yaml @@ -9,7 +9,7 @@ change_type: enhancement component: pkg/translator/prometheusremotewrite # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). -note: Downscale exponential histograms to fit prometheus native histograms. +note: Downscale exponential histograms to fit prometheus native histograms if necessary. # Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. issues: [17565] From 08f6cf4ae0cf25891c5f355aeb4641ed31078664 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Fri, 14 Jul 2023 06:40:42 +0200 Subject: [PATCH 12/16] Rename bucketsCount to numBuckets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: György Krajcsovits --- pkg/translator/prometheusremotewrite/histograms.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/translator/prometheusremotewrite/histograms.go b/pkg/translator/prometheusremotewrite/histograms.go index 5b042c14836b..ec25abcefcca 100644 --- a/pkg/translator/prometheusremotewrite/histograms.go +++ b/pkg/translator/prometheusremotewrite/histograms.go @@ -132,7 +132,7 @@ func convertBucketsLayout(buckets pmetric.ExponentialHistogramDataPointBuckets, // Let the compiler figure out that this is const during this function by // moving it into a local variable. - bucketsCount := bucketCounts.Len() + numBuckets := bucketCounts.Len() // The offset is scaled and adjusted by 1 as described above. bucketIdx := buckets.Offset()>>scaleDown + 1 @@ -141,7 +141,7 @@ func convertBucketsLayout(buckets pmetric.ExponentialHistogramDataPointBuckets, Length: 0, }) - for i := 0; i < bucketsCount; i++ { + for i := 0; i < numBuckets; i++ { // The offset is scaled and adjusted by 1 as described above. nextBucketIdx := (int32(i)+buckets.Offset())>>scaleDown + 1 if bucketIdx == nextBucketIdx { // We have not collected enough buckets to merge yet. @@ -174,7 +174,7 @@ func convertBucketsLayout(buckets pmetric.ExponentialHistogramDataPointBuckets, bucketIdx = nextBucketIdx } // Need to use the last item's index. The offset is scaled and adjusted by 1 as described above. - gap := (int32(bucketsCount)+buckets.Offset()-1)>>scaleDown + 1 - bucketIdx + gap := (int32(numBuckets)+buckets.Offset()-1)>>scaleDown + 1 - bucketIdx if gap > 2 { // We have to create a new span, because we have found a gap // of more than two buckets. The constant 2 is copied from the logic in From 36ace2370339eba5d59073242028d93ada1202f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Fri, 14 Jul 2023 09:59:54 +0200 Subject: [PATCH 13/16] Fix count calculation and add asserts on input and output counts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: György Krajcsovits --- .../prometheusremotewrite/histograms.go | 16 +++++++++++++- .../prometheusremotewrite/histograms_test.go | 22 ++++++++++++++++++- 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/pkg/translator/prometheusremotewrite/histograms.go b/pkg/translator/prometheusremotewrite/histograms.go index ec25abcefcca..0581e8f93552 100644 --- a/pkg/translator/prometheusremotewrite/histograms.go +++ b/pkg/translator/prometheusremotewrite/histograms.go @@ -96,7 +96,11 @@ func exponentialToNativeHistogram(p pmetric.ExponentialHistogramDataPoint) (prom if p.HasSum() { h.Sum = p.Sum() } - h.Count = &prompb.Histogram_CountInt{CountInt: p.Count()} + if scaleDown > 0 { + h.Count = &prompb.Histogram_CountInt{CountInt: nativeHistogramBucketCount(&h)} + } else { + h.Count = &prompb.Histogram_CountInt{CountInt: p.Count()} + } } return h, nil } @@ -194,3 +198,13 @@ func convertBucketsLayout(buckets pmetric.ExponentialHistogramDataPointBuckets, return spans, deltas } + +func nativeHistogramBucketCount(h *prompb.Histogram) (count uint64) { + for _, span := range h.PositiveSpans { + count += uint64(span.Length) + } + for _, span := range h.NegativeSpans { + count += uint64(span.Length) + } + return +} diff --git a/pkg/translator/prometheusremotewrite/histograms_test.go b/pkg/translator/prometheusremotewrite/histograms_test.go index d6a29e2738f7..82875cbcdfc0 100644 --- a/pkg/translator/prometheusremotewrite/histograms_test.go +++ b/pkg/translator/prometheusremotewrite/histograms_test.go @@ -536,7 +536,7 @@ func TestExponentialToNativeHistogram(t *testing.T) { }, wantNativeHist: func() prompb.Histogram { return prompb.Histogram{ - Count: &prompb.Histogram_CountInt{CountInt: 6}, + Count: &prompb.Histogram_CountInt{CountInt: 4}, Sum: 10.1, Schema: 8, ZeroThreshold: defaultZeroThreshold, @@ -552,6 +552,7 @@ func TestExponentialToNativeHistogram(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + validateHistogramCount(t, tt.exponentialHist()) // Sanity check. got, err := exponentialToNativeHistogram(tt.exponentialHist()) if tt.wantErrMessage != "" { assert.ErrorContains(t, err, tt.wantErrMessage) @@ -560,10 +561,29 @@ func TestExponentialToNativeHistogram(t *testing.T) { require.NoError(t, err) assert.Equal(t, tt.wantNativeHist(), got) + validateNativeHistogramCount(t, got) }) } } +func validateHistogramCount(t *testing.T, h pmetric.ExponentialHistogramDataPoint) { + require.Equal(t, h.Count(), uint64(h.Positive().BucketCounts().Len())+uint64(h.Negative().BucketCounts().Len())) +} + +func validateNativeHistogramCount(t *testing.T, h prompb.Histogram) { + require.NotNil(t, h.Count) + require.IsType(t, &prompb.Histogram_CountInt{}, h.Count) + want := h.Count.(*prompb.Histogram_CountInt).CountInt + actualCount := uint64(0) + for _, span := range h.PositiveSpans { + actualCount += uint64(span.Length) + } + for _, span := range h.NegativeSpans { + actualCount += uint64(span.Length) + } + assert.Equal(t, want, actualCount) +} + func TestAddSingleExponentialHistogramDataPoint(t *testing.T) { tests := []struct { name string From 0b8f3181852ba1ddca599a4204b635e05e362929 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Fri, 14 Jul 2023 12:23:41 +0200 Subject: [PATCH 14/16] Fix assert on count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Undo previous mess. Signed-off-by: György Krajcsovits --- .../prometheusremotewrite/histograms.go | 6 +-- .../prometheusremotewrite/histograms_test.go | 41 ++++++++++++++----- 2 files changed, 32 insertions(+), 15 deletions(-) diff --git a/pkg/translator/prometheusremotewrite/histograms.go b/pkg/translator/prometheusremotewrite/histograms.go index 0581e8f93552..d7337e404568 100644 --- a/pkg/translator/prometheusremotewrite/histograms.go +++ b/pkg/translator/prometheusremotewrite/histograms.go @@ -96,11 +96,7 @@ func exponentialToNativeHistogram(p pmetric.ExponentialHistogramDataPoint) (prom if p.HasSum() { h.Sum = p.Sum() } - if scaleDown > 0 { - h.Count = &prompb.Histogram_CountInt{CountInt: nativeHistogramBucketCount(&h)} - } else { - h.Count = &prompb.Histogram_CountInt{CountInt: p.Count()} - } + h.Count = &prompb.Histogram_CountInt{CountInt: p.Count()} } return h, nil } diff --git a/pkg/translator/prometheusremotewrite/histograms_test.go b/pkg/translator/prometheusremotewrite/histograms_test.go index 82875cbcdfc0..34003d3048ec 100644 --- a/pkg/translator/prometheusremotewrite/histograms_test.go +++ b/pkg/translator/prometheusremotewrite/histograms_test.go @@ -536,7 +536,7 @@ func TestExponentialToNativeHistogram(t *testing.T) { }, wantNativeHist: func() prompb.Histogram { return prompb.Histogram{ - Count: &prompb.Histogram_CountInt{CountInt: 4}, + Count: &prompb.Histogram_CountInt{CountInt: 6}, Sum: 10.1, Schema: 8, ZeroThreshold: defaultZeroThreshold, @@ -552,7 +552,7 @@ func TestExponentialToNativeHistogram(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - validateHistogramCount(t, tt.exponentialHist()) // Sanity check. + validateExponentialHistogramCount(t, tt.exponentialHist()) // Sanity check. got, err := exponentialToNativeHistogram(tt.exponentialHist()) if tt.wantErrMessage != "" { assert.ErrorContains(t, err, tt.wantErrMessage) @@ -566,22 +566,43 @@ func TestExponentialToNativeHistogram(t *testing.T) { } } -func validateHistogramCount(t *testing.T, h pmetric.ExponentialHistogramDataPoint) { - require.Equal(t, h.Count(), uint64(h.Positive().BucketCounts().Len())+uint64(h.Negative().BucketCounts().Len())) +func validateExponentialHistogramCount(t *testing.T, h pmetric.ExponentialHistogramDataPoint) { + actualCount := uint64(0) + for _, bucket := range h.Positive().BucketCounts().AsRaw() { + actualCount += bucket + } + for _, bucket := range h.Negative().BucketCounts().AsRaw() { + actualCount += bucket + } + require.Equal(t, h.Count(), actualCount, "exponential histogram count mismatch") } func validateNativeHistogramCount(t *testing.T, h prompb.Histogram) { require.NotNil(t, h.Count) require.IsType(t, &prompb.Histogram_CountInt{}, h.Count) want := h.Count.(*prompb.Histogram_CountInt).CountInt - actualCount := uint64(0) - for _, span := range h.PositiveSpans { - actualCount += uint64(span.Length) + var ( + actualCount uint64 + prevCount int64 + ) + for i, delta := range h.PositiveDeltas { + if i == 0 { + actualCount += uint64(delta) + } else { + actualCount += uint64(prevCount + delta) + } + prevCount += delta } - for _, span := range h.NegativeSpans { - actualCount += uint64(span.Length) + prevCount = 0 + for i, delta := range h.NegativeDeltas { + if i == 0 { + actualCount += uint64(delta) + } else { + actualCount += uint64(prevCount + delta) + } + prevCount += delta } - assert.Equal(t, want, actualCount) + assert.Equal(t, want, actualCount, "native histogram count mismatch") } func TestAddSingleExponentialHistogramDataPoint(t *testing.T) { From bafdf7b8b836022482d189276aed441e427f5e14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Fri, 14 Jul 2023 12:25:32 +0200 Subject: [PATCH 15/16] Remove unused function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: György Krajcsovits --- pkg/translator/prometheusremotewrite/histograms.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/pkg/translator/prometheusremotewrite/histograms.go b/pkg/translator/prometheusremotewrite/histograms.go index d7337e404568..ec25abcefcca 100644 --- a/pkg/translator/prometheusremotewrite/histograms.go +++ b/pkg/translator/prometheusremotewrite/histograms.go @@ -194,13 +194,3 @@ func convertBucketsLayout(buckets pmetric.ExponentialHistogramDataPointBuckets, return spans, deltas } - -func nativeHistogramBucketCount(h *prompb.Histogram) (count uint64) { - for _, span := range h.PositiveSpans { - count += uint64(span.Length) - } - for _, span := range h.NegativeSpans { - count += uint64(span.Length) - } - return -} From a685502107f27fc38d39e40f13ff79bd9e64d085 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Fri, 14 Jul 2023 12:29:41 +0200 Subject: [PATCH 16/16] Simplify code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: György Krajcsovits --- .../prometheusremotewrite/histograms_test.go | 24 +++++++------------ 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/pkg/translator/prometheusremotewrite/histograms_test.go b/pkg/translator/prometheusremotewrite/histograms_test.go index 34003d3048ec..49e4768e5383 100644 --- a/pkg/translator/prometheusremotewrite/histograms_test.go +++ b/pkg/translator/prometheusremotewrite/histograms_test.go @@ -583,24 +583,16 @@ func validateNativeHistogramCount(t *testing.T, h prompb.Histogram) { want := h.Count.(*prompb.Histogram_CountInt).CountInt var ( actualCount uint64 - prevCount int64 + prevBucket int64 ) - for i, delta := range h.PositiveDeltas { - if i == 0 { - actualCount += uint64(delta) - } else { - actualCount += uint64(prevCount + delta) - } - prevCount += delta + for _, delta := range h.PositiveDeltas { + prevBucket += delta + actualCount += uint64(prevBucket) } - prevCount = 0 - for i, delta := range h.NegativeDeltas { - if i == 0 { - actualCount += uint64(delta) - } else { - actualCount += uint64(prevCount + delta) - } - prevCount += delta + prevBucket = 0 + for _, delta := range h.NegativeDeltas { + prevBucket += delta + actualCount += uint64(prevBucket) } assert.Equal(t, want, actualCount, "native histogram count mismatch") }