util: add native histogram support to SendSumOfHistograms#7178
util: add native histogram support to SendSumOfHistograms#7178siddarth2810 wants to merge 3 commits intocortexproject:masterfrom
Conversation
Extend HistogramData struct with native histogram fields. Convert input spans+deltas to map[int]int64 for aggregation. On output, MustNewConstNativeHistogram calls makeBucketsFromMap() to reconstruct spans+deltas for wire format. Signed-off-by: Siddarth Gundu <siddarthg0910@gmail.com>
Signed-off-by: Siddarth Gundu <siddarthg0910@gmail.com>
isNative() treated any Histogram with a non-zero ZeroThreshold as native, even when classic buckets were present. In the alertmanager tests the incoming histograms have classic buckets and native histogram fields like `ZeroThreshold` but no spans/deltas, so AddHistogram() routed them to the native path, causing validateCount panic when bucket population != sampleCount. Signed-off-by: Siddarth Gundu <siddarthg0910@gmail.com>
siddarth2810
force-pushed
the
support-native-histograms
branch
from
f335ae6 to
03942b1
Compare
There was a problem hiding this comment.
Thanks for the PR,
Looking at
cortex/pkg/storegateway/bucket_store_metrics_test.go
Lines 534 to 545 in 65c6a2a
I think we need a TestBucketStoreNativeHistogramMetrics. do you agree?. We want to check what it is returning after this change. right?.
|
|
||
| classic := len(histo.GetBucket()) > 0 | ||
|
|
||
| return native && !classic |
There was a problem hiding this comment.
You mentioned that this is for the alertmanager test. But can we reproduce it in a unit test in metrics_helper_test.go? I feel this is unlikely to have hybrid histograms.
There was a problem hiding this comment.
Nvm I got it now. As long as we define histogram to be available as NH then every observation will add to both classic histogram bucket as well as NH buckets
yeya24
left a comment
yeya24
left a comment
There was a problem hiding this comment.
The usecase of having SendSumOfHistograms to support native histogram is for this metrics aggregation continues to work if the underlying data is NH.
I wonder if it is possible to use a real metric like from Prometheus and we aggregate it via SendSumOfHistograms and make sure it is working properly. But it is not a blocker
| len(h.GetNegativeSpan()) > 0 || | ||
| h.GetZeroCount() > 0 || | ||
| h.GetZeroThreshold() > 0, | ||
| "expected a native histogram output (spans/zero bucket present)", |
There was a problem hiding this comment.
Can we assert on detailed fields of NH to make sure the generated data is correct? Instead of simply checking it is a native histogram
3 participants
Extend HistogramData struct with native histogram fields. Convert input spans+deltas to
map[int]int64for aggregation. On output,MustNewConstNativeHistogramcallsmakeBucketsFromMap()to reconstruct spans+deltas for wire format.What this PR does:
Which issue(s) this PR fixes:
Fixes #6489
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]