Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pkg/translate/promremotewrite] downscale exponential histograms for prometheus #24026

Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
4ee29ff
promremotewrite: Fix histogram unit test observation count
krajorama Jul 7, 2023
bf70fc8
Promremotewrite: test histogram downscaling
krajorama Jul 7, 2023
b8d41b6
Add changelog file.
krajorama Jul 7, 2023
7f2d6ae
Update convertBucketsLayout and its unit tests
krajorama Jul 7, 2023
a7e0f92
Fix offsets and enable test
krajorama Jul 7, 2023
79b9a1a
Implement scaledown
krajorama Jul 7, 2023
c1e8359
Benchmark convert buckets layout
krajorama Jul 9, 2023
f9475bc
Optimize: do not recalc length of input buckets.
krajorama Jul 9, 2023
e2a964f
Optimize: reduce variable scope
krajorama Jul 9, 2023
af9e160
Update .chloggen/prometheusremotewrite-downscale-histogram.yaml
krajorama Jul 12, 2023
413e3d0
Merge branch 'main' into promremotewrite-downscale-histograms-17565
jpkrohling Jul 12, 2023
20fbc94
Merge branch 'main' into promremotewrite-downscale-histograms-17565
Aneurysm9 Jul 13, 2023
8486261
Update .chloggen/prometheusremotewrite-downscale-histogram.yaml
krajorama Jul 14, 2023
08f6cf4
Rename bucketsCount to numBuckets
krajorama Jul 14, 2023
d6499e6
Merge branch 'main' into promremotewrite-downscale-histograms-17565
krajorama Jul 14, 2023
36ace23
Fix count calculation and add asserts on input and output counts
krajorama Jul 14, 2023
b548ab7
Merge branch 'main' into promremotewrite-downscale-histograms-17565
krajorama Jul 14, 2023
0b8f318
Fix assert on count
krajorama Jul 14, 2023
bafdf7b
Remove unused function
krajorama Jul 14, 2023
a685502
Simplify code
krajorama Jul 14, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions .chloggen/prometheusremotewrite-downscale-histogram.yaml
Original file line number Diff line number Diff line change
@@ -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.
krajorama marked this conversation as resolved.
Show resolved Hide resolved

# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
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.
# Use pipe (|) for multiline entries.
subtext:
82 changes: 61 additions & 21 deletions pkg/translator/prometheusremotewrite/histograms.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
nSpans, nDeltas := convertBucketsLayout(p.Negative())
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,
Expand Down Expand Up @@ -104,17 +109,19 @@ 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) {
//
// 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
}

var (
spans []prompb.BucketSpan
deltas []int64
prevCount int64
nextBucketIdx int32
spans []prompb.BucketSpan
deltas []int64
count int64
prevCount int64
)

appendDelta := func(count int64) {
Expand All @@ -123,34 +130,67 @@ func convertBucketsLayout(buckets pmetric.ExponentialHistogramDataPointBuckets)
prevCount = count
}

for i := 0; i < bucketCounts.Len(); i++ {
count := int64(bucketCounts.At(i))
// Let the compiler figure out that this is const during this function by
// moving it into a local variable.
bucketsCount := bucketCounts.Len()
krajorama marked this conversation as resolved.
Show resolved Hide resolved

// 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 < 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.
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, 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
}
// 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
// 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
}
Loading