Skip to content

Commit

Permalink
fix(nhcb): created timestamp fails when keeping classic histograms (p…
Browse files Browse the repository at this point in the history 
…rometheus#15218)

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 <gyorgy.krajcsovits@grafana.com>
  • Loading branch information
@krajorama
krajorama authored Oct 28, 2024
1 parent bab587b commit eb3b349
Show file tree
Hide file tree
Showing 2 changed files with 190 additions and 113 deletions.
2 changes: 1 addition & 1 deletion model/textparse/nhcbparse.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
301 changes: 189 additions & 112 deletions model/textparse/nhcbparse_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 |.
Expand All @@ -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
Expand Down Expand Up @@ -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
}

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{},
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())
}
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},
return "Prometheus", factory, []int{1}, parserOptions{}
},
}

testCases := []testCase{}
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),
},
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,
},
{
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...)
}
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.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,
},
lset: labels.FromStrings("__name__", metric),
t: int64p(1234568),
ct: int64p(1000),
},
}
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)
}
Expand Down Expand Up @@ -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`
}

0 comments on commit eb3b349

Please sign in to comment.