From ee1fe2e9f0231b160a4b00d25f5dcaaf8ca2d3b4 Mon Sep 17 00:00:00 2001 From: Lukasz Mierzwa Date: Fri, 22 Mar 2024 15:27:50 +0000 Subject: [PATCH] Fix query/cost reporting --- docs/changelog.md | 7 ++ internal/checks/query_cost.go | 35 +++++---- internal/checks/query_cost_test.go | 116 +++++++++++------------------ 3 files changed, 72 insertions(+), 86 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index 84504f30..2e1ea50c 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -1,5 +1,12 @@ # Changelog +## v0.56.2 + +### Fixed + +- [query/cost](checks/query/cost.md) will now only create reports if a query is more + expensive than any of the configured limits. + ## v0.56.1 ### Fixed diff --git a/internal/checks/query_cost.go b/internal/checks/query_cost.go index 3766deaa..ece99c42 100644 --- a/internal/checks/query_cost.go +++ b/internal/checks/query_cost.go @@ -98,23 +98,17 @@ func (c CostCheck) Check(ctx context.Context, _ string, rule parser.Rule, _ []di } } - var above string - severity := Information if c.maxSeries > 0 && series > c.maxSeries { - severity = c.severity - above = fmt.Sprintf(", maximum allowed series is %d.", c.maxSeries) - } else { - estimate += "." + problems = append(problems, Problem{ + Lines: expr.Value.Lines, + Reporter: c.Reporter(), + Text: fmt.Sprintf("%s returned %d result(s)%s, maximum allowed series is %d.", promText(c.prom.Name(), qr.URI), series, estimate, c.maxSeries), + Details: maybeComment(c.comment), + Severity: c.severity, + }) + return problems } - problems = append(problems, Problem{ - Lines: expr.Value.Lines, - Reporter: c.Reporter(), - Text: fmt.Sprintf("%s returned %d result(s)%s%s", promText(c.prom.Name(), qr.URI), series, estimate, above), - Details: maybeComment(c.comment), - Severity: severity, - }) - if c.maxTotalSamples > 0 && qr.Stats.Samples.TotalQueryableSamples > c.maxTotalSamples { problems = append(problems, Problem{ Lines: expr.Value.Lines, @@ -123,6 +117,7 @@ func (c CostCheck) Check(ctx context.Context, _ string, rule parser.Rule, _ []di Details: maybeComment(c.comment), Severity: c.severity, }) + return problems } if c.maxPeakSamples > 0 && qr.Stats.Samples.PeakSamples > c.maxPeakSamples { @@ -133,6 +128,7 @@ func (c CostCheck) Check(ctx context.Context, _ string, rule parser.Rule, _ []di Details: maybeComment(c.comment), Severity: c.severity, }) + return problems } evalDur := time.Duration(qr.Stats.Timings.EvalTotalTime * float64(time.Second)) @@ -144,6 +140,17 @@ func (c CostCheck) Check(ctx context.Context, _ string, rule parser.Rule, _ []di Details: maybeComment(c.comment), Severity: c.severity, }) + return problems + } + + if series > 0 && c.maxSeries == 0 && c.maxTotalSamples == 0 && c.maxPeakSamples == 0 && c.maxEvaluationDuration == 0 { + problems = append(problems, Problem{ + Lines: expr.Value.Lines, + Reporter: c.Reporter(), + Text: fmt.Sprintf("%s returned %d result(s)%s.", promText(c.prom.Name(), qr.URI), series, estimate), + Details: maybeComment(c.comment), + Severity: Information, + }) } return problems diff --git a/internal/checks/query_cost_test.go b/internal/checks/query_cost_test.go index b1e71371..4e2917e5 100644 --- a/internal/checks/query_cost_test.go +++ b/internal/checks/query_cost_test.go @@ -56,19 +56,7 @@ func TestCostCheck(t *testing.T) { return checks.NewCostCheck(prom, 0, 0, 0, 0, "", checks.Bug) }, prometheus: newSimpleProm, - problems: func(uri string) []checks.Problem { - return []checks.Problem{ - { - Lines: parser.LineRange{ - First: 2, - Last: 2, - }, - Reporter: "query/cost", - Text: costText("prom", uri, 0) + ".", - Severity: checks.Information, - }, - } - }, + problems: noProblems, mocks: []*prometheusMock{ { conds: []requestCondition{ @@ -513,19 +501,7 @@ func TestCostCheck(t *testing.T) { return checks.NewCostCheck(prom, 0, 0, 0, time.Second*5, "", checks.Bug) }, prometheus: newSimpleProm, - problems: func(uri string) []checks.Problem { - return []checks.Problem{ - { - Lines: parser.LineRange{ - First: 2, - Last: 2, - }, - Reporter: "query/cost", - Text: costText("prom", uri, 1) + memUsageText("4.0KiB") + ".", - Severity: checks.Information, - }, - } - }, + problems: noProblems, mocks: []*prometheusMock{ { conds: []requestCondition{ @@ -563,15 +539,6 @@ func TestCostCheck(t *testing.T) { prometheus: newSimpleProm, problems: func(uri string) []checks.Problem { return []checks.Problem{ - { - Lines: parser.LineRange{ - First: 2, - Last: 2, - }, - Reporter: "query/cost", - Text: costText("prom", uri, 1) + memUsageText("4.0KiB") + ".", - Severity: checks.Information, - }, { Lines: parser.LineRange{ First: 2, @@ -581,24 +548,6 @@ func TestCostCheck(t *testing.T) { Text: totalSamplesText("prom", uri, 200, 100), Severity: checks.Bug, }, - { - Lines: parser.LineRange{ - First: 2, - Last: 2, - }, - Reporter: "query/cost", - Text: peakSamplesText("prom", uri, 20, 10), - Severity: checks.Bug, - }, - { - Lines: parser.LineRange{ - First: 2, - Last: 2, - }, - Reporter: "query/cost", - Text: evalDurText("prom", uri, "5s100ms", "5s"), - Severity: checks.Bug, - }, } }, mocks: []*prometheusMock{ @@ -634,10 +583,10 @@ func TestCostCheck(t *testing.T) { }, }, { - description: "stats - info", + description: "stats - peak samples", content: content, checker: func(prom *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewCostCheck(prom, 0, 100, 10, time.Second*5, "some text", checks.Information) + return checks.NewCostCheck(prom, 0, 300, 10, time.Second*5, "some text", checks.Information) }, prometheus: newSimpleProm, problems: func(uri string) []checks.Problem { @@ -648,30 +597,53 @@ func TestCostCheck(t *testing.T) { Last: 2, }, Reporter: "query/cost", - Text: costText("prom", uri, 1) + memUsageText("4.0KiB") + ".", + Text: peakSamplesText("prom", uri, 20, 10), Details: "Rule comment: some text", Severity: checks.Information, }, - { - Lines: parser.LineRange{ - First: 2, - Last: 2, + } + }, + mocks: []*prometheusMock{ + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: `count(sum(foo))`}, + }, + resp: vectorResponse{ + samples: []*model.Sample{generateSample(map[string]string{})}, + stats: promapi.QueryStats{ + Timings: promapi.QueryTimings{ + EvalTotalTime: 5.1, + }, + Samples: promapi.QuerySamples{ + TotalQueryableSamples: 200, + PeakSamples: 20, + }, }, - Reporter: "query/cost", - Text: totalSamplesText("prom", uri, 200, 100), - Details: "Rule comment: some text", - Severity: checks.Information, }, - { - Lines: parser.LineRange{ - First: 2, - Last: 2, + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: checks.BytesPerSampleQuery}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSampleWithValue(map[string]string{}, 4096), }, - Reporter: "query/cost", - Text: peakSamplesText("prom", uri, 20, 10), - Details: "Rule comment: some text", - Severity: checks.Information, }, + }, + }, + }, + { + description: "stats - duration", + content: content, + checker: func(prom *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewCostCheck(prom, 0, 300, 30, time.Second*5, "some text", checks.Information) + }, + prometheus: newSimpleProm, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ { Lines: parser.LineRange{ First: 2,