From eb3b349024de77bb57499fddf3a7f55b449f95cf Mon Sep 17 00:00:00 2001 From: George Krajcsovits Date: Mon, 28 Oct 2024 08:31:43 +0100 Subject: [PATCH] fix(nhcb): created timestamp fails when keeping classic histograms (#15218) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The wrong source was used to return the created timestamp, leading to index out of bound panic. One line fix. Refactor the requirement test to be generic and be able to test OpenMetrics and Prom parsers as well. There are some differencies in what the parsers support, the Prom parser doesn't have created timestamp. The protobuf parser uses different formatting to identify the metric for the scrape loop. Each parser represents the sample timestamp differently. Signed-off-by: György Krajcsovits --- model/textparse/nhcbparse.go | 2 +- model/textparse/nhcbparse_test.go | 303 +++++++++++++++++++----------- 2 files changed, 191 insertions(+), 114 deletions(-) diff --git a/model/textparse/nhcbparse.go b/model/textparse/nhcbparse.go index 79f5c892a..d019c327c 100644 --- a/model/textparse/nhcbparse.go +++ b/model/textparse/nhcbparse.go @@ -169,7 +169,7 @@ func (p *NHCBParser) CreatedTimestamp() *int64 { return p.parser.CreatedTimestamp() } case stateCollecting: - return p.parser.CreatedTimestamp() + return p.tempCT case stateEmitting: return p.ctNHCB } diff --git a/model/textparse/nhcbparse_test.go b/model/textparse/nhcbparse_test.go index b97de0f7e..6152a8503 100644 --- a/model/textparse/nhcbparse_test.go +++ b/model/textparse/nhcbparse_test.go @@ -524,9 +524,6 @@ something_bucket{a="b",le="+Inf"} 9 # {id="something-test"} 2e100 123.000 // "classic" means the option "always_scrape_classic_histograms". // "nhcb" means the option "convert_classic_histograms_to_nhcb". // -// Currently only with the ProtoBuf parser that supports exponential -// histograms. -// // Case 1. Only classic histogram is exposed. // // | Scrape Config | Expect classic | Expect exponential | Expect NHCB |. @@ -550,7 +547,7 @@ something_bucket{a="b",le="+Inf"} 9 # {id="something-test"} 2e100 123.000 // | classic=true, nhcb=false | NO | YES | NO |. // | classic=false, nhcb=true | NO | YES | NO |. // | classic=true, nhcb=true | NO | YES | NO |. -func TestNHCBParserProtoBufParser_NoNHCBWhenExponential(t *testing.T) { +func TestNHCBParser_NoNHCBWhenExponential(t *testing.T) { type requirement struct { expectClassic bool expectExponential bool @@ -581,134 +578,190 @@ func TestNHCBParserProtoBufParser_NoNHCBWhenExponential(t *testing.T) { }, } + // Create parser from keep classic option. + type parserFactory func(bool) Parser + type testCase struct { name string + parser parserFactory classic bool nhcb bool exp []parsedEntry } + type parserOptions struct { + useUTF8sep bool + hasCreatedTimeStamp bool + } + // Defines the parser name, the Parser factory and the test cases + // supported by the parser and parser options. + parsers := []func() (string, parserFactory, []int, parserOptions){ + func() (string, parserFactory, []int, parserOptions) { + factory := func(keepClassic bool) Parser { + inputBuf := createTestProtoBufHistogram(t) + return NewProtobufParser(inputBuf.Bytes(), keepClassic, labels.NewSymbolTable()) + } + return "ProtoBuf", factory, []int{1, 2, 3}, parserOptions{useUTF8sep: true, hasCreatedTimeStamp: true} + }, + func() (string, parserFactory, []int, parserOptions) { + factory := func(keepClassic bool) Parser { + input := createTestOpenMetricsHistogram() + return NewOpenMetricsParser([]byte(input), labels.NewSymbolTable(), WithOMParserCTSeriesSkipped()) + } + return "OpenMetrics", factory, []int{1}, parserOptions{hasCreatedTimeStamp: true} + }, + func() (string, parserFactory, []int, parserOptions) { + factory := func(keepClassic bool) Parser { + input := createTestPromHistogram() + return NewPromParser([]byte(input), labels.NewSymbolTable()) + } + return "Prometheus", factory, []int{1}, parserOptions{} + }, + } + testCases := []testCase{} - for _, classic := range []bool{false, true} { - for _, nhcb := range []bool{false, true} { - tc := testCase{ - name: "classic=" + strconv.FormatBool(classic) + ", nhcb=" + strconv.FormatBool(nhcb), - classic: classic, - nhcb: nhcb, - exp: []parsedEntry{}, - } - for i, caseI := range cases { - req := caseI[tc.name] - metric := "test_histogram" + strconv.Itoa(i+1) - tc.exp = append(tc.exp, parsedEntry{ - m: metric, - help: "Test histogram " + strconv.Itoa(i+1), - }) - tc.exp = append(tc.exp, parsedEntry{ - m: metric, - typ: model.MetricTypeHistogram, - }) - if req.expectExponential { - // Always expect exponential histogram first. - exponentialSeries := []parsedEntry{ - { - m: metric, - shs: &histogram.Histogram{ - Schema: 3, - Count: 175, - Sum: 0.0008280461746287094, - ZeroThreshold: 2.938735877055719e-39, - ZeroCount: 2, - 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}, + for _, parser := range parsers { + for _, classic := range []bool{false, true} { + for _, nhcb := range []bool{false, true} { + parserName, parser, supportedCases, options := parser() + requirementName := "classic=" + strconv.FormatBool(classic) + ", nhcb=" + strconv.FormatBool(nhcb) + tc := testCase{ + name: "parser=" + parserName + ", " + requirementName, + parser: parser, + classic: classic, + nhcb: nhcb, + exp: []parsedEntry{}, + } + for _, caseNumber := range supportedCases { + caseI := cases[caseNumber-1] + req, ok := caseI[requirementName] + require.True(t, ok, "Case %d does not have requirement %s", caseNumber, requirementName) + metric := "test_histogram" + strconv.Itoa(caseNumber) + tc.exp = append(tc.exp, parsedEntry{ + m: metric, + help: "Test histogram " + strconv.Itoa(caseNumber), + }) + tc.exp = append(tc.exp, parsedEntry{ + m: metric, + typ: model.MetricTypeHistogram, + }) + + var ct *int64 + if options.hasCreatedTimeStamp { + ct = int64p(1000) + } + + var bucketForMetric func(string) string + if options.useUTF8sep { + bucketForMetric = func(s string) string { + return "_bucket\xffle\xff" + s + } + } else { + bucketForMetric = func(s string) string { + return "_bucket{le=\"" + s + "\"}" + } + } + + if req.expectExponential { + // Always expect exponential histogram first. + exponentialSeries := []parsedEntry{ + { + m: metric, + shs: &histogram.Histogram{ + Schema: 3, + Count: 175, + Sum: 0.0008280461746287094, + ZeroThreshold: 2.938735877055719e-39, + ZeroCount: 2, + 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}, + }, + lset: labels.FromStrings("__name__", metric), + t: int64p(1234568), + ct: ct, }, - lset: labels.FromStrings("__name__", metric), - t: int64p(1234568), - ct: int64p(1000), - }, + } + tc.exp = append(tc.exp, exponentialSeries...) } - tc.exp = append(tc.exp, exponentialSeries...) - } - if req.expectClassic { - // Always expect classic histogram series after exponential. - classicSeries := []parsedEntry{ - { - m: metric + "_count", - v: 175, - lset: labels.FromStrings("__name__", metric+"_count"), - t: int64p(1234568), - ct: int64p(1000), - }, - { - m: metric + "_sum", - v: 0.0008280461746287094, - lset: labels.FromStrings("__name__", metric+"_sum"), - t: int64p(1234568), - ct: int64p(1000), - }, - { - m: metric + "_bucket\xffle\xff-0.0004899999999999998", - v: 2, - lset: labels.FromStrings("__name__", metric+"_bucket", "le", "-0.0004899999999999998"), - t: int64p(1234568), - ct: int64p(1000), - }, - { - m: metric + "_bucket\xffle\xff-0.0003899999999999998", - v: 4, - lset: labels.FromStrings("__name__", metric+"_bucket", "le", "-0.0003899999999999998"), - t: int64p(1234568), - ct: int64p(1000), - }, - { - m: metric + "_bucket\xffle\xff-0.0002899999999999998", - v: 16, - lset: labels.FromStrings("__name__", metric+"_bucket", "le", "-0.0002899999999999998"), - t: int64p(1234568), - ct: int64p(1000), - }, - { - m: metric + "_bucket\xffle\xff+Inf", - v: 175, - lset: labels.FromStrings("__name__", metric+"_bucket", "le", "+Inf"), - t: int64p(1234568), - ct: int64p(1000), - }, - } - tc.exp = append(tc.exp, classicSeries...) - } - if req.expectNHCB { - // Always expect NHCB series after classic. - nhcbSeries := []parsedEntry{ - { - m: metric + "{}", - shs: &histogram.Histogram{ - Schema: histogram.CustomBucketsSchema, - Count: 175, - Sum: 0.0008280461746287094, - PositiveSpans: []histogram.Span{{Length: 4}}, - PositiveBuckets: []int64{2, 0, 10, 147}, - CustomValues: []float64{-0.0004899999999999998, -0.0003899999999999998, -0.0002899999999999998}, + if req.expectClassic { + // Always expect classic histogram series after exponential. + classicSeries := []parsedEntry{ + { + m: metric + "_count", + v: 175, + lset: labels.FromStrings("__name__", metric+"_count"), + t: int64p(1234568), + ct: ct, }, - lset: labels.FromStrings("__name__", metric), - t: int64p(1234568), - ct: int64p(1000), - }, + { + m: metric + "_sum", + v: 0.0008280461746287094, + lset: labels.FromStrings("__name__", metric+"_sum"), + t: int64p(1234568), + ct: ct, + }, + { + m: metric + bucketForMetric("-0.0004899999999999998"), + v: 2, + lset: labels.FromStrings("__name__", metric+"_bucket", "le", "-0.0004899999999999998"), + t: int64p(1234568), + ct: ct, + }, + { + m: metric + bucketForMetric("-0.0003899999999999998"), + v: 4, + lset: labels.FromStrings("__name__", metric+"_bucket", "le", "-0.0003899999999999998"), + t: int64p(1234568), + ct: ct, + }, + { + m: metric + bucketForMetric("-0.0002899999999999998"), + v: 16, + lset: labels.FromStrings("__name__", metric+"_bucket", "le", "-0.0002899999999999998"), + t: int64p(1234568), + ct: ct, + }, + { + m: metric + bucketForMetric("+Inf"), + v: 175, + lset: labels.FromStrings("__name__", metric+"_bucket", "le", "+Inf"), + t: int64p(1234568), + ct: ct, + }, + } + tc.exp = append(tc.exp, classicSeries...) + } + if req.expectNHCB { + // Always expect NHCB series after classic. + nhcbSeries := []parsedEntry{ + { + m: metric + "{}", + shs: &histogram.Histogram{ + Schema: histogram.CustomBucketsSchema, + Count: 175, + Sum: 0.0008280461746287094, + PositiveSpans: []histogram.Span{{Length: 4}}, + PositiveBuckets: []int64{2, 0, 10, 147}, + CustomValues: []float64{-0.0004899999999999998, -0.0003899999999999998, -0.0002899999999999998}, + }, + lset: labels.FromStrings("__name__", metric), + t: int64p(1234568), + ct: ct, + }, + } + tc.exp = append(tc.exp, nhcbSeries...) } - tc.exp = append(tc.exp, nhcbSeries...) } + testCases = append(testCases, tc) } - testCases = append(testCases, tc) } } - inputBuf := createTestProtoBufHistogram(t) - for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - p := NewProtobufParser(inputBuf.Bytes(), tc.classic, labels.NewSymbolTable()) + p := tc.parser(tc.classic) if tc.nhcb { p = NewNHCBParser(p, labels.NewSymbolTable(), tc.classic) } @@ -860,3 +913,27 @@ metric: < return buf } + +func createTestOpenMetricsHistogram() string { + return `# HELP test_histogram1 Test histogram 1 +# TYPE test_histogram1 histogram +test_histogram1_count 175 1234.568 +test_histogram1_sum 0.0008280461746287094 1234.568 +test_histogram1_bucket{le="-0.0004899999999999998"} 2 1234.568 +test_histogram1_bucket{le="-0.0003899999999999998"} 4 1234.568 +test_histogram1_bucket{le="-0.0002899999999999998"} 16 1234.568 +test_histogram1_bucket{le="+Inf"} 175 1234.568 +test_histogram1_created 1 +# EOF` +} + +func createTestPromHistogram() string { + return `# HELP test_histogram1 Test histogram 1 +# TYPE test_histogram1 histogram +test_histogram1_count 175 1234568 +test_histogram1_sum 0.0008280461746287094 1234768 +test_histogram1_bucket{le="-0.0004899999999999998"} 2 1234568 +test_histogram1_bucket{le="-0.0003899999999999998"} 4 1234568 +test_histogram1_bucket{le="-0.0002899999999999998"} 16 1234568 +test_histogram1_bucket{le="+Inf"} 175 1234568` +}