From 2ea8df4734e8b940b2a78166f4fb4b0288211336 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Tue, 18 Jul 2023 18:40:51 +0200 Subject: [PATCH 1/2] histogram: Expose #12305 Native histograms without a zero threshold aren't federated properly. This adds a test to prove the specific failure mode, which is that histograms with a zero threshold of zero are federated as classic histograms. The underlying reason is that the protobuf parser identifies a native histogram by detecting a zero bucket or by detecting integer buckets. Therefore, a float histogram with a zero threshold of zero and an unpopulated zero bucket falls through the cracks (no integer buckets, no zero bucket). This commit also addse a test case for the latter. Signed-off-by: beorn7 --- model/textparse/protobufparse_test.go | 86 +++++++++++++++++++++++---- web/federate_test.go | 27 ++++++++- 2 files changed, 101 insertions(+), 12 deletions(-) diff --git a/model/textparse/protobufparse_test.go b/model/textparse/protobufparse_test.go index 88ad9f221..07f84f4ae 100644 --- a/model/textparse/protobufparse_test.go +++ b/model/textparse/protobufparse_test.go @@ -463,6 +463,24 @@ metric: < > > +`, + `name: "test_float_histogram_with_zerothreshold_zero" +help: "Test float histogram with a zero threshold of zero." +type: HISTOGRAM +metric: < + histogram: < + sample_count_float: 5.0 + sample_sum: 12.1 + schema: 3 + positive_span: < + offset: 8 + length: 2 + > + positive_count: 2.0 + positive_count: 3.0 + > +> + `, `name: "rpc_durations_seconds" help: "RPC latency distributions." @@ -850,6 +868,30 @@ func TestProtobufParse(t *testing.T) { "foo", "baz", ), }, + { + m: "test_float_histogram_with_zerothreshold_zero", + help: "Test float histogram with a zero threshold of zero.", + }, + { + m: "test_float_histogram_with_zerothreshold_zero", + typ: MetricTypeHistogram, + }, + { + m: "test_float_histogram_with_zerothreshold_zero", + fhs: &histogram.FloatHistogram{ + Count: 5.0, + Sum: 12.1, + Schema: 3, + PositiveSpans: []histogram.Span{ + {Offset: 8, Length: 2}, + }, + PositiveBuckets: []float64{2.0, 3.0}, + NegativeSpans: []histogram.Span{}, + }, + lset: labels.FromStrings( + "__name__", "test_float_histogram_with_zerothreshold_zero", + ), + }, { m: "rpc_durations_seconds", help: "RPC latency distributions.", @@ -1550,14 +1592,38 @@ func TestProtobufParse(t *testing.T) { ), }, { // 67 + m: "test_float_histogram_with_zerothreshold_zero", + help: "Test float histogram with a zero threshold of zero.", + }, + { // 68 + m: "test_float_histogram_with_zerothreshold_zero", + typ: MetricTypeHistogram, + }, + { // 69 + m: "test_float_histogram_with_zerothreshold_zero", + fhs: &histogram.FloatHistogram{ + Count: 5.0, + Sum: 12.1, + Schema: 3, + PositiveSpans: []histogram.Span{ + {Offset: 8, Length: 2}, + }, + PositiveBuckets: []float64{2.0, 3.0}, + NegativeSpans: []histogram.Span{}, + }, + lset: labels.FromStrings( + "__name__", "test_float_histogram_with_zerothreshold_zero", + ), + }, + { // 70 m: "rpc_durations_seconds", help: "RPC latency distributions.", }, - { // 68 + { // 71 m: "rpc_durations_seconds", typ: MetricTypeSummary, }, - { // 69 + { // 72 m: "rpc_durations_seconds_count\xffservice\xffexponential", v: 262, lset: labels.FromStrings( @@ -1565,7 +1631,7 @@ func TestProtobufParse(t *testing.T) { "service", "exponential", ), }, - { // 70 + { // 73 m: "rpc_durations_seconds_sum\xffservice\xffexponential", v: 0.00025551262820703587, lset: labels.FromStrings( @@ -1573,7 +1639,7 @@ func TestProtobufParse(t *testing.T) { "service", "exponential", ), }, - { // 71 + { // 74 m: "rpc_durations_seconds\xffservice\xffexponential\xffquantile\xff0.5", v: 6.442786329648548e-07, lset: labels.FromStrings( @@ -1582,7 +1648,7 @@ func TestProtobufParse(t *testing.T) { "service", "exponential", ), }, - { // 72 + { // 75 m: "rpc_durations_seconds\xffservice\xffexponential\xffquantile\xff0.9", v: 1.9435742936658396e-06, lset: labels.FromStrings( @@ -1591,7 +1657,7 @@ func TestProtobufParse(t *testing.T) { "service", "exponential", ), }, - { // 73 + { // 76 m: "rpc_durations_seconds\xffservice\xffexponential\xffquantile\xff0.99", v: 4.0471608667037015e-06, lset: labels.FromStrings( @@ -1600,22 +1666,22 @@ func TestProtobufParse(t *testing.T) { "service", "exponential", ), }, - { // 74 + { // 77 m: "without_quantiles", help: "A summary without quantiles.", }, - { // 75 + { // 78 m: "without_quantiles", typ: MetricTypeSummary, }, - { // 76 + { // 79 m: "without_quantiles_count", v: 42, lset: labels.FromStrings( "__name__", "without_quantiles_count", ), }, - { // 77 + { // 80 m: "without_quantiles_sum", v: 1.234, lset: labels.FromStrings( diff --git a/web/federate_test.go b/web/federate_test.go index 6bc0ae212..4fdcf8daa 100644 --- a/web/federate_test.go +++ b/web/federate_test.go @@ -335,18 +335,41 @@ func TestFederationWithNativeHistograms(t *testing.T) { }, NegativeBuckets: []int64{1, 1, -1, 0}, } + histWithoutZeroBucket := &histogram.Histogram{ + Count: 20, + Sum: 99.23, + Schema: 1, + PositiveSpans: []histogram.Span{ + {Offset: 0, Length: 2}, + {Offset: 1, Length: 2}, + }, + PositiveBuckets: []int64{2, 2, -2, 0}, + NegativeSpans: []histogram.Span{ + {Offset: 0, Length: 2}, + {Offset: 1, Length: 2}, + }, + NegativeBuckets: []int64{2, 2, -2, 0}, + } app := db.Appender(context.Background()) for i := 0; i < 6; i++ { l := labels.FromStrings("__name__", "test_metric", "foo", fmt.Sprintf("%d", i)) expL := labels.FromStrings("__name__", "test_metric", "instance", "", "foo", fmt.Sprintf("%d", i)) - if i%3 == 0 { + switch i { + case 0, 3: _, err = app.Append(0, l, 100*60*1000, float64(i*100)) expVec = append(expVec, promql.Sample{ T: 100 * 60 * 1000, F: float64(i * 100), Metric: expL, }) - } else { + case 4: + _, err = app.AppendHistogram(0, l, 100*60*1000, histWithoutZeroBucket.Copy(), nil) + expVec = append(expVec, promql.Sample{ + T: 100 * 60 * 1000, + H: histWithoutZeroBucket.ToFloat(), + Metric: expL, + }) + default: hist.ZeroCount++ _, err = app.AppendHistogram(0, l, 100*60*1000, hist.Copy(), nil) expVec = append(expVec, promql.Sample{ From 071f4bbea44c0cc37358573a181f616dbeeede2c Mon Sep 17 00:00:00 2001 From: beorn7 Date: Wed, 19 Jul 2023 00:59:41 +0200 Subject: [PATCH 2/2] histograms: Fix parsing float histograms without zero bucket If a float histogram has a zero bucket with a threshold of zero _and_ an empty zero bucket, it wasn't identified as a native histogram because the `isNativeHistogram` helper function only looked at integer buckets. Signed-off-by: beorn7 --- model/textparse/protobufparse.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/model/textparse/protobufparse.go b/model/textparse/protobufparse.go index 2ef52da5c..29ccdb84d 100644 --- a/model/textparse/protobufparse.go +++ b/model/textparse/protobufparse.go @@ -105,7 +105,7 @@ func (p *ProtobufParser) Series() ([]byte, *int64, float64) { v = float64(s.GetSampleCount()) case -1: v = s.GetSampleSum() - // Need to detect a summaries without quantile here. + // Need to detect summaries without quantile here. if len(s.GetQuantile()) == 0 { p.fieldsDone = true } @@ -564,8 +564,10 @@ func formatOpenMetricsFloat(f float64) string { // deciding if a histogram should be ingested as a conventional one or a native // one. func isNativeHistogram(h *dto.Histogram) bool { - return len(h.GetNegativeDelta()) > 0 || - len(h.GetPositiveDelta()) > 0 || + return h.GetZeroThreshold() > 0 || h.GetZeroCount() > 0 || - h.GetZeroThreshold() > 0 + len(h.GetNegativeDelta()) > 0 || + len(h.GetPositiveDelta()) > 0 || + len(h.GetNegativeCount()) > 0 || + len(h.GetPositiveCount()) > 0 }