From 2a781ec5ac5643f14605ed37078a6daa5189caba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Mon, 21 Aug 2023 13:12:45 +0200 Subject: [PATCH 1/6] Replicate infinite loop in native-classic histogram scrape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enable scraping a native histogram with exemplars that leads to infinite loop. Signed-off-by: György Krajcsovits --- scrape/scrape_test.go | 116 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 108 insertions(+), 8 deletions(-) diff --git a/scrape/scrape_test.go b/scrape/scrape_test.go index 8578f1bec6..1a3d837553 100644 --- a/scrape/scrape_test.go +++ b/scrape/scrape_test.go @@ -1982,13 +1982,14 @@ func TestScrapeLoopAppendNoStalenessIfTimestamp(t *testing.T) { func TestScrapeLoopAppendExemplar(t *testing.T) { tests := []struct { - title string - scrapeText string - contentType string - discoveryLabels []string - floats []floatSample - histograms []histogramSample - exemplars []exemplar.Exemplar + title string + scrapeClassicHistograms bool + scrapeText string + contentType string + discoveryLabels []string + floats []floatSample + histograms []histogramSample + exemplars []exemplar.Exemplar }{ { title: "Metric without exemplars", @@ -2142,6 +2143,105 @@ metric: < {Labels: labels.FromStrings("dummyID", "5617"), Value: -0.00029, Ts: 1234568, HasTs: false}, }, }, + { + title: "Native histogram with two exemplars scraped as classic histogram", + scrapeText: `name: "test_histogram" +help: "Test histogram with many buckets removed to keep it manageable in size." +type: HISTOGRAM +metric: < + histogram: < + sample_count: 175 + sample_sum: 0.0008280461746287094 + bucket: < + cumulative_count: 2 + upper_bound: -0.0004899999999999998 + > + bucket: < + cumulative_count: 4 + upper_bound: -0.0003899999999999998 + exemplar: < + label: < + name: "dummyID" + value: "59727" + > + value: -0.00039 + timestamp: < + seconds: 1625851155 + nanos: 146848499 + > + > + > + bucket: < + cumulative_count: 16 + upper_bound: -0.0002899999999999998 + exemplar: < + label: < + name: "dummyID" + value: "5617" + > + value: -0.00029 + > + > + schema: 3 + zero_threshold: 2.938735877055719e-39 + zero_count: 2 + negative_span: < + offset: -162 + length: 1 + > + negative_span: < + offset: 23 + length: 4 + > + negative_delta: 1 + negative_delta: 3 + negative_delta: -2 + negative_delta: -1 + negative_delta: 1 + positive_span: < + offset: -161 + length: 1 + > + positive_span: < + offset: 8 + length: 3 + > + positive_delta: 1 + positive_delta: 2 + positive_delta: -1 + positive_delta: -1 + > + timestamp_ms: 1234568 +> + +`, + scrapeClassicHistograms: true, + contentType: "application/vnd.google.protobuf", + histograms: []histogramSample{{ + t: 1234568, + h: &histogram.Histogram{ + Count: 175, + ZeroCount: 2, + Sum: 0.0008280461746287094, + ZeroThreshold: 2.938735877055719e-39, + Schema: 3, + PositiveSpans: []histogram.Span{ + {Offset: -161, Length: 1}, + {Offset: 8, Length: 3}, + }, + NegativeSpans: []histogram.Span{ + {Offset: -162, Length: 1}, + {Offset: 23, Length: 4}, + }, + PositiveBuckets: []int64{1, 2, -1, -1}, + NegativeBuckets: []int64{1, 3, -2, -1, 1}, + }, + }}, + exemplars: []exemplar.Exemplar{ + {Labels: labels.FromStrings("dummyID", "59727"), Value: -0.00039, Ts: 1625851155146, HasTs: true}, + {Labels: labels.FromStrings("dummyID", "5617"), Value: -0.00029, Ts: 1234568, HasTs: false}, + }, + }, } for _, test := range tests { @@ -2168,7 +2268,7 @@ metric: < nil, 0, 0, - false, + test.scrapeClassicHistograms, false, false, nil, From 2ae8c2bd3d91519b0b342897d1f1e6dc64496348 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Mon, 21 Aug 2023 13:55:13 +0200 Subject: [PATCH 2/6] Set expected values in test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The parsing doesn't seem to be perfect as I don't get all classic buckets possibly another bug found? Signed-off-by: György Krajcsovits --- scrape/scrape_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/scrape/scrape_test.go b/scrape/scrape_test.go index 1a3d837553..3a5d3df066 100644 --- a/scrape/scrape_test.go +++ b/scrape/scrape_test.go @@ -2217,6 +2217,14 @@ metric: < `, scrapeClassicHistograms: true, contentType: "application/vnd.google.protobuf", + floats: []floatSample{ + {metric: labels.FromStrings("__name__", "test_histogram_count"), t: 1234568, f: 175}, + {metric: labels.FromStrings("__name__", "test_histogram_sum"), t: 1234568, f: 0.0008280461746287094}, + {metric: labels.FromStrings("__name__", "test_histogram_bucket", "le", "-0.0004899999999999998"), t: 1234568, f: 2}, + // {metric: labels.FromStrings("__name__", "test_histogram_bucket", "le", "-0.0003899999999999998"), t: 1234568, f: 4}, + // {metric: labels.FromStrings("__name__", "test_histogram_bucket", "le", "-0.0002899999999999998"), t: 1234568, f: 16}, + {metric: labels.FromStrings("__name__", "test_histogram_bucket", "le", "+Inf"), t: 1234568, f: 175}, + }, histograms: []histogramSample{{ t: 1234568, h: &histogram.Histogram{ @@ -2240,6 +2248,8 @@ metric: < exemplars: []exemplar.Exemplar{ {Labels: labels.FromStrings("dummyID", "59727"), Value: -0.00039, Ts: 1625851155146, HasTs: true}, {Labels: labels.FromStrings("dummyID", "5617"), Value: -0.00029, Ts: 1234568, HasTs: false}, + {Labels: labels.FromStrings("dummyID", "59727"), Value: -0.00039, Ts: 1625851155146, HasTs: true}, + {Labels: labels.FromStrings("dummyID", "5617"), Value: -0.00029, Ts: 1234568, HasTs: false}, }, }, } @@ -2278,6 +2288,9 @@ metric: < now := time.Now() for i := range test.floats { + if test.floats[i].t != 0 { + continue + } test.floats[i].t = timestamp.FromTime(now) } From e846736134b53ec982a6f93c4381dc7046c5dd32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Mon, 21 Aug 2023 14:13:49 +0200 Subject: [PATCH 3/6] Fix typo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: György Krajcsovits --- model/textparse/protobufparse.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/model/textparse/protobufparse.go b/model/textparse/protobufparse.go index c111bb0657..a9c040d335 100644 --- a/model/textparse/protobufparse.go +++ b/model/textparse/protobufparse.go @@ -293,7 +293,7 @@ func (p *ProtobufParser) Metric(l *labels.Labels) string { // Exemplar writes the exemplar of the current sample into the passed // exemplar. It returns if an exemplar exists or not. In case of a native // histogram, the legacy bucket section is still used for exemplars. To ingest -// all examplars, call the Exemplar method repeatedly until it returns false. +// all exemplars, call the Exemplar method repeatedly until it returns false. func (p *ProtobufParser) Exemplar(ex *exemplar.Exemplar) bool { m := p.mf.GetMetric()[p.metricPos] var exProto *dto.Exemplar From 983c0c5e9d2b726c96c58414b24942f3ee1b54a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Mon, 21 Aug 2023 14:44:53 +0200 Subject: [PATCH 4/6] Add missing buckets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit My previous proposal for a fix was wrong and also missed these. Signed-off-by: György Krajcsovits --- scrape/scrape_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scrape/scrape_test.go b/scrape/scrape_test.go index 3a5d3df066..b680bb3376 100644 --- a/scrape/scrape_test.go +++ b/scrape/scrape_test.go @@ -2221,8 +2221,8 @@ metric: < {metric: labels.FromStrings("__name__", "test_histogram_count"), t: 1234568, f: 175}, {metric: labels.FromStrings("__name__", "test_histogram_sum"), t: 1234568, f: 0.0008280461746287094}, {metric: labels.FromStrings("__name__", "test_histogram_bucket", "le", "-0.0004899999999999998"), t: 1234568, f: 2}, - // {metric: labels.FromStrings("__name__", "test_histogram_bucket", "le", "-0.0003899999999999998"), t: 1234568, f: 4}, - // {metric: labels.FromStrings("__name__", "test_histogram_bucket", "le", "-0.0002899999999999998"), t: 1234568, f: 16}, + {metric: labels.FromStrings("__name__", "test_histogram_bucket", "le", "-0.0003899999999999998"), t: 1234568, f: 4}, + {metric: labels.FromStrings("__name__", "test_histogram_bucket", "le", "-0.0002899999999999998"), t: 1234568, f: 16}, {metric: labels.FromStrings("__name__", "test_histogram_bucket", "le", "+Inf"), t: 1234568, f: 175}, }, histograms: []histogramSample{{ From 3d9a830f2f545ed64d1dc55438a5b9141a01acdf Mon Sep 17 00:00:00 2001 From: beorn7 Date: Tue, 22 Aug 2023 20:48:52 +0200 Subject: [PATCH 5/6] textparse: Expose #12731 in protobufparse_test.go Signed-off-by: beorn7 --- model/textparse/protobufparse_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/model/textparse/protobufparse_test.go b/model/textparse/protobufparse_test.go index 53523a5dd9..5436d7f3e3 100644 --- a/model/textparse/protobufparse_test.go +++ b/model/textparse/protobufparse_test.go @@ -1779,6 +1779,7 @@ func TestProtobufParse(t *testing.T) { } else { require.Equal(t, true, found, "i: %d", i) require.Equal(t, exp[i].e[0], e, "i: %d", i) + require.False(t, p.Exemplar(&e), "too many exemplars returned, i: %d", i) } case EntryHistogram: From 65ccf4460afc3f83adb231b3ec15073ecc924f14 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Tue, 22 Aug 2023 21:03:54 +0200 Subject: [PATCH 6/6] textparse: Fix endless loop #12731 PR #12557 introduced the possibility of parsing multiple exemplars per native histograms. It did so by requiring the `Exemplar` method of the parser to be called repeatedly until it returns false. However, the protobuf parser code wasn't correctly updated for the old case of a single exemplar for a classic bucket (if actually parsed as a classic bucket) and a single exemplar on a counter. In those cases, the method would return `true` forever, yielding the same exemplar again and again, leading to an endless loop. With this fix, the state is now tracked and the single exemplar is only returned once. Signed-off-by: beorn7 --- model/textparse/protobufparse.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/model/textparse/protobufparse.go b/model/textparse/protobufparse.go index a9c040d335..fbb84a2bd3 100644 --- a/model/textparse/protobufparse.go +++ b/model/textparse/protobufparse.go @@ -56,6 +56,10 @@ type ProtobufParser struct { fieldsDone bool // true if no more fields of a Summary or (legacy) Histogram to be processed. redoClassic bool // true after parsing a native histogram if we need to parse it again as a classic histogram. + // exemplarReturned is set to true each time an exemplar has been + // returned, and set back to false upon each Next() call. + exemplarReturned bool + // state is marked by the entry we are processing. EntryInvalid implies // that we have to decode the next MetricFamily. state Entry @@ -295,6 +299,10 @@ func (p *ProtobufParser) Metric(l *labels.Labels) string { // histogram, the legacy bucket section is still used for exemplars. To ingest // all exemplars, call the Exemplar method repeatedly until it returns false. func (p *ProtobufParser) Exemplar(ex *exemplar.Exemplar) bool { + if p.exemplarReturned && p.state == EntrySeries { + // We only ever return one exemplar per (non-native-histogram) series. + return false + } m := p.mf.GetMetric()[p.metricPos] var exProto *dto.Exemplar switch p.mf.GetType() { @@ -335,6 +343,7 @@ func (p *ProtobufParser) Exemplar(ex *exemplar.Exemplar) bool { } p.builder.Sort() ex.Labels = p.builder.Labels() + p.exemplarReturned = true return true } @@ -342,6 +351,7 @@ func (p *ProtobufParser) Exemplar(ex *exemplar.Exemplar) bool { // text format parser). It returns (EntryInvalid, io.EOF) if no samples were // read. func (p *ProtobufParser) Next() (Entry, error) { + p.exemplarReturned = false switch p.state { case EntryInvalid: p.metricPos = 0