diff --git a/docs/changelog.md b/docs/changelog.md index e91ec6c9..060a16d9 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -1,5 +1,11 @@ # Changelog +## v0.80.0 + +### Fixed + +- [promql/vector_matching](checks/promql/vector_matching.md) check no longer reports problems that are also reported by [promql/impossible](checks/promql/impossible.md). + ## v0.79.0 ### Changed diff --git a/internal/checks/promql_impossible_test.snap b/internal/checks/promql_impossible_test.snap index 6ed2e44f..cde631d4 100755 --- a/internal/checks/promql_impossible_test.snap +++ b/internal/checks/promql_impossible_test.snap @@ -2513,11 +2513,6 @@ --- -[TestImpossibleCheck/match_on_group_right_label - 1] -[] - ---- - [TestImpossibleCheck/max_by()_unless - 1] [] diff --git a/internal/checks/promql_vector_matching.go b/internal/checks/promql_vector_matching.go index 0c144543..6df3600e 100644 --- a/internal/checks/promql_vector_matching.go +++ b/internal/checks/promql_vector_matching.go @@ -139,6 +139,12 @@ func (c VectorMatchingCheck) checkNode(ctx context.Context, rule parser.Rule, ex } } + lhsSources := source.LabelsSource(expr.Value.Value, node.Expr.(*promParser.BinaryExpr).LHS) + rhsSources := source.LabelsSource(expr.Value.Value, node.Expr.(*promParser.BinaryExpr).RHS) + if !canJoinStatically(lhsSources, rhsSources, n.VectorMatching) { + goto NEXT + } + leftLabels, leftURI, err := c.seriesLabels(ctx, n.LHS.String(), ignored...) if err != nil { problems = append(problems, problemFromError(err, rule, c.Reporter(), c.prom.Name(), Bug)) @@ -284,6 +290,39 @@ NEXT: return problems } +func canJoinStatically(lhsSources, rhsSources []source.Source, vm *promParser.VectorMatching) bool { + for _, ls := range lhsSources { + for _, rs := range rhsSources { + switch { + case vm.On && len(vm.MatchingLabels) == 0: + return true + case vm.On: + for _, name := range vm.MatchingLabels { + if ls.CanHaveLabel(name) && !rs.CanHaveLabel(name) { + return false + } + if rs.CanHaveLabel(name) && !ls.CanHaveLabel(name) { + return false + } + } + default: + for name, l := range ls.Labels { + if l.Kind != source.GuaranteedLabel { + continue + } + if slices.Contains(vm.MatchingLabels, name) { + continue + } + if !rs.CanHaveLabel(name) { + return false + } + } + } + } + } + return true +} + func (c VectorMatchingCheck) seriesLabels(ctx context.Context, query string, ignored ...model.LabelName) (labelSets, string, error) { var expr strings.Builder expr.WriteString(wrapExpr(query, "count")) diff --git a/internal/checks/promql_vector_matching_test.go b/internal/checks/promql_vector_matching_test.go index 1c4efdd1..46e21cef 100644 --- a/internal/checks/promql_vector_matching_test.go +++ b/internal/checks/promql_vector_matching_test.go @@ -1178,6 +1178,106 @@ func TestVectorMatchingCheck(t *testing.T) { }, problems: true, }, + // Verifies that promql_vector_matching doesn't report when the join is statically + // impossible - the promql_impossible check already handles this case. + // sum by(instance) removes job from LHS, but on(instance, job) requires it. + { + description: "skip when join is statically impossible via on()", + content: "- record: foo\n expr: sum by(instance) (foo) * on(instance, job) bar\n", + checker: newVectorMatchingCheck, + prometheus: newSimpleProm, + mocks: []*prometheusMock{ + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(\nsum by (instance) (foo) * on (instance, job) bar\n)"}, + }, + resp: respondWithEmptyVector(), + }, + }, + }, + // Verifies that vector_matching doesn't report when the join is statically + // impossible due to label mismatch without on/ignoring modifiers. + // LHS has instance as guaranteed label via the selector, but sum(bar) + // removes all labels making instance structurally impossible on RHS. + { + description: "skip when join is statically impossible via guaranteed labels", + content: "- record: foo\n expr: foo{instance=\"a\"} / sum(bar)\n", + checker: newVectorMatchingCheck, + prometheus: newSimpleProm, + mocks: []*prometheusMock{ + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(\nfoo{instance=\"a\"} / sum(bar)\n)"}, + }, + resp: respondWithEmptyVector(), + }, + }, + }, + // Verifies that vector_matching skips when on() label is structurally + // impossible on the RHS. LHS (foo) can have job, but RHS sum by(instance) + // removes job, hitting the ls.CanHaveLabel && !rs.CanHaveLabel branch. + { + description: "skip when on() label is statically impossible on RHS", + content: "- record: foo\n expr: foo * on(instance, job) sum by(instance) (bar)\n", + checker: newVectorMatchingCheck, + prometheus: newSimpleProm, + mocks: []*prometheusMock{ + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(\nfoo * on (instance, job) sum by (instance) (bar)\n)"}, + }, + resp: respondWithEmptyVector(), + }, + }, + }, + // Verifies that ignoring() with a guaranteed label in the ignore list + // doesn't cause a false skip. LHS guarantees instance via the selector, + // and instance is in the ignoring() list so it should be excluded from + // the label comparison, allowing the check to proceed normally. + { + description: "ignoring() skips guaranteed label in ignore list", + content: "- record: foo\n expr: foo{instance=\"a\"} / ignoring(instance) bar\n", + checker: newVectorMatchingCheck, + prometheus: newSimpleProm, + mocks: []*prometheusMock{ + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(\nfoo{instance=\"a\"} / ignoring (instance) bar\n)"}, + }, + resp: respondWithEmptyVector(), + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(\nfoo{instance=\"a\"}\n) without(__name__,instance)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "job": "aaa", + }), + }, + }, + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(\nbar\n) without(__name__,instance)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "job": "aaa", + }), + }, + }, + }, + }, + }, } runTests(t, testCases) } diff --git a/internal/checks/promql_vector_matching_test.snap b/internal/checks/promql_vector_matching_test.snap index fc103706..4a2110ae 100755 --- a/internal/checks/promql_vector_matching_test.snap +++ b/internal/checks/promql_vector_matching_test.snap @@ -190,6 +190,11 @@ --- +[TestVectorMatchingCheck/ignoring()_skips_guaranteed_label_in_ignore_list - 1] +[] + +--- + [TestVectorMatchingCheck/max()_by(a)_and_on(a)_foo - 1] - description: max() by(a) and on(a) foo content: "\n- record: foo\n expr: |\n max by (cluster, env) (\n increase(error_total{}[10m])\n ) > 0\n and on (cluster)\n cluster_metadata{cluster=\"foo\", environment=\"prod\"} > 0\n " @@ -400,6 +405,21 @@ --- +[TestVectorMatchingCheck/skip_when_join_is_statically_impossible_via_guaranteed_labels - 1] +[] + +--- + +[TestVectorMatchingCheck/skip_when_join_is_statically_impossible_via_on() - 1] +[] + +--- + +[TestVectorMatchingCheck/skip_when_on()_label_is_statically_impossible_on_RHS - 1] +[] + +--- + [TestVectorMatchingCheck/skips_number_comparison_on_LHS - 1] []