From 281e23d9a3ef836ade6ba527547d44bbb0dfc8db Mon Sep 17 00:00:00 2001 From: Lukasz Mierzwa Date: Thu, 2 Apr 2026 11:46:51 +0100 Subject: [PATCH] Fix query/cost suggestions --- docs/changelog.md | 1 + internal/checks/query_cost.go | 5 +++ internal/checks/query_cost_test.go | 53 ++++++++++++++++++++++++++++ internal/checks/query_cost_test.snap | 5 +++ 4 files changed, 64 insertions(+) diff --git a/docs/changelog.md b/docs/changelog.md index 84c30ad2..3c49a580 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -7,6 +7,7 @@ - [promql/vector_matching](checks/promql/vector_matching.md) check no longer reports problems that are also reported by [promql/impossible](checks/promql/impossible.md). - Fixed problem description in [promql/series](checks/promql/series.md) check for disable comments that have no effect. +- Fixed invalid suggestions from the [query/cost](checks/query/cost.md) check. ## v0.79.0 diff --git a/internal/checks/query_cost.go b/internal/checks/query_cost.go index 99a60ee9..0eaee3f2 100644 --- a/internal/checks/query_cost.go +++ b/internal/checks/query_cost.go @@ -478,6 +478,11 @@ func (c CostCheck) isSuggestionFor(src, potential source.Source, join *source.Jo goto NEXT } } + for _, name := range src.TransformedLabels(source.PossibleLabel, source.GuaranteedLabel) { + if !potential.CanHaveLabel(name) { + goto NEXT + } + } var extra string if len(lms) > 0 { var buf strings.Builder diff --git a/internal/checks/query_cost_test.go b/internal/checks/query_cost_test.go index 30a7d724..2bc00a6e 100644 --- a/internal/checks/query_cost_test.go +++ b/internal/checks/query_cost_test.go @@ -1705,6 +1705,59 @@ func TestCostCheck(t *testing.T) { }, }, }, + { + // Query uses by(instance) aggregation but the recording rule uses without(instance), + // suggesting this rule would produce different label sets and is not a valid substitution. + description: "suggest recording rule / by vs without mismatch", + content: "- alert: foo\n expr: sum by(instance) (rate(foo_total[5m])) > 10\n", + checker: func(prom *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewCostCheck(prom, 100, 100, 0, 0, "check comment", checks.Warning) + }, + prometheus: newSimpleProm, + entries: mustParseContent(` +- record: colo:foo + expr: sum(rate(foo_total[5m])) without(instance) +`), + mocks: []*prometheusMock{ + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(\nsum by(instance) (rate(foo_total[5m])) > 10\n)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + }, + stats: promapi.QueryStats{ + Samples: promapi.QuerySamples{ + TotalQueryableSamples: 99, + PeakSamples: 19, + }, + Timings: promapi.QueryTimings{ + EvalTotalTime: 60.3, + }, + }, + }, + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: checks.BytesPerSampleQuery}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSampleWithValue(map[string]string{}, 2048), + }, + }, + }, + }, + }, { description: "comments at the end", content: ` diff --git a/internal/checks/query_cost_test.snap b/internal/checks/query_cost_test.snap index 8da3462e..14e13e30 100755 --- a/internal/checks/query_cost_test.snap +++ b/internal/checks/query_cost_test.snap @@ -397,6 +397,11 @@ --- +[TestCostCheck/suggest_recording_rule_/_by_vs_without_mismatch - 1] +[] + +--- + [TestCostCheck/suggest_recording_rule_/_complex - 1] - description: suggest recording rule / complex content: |