From 454cb49f0a5361217873e312f1a99079225efa23 Mon Sep 17 00:00:00 2001 From: Lukasz Mierzwa Date: Fri, 26 Nov 2021 11:21:14 +0000 Subject: [PATCH 1/2] Improve parse output --- cmd/pint/{debug.go => parse.go} | 8 +++++++- cmd/pint/tests/0015_parse_1.txt | 6 +++++- cmd/pint/tests/0016_parse_2.txt | 6 +++++- 3 files changed, 17 insertions(+), 3 deletions(-) rename cmd/pint/{debug.go => parse.go} (91%) diff --git a/cmd/pint/debug.go b/cmd/pint/parse.go similarity index 91% rename from cmd/pint/debug.go rename to cmd/pint/parse.go index 2d5da7ce..9281fece 100644 --- a/cmd/pint/debug.go +++ b/cmd/pint/parse.go @@ -42,7 +42,13 @@ func parseNode(node parser.Node, level int) { printNode(level, "* Op: %v", n.Op) printNode(level, "* LHS: %v", n.LHS) printNode(level, "* RHS: %v", n.RHS) - printNode(level, "* VectorMatching: %v", n.VectorMatching) + printNode(level, "* VectorMatching:") + if n.VectorMatching != nil { + printNode(level+levelStep, "* Card: %v", n.VectorMatching.Card) + printNode(level+levelStep, "* MatchingLabels: %v", n.VectorMatching.MatchingLabels) + printNode(level+levelStep, "* On: %v", n.VectorMatching.On) + printNode(level+levelStep, "* Include: %v", n.VectorMatching.Include) + } printNode(level, "* ReturnBool: %v", n.ReturnBool) case *parser.EvalStmt: printNode(level, "EvalStmt:") diff --git a/cmd/pint/tests/0015_parse_1.txt b/cmd/pint/tests/0015_parse_1.txt index 1cd4bd17..ba7b6d70 100644 --- a/cmd/pint/tests/0015_parse_1.txt +++ b/cmd/pint/tests/0015_parse_1.txt @@ -9,7 +9,11 @@ cmp stdout stdout.txt * Op: + * LHS: sum without(job) (rate(foo[5m])) * RHS: sum without(job) (rate(bar[5m])) - * VectorMatching: &{one-to-one [] false []} + * VectorMatching: + * Card: one-to-one + * MatchingLabels: [] + * On: false + * Include: [] * ReturnBool: false ++ node: sum without(job) (rate(foo[5m])) AggregateExpr: diff --git a/cmd/pint/tests/0016_parse_2.txt b/cmd/pint/tests/0016_parse_2.txt index 4b940123..fe24caec 100644 --- a/cmd/pint/tests/0016_parse_2.txt +++ b/cmd/pint/tests/0016_parse_2.txt @@ -9,7 +9,11 @@ cmp stdout stdout.txt * Op: / * LHS: sum without(job) (rate(foo[5m])) * RHS: sum without(job) (rate(bar[5m])) - * VectorMatching: &{many-to-one [instance] true []} + * VectorMatching: + * Card: many-to-one + * MatchingLabels: [instance] + * On: true + * Include: [] * ReturnBool: false ++ node: sum without(job) (rate(foo[5m])) AggregateExpr: From 3b6c7e83e70780ee2a8c353642dd77cc564fb92c Mon Sep 17 00:00:00 2001 From: Lukasz Mierzwa Date: Fri, 26 Nov 2021 11:22:20 +0000 Subject: [PATCH 2/2] Improve alerts/template logic --- internal/checks/alerts_template_test.go | 20 ++++++++++++ internal/parser/utils/aggregation.go | 38 +++++++++++++++++++++-- internal/parser/utils/aggregation_test.go | 12 +++++++ 3 files changed, 68 insertions(+), 2 deletions(-) diff --git a/internal/checks/alerts_template_test.go b/internal/checks/alerts_template_test.go index ab7ec16f..7ef23e71 100644 --- a/internal/checks/alerts_template_test.go +++ b/internal/checks/alerts_template_test.go @@ -310,6 +310,26 @@ func TestTemplateCheck(t *testing.T) { }, }, }, + { + description: "annotation label missing from metrics (group_left)", + content: ` +- alert: Foo Is Down + expr: count(build_info) by (instance, version) != ignoring(package) group_left(foo) count(package_installed) by (instance, version, package) + annotations: + summary: '{{ $labels.instance }} on {{ .Labels.foo }} in down' + help: '{{ $labels.ixtance }}' +`, + checker: checks.NewTemplateCheck(checks.Bug), + problems: []checks.Problem{ + { + Fragment: `help: {{ $labels.ixtance }}`, + Lines: []int{6}, + Reporter: "alerts/template", + Text: `template is using "ixtance" label but the query doesn't preseve it`, + Severity: checks.Bug, + }, + }, + }, } runTests(t, testCases) } diff --git a/internal/parser/utils/aggregation.go b/internal/parser/utils/aggregation.go index e01ac91e..9bcf4759 100644 --- a/internal/parser/utils/aggregation.go +++ b/internal/parser/utils/aggregation.go @@ -33,9 +33,43 @@ func HasOuterAggregation(node *parser.PromQLNode) (aggs []*promParser.AggregateE NEXT: if n, ok := node.Node.(*promParser.BinaryExpr); ok { + if n.VectorMatching != nil { + switch n.VectorMatching.Card { + case promParser.CardOneToOne: + case promParser.CardOneToMany: + for i, child := range node.Children { + if i == len(node.Children)-1 { + a := HasOuterAggregation(child) + if len(a) > 0 && !a[0].Without { + a[0].Grouping = append(a[0].Grouping, n.VectorMatching.Include...) + } + return a + } + } + case promParser.CardManyToOne: + a := HasOuterAggregation(node.Children[0]) + if len(a) > 0 && !a[0].Without { + a[0].Grouping = append(a[0].Grouping, n.VectorMatching.Include...) + } + return a + case promParser.CardManyToMany: + default: + log.Warn().Str("matching", n.VectorMatching.Card.String()).Msg("Unsupported VectorMatching operation") + } + } + if n.Op.IsComparisonOperator() { - for _, child := range node.Children { - return HasOuterAggregation(child) + for i, child := range node.Children { + if n.VectorMatching != nil { + a := HasOuterAggregation(child) + if len(a) > 0 && !a[0].Without { + a[0].Grouping = append(a[0].Grouping, n.VectorMatching.Include...) + } + return a + } + if i == 0 { + return HasOuterAggregation(child) + } } } else { switch n.Op { diff --git a/internal/parser/utils/aggregation_test.go b/internal/parser/utils/aggregation_test.go index 69d10b4a..aa667cba 100644 --- a/internal/parser/utils/aggregation_test.go +++ b/internal/parser/utils/aggregation_test.go @@ -77,6 +77,18 @@ func TestHasOuterAggregation(t *testing.T) { expr: "(foo unless on(instance, version, package) bar) and on(instance) (sum(enabled) by(instance) > 0)", output: []string{}, }, + { + expr: "count(build_info) by (instance, version) != ignoring(bar) group_left(package) count(foo) by (instance, version, package)", + output: []string{"count by(instance, version, package) (build_info)"}, + }, + { + expr: "sum(foo) without() != on() group_left(instance) sum(vector(0))", + output: []string{"sum without() (foo)"}, + }, + { + expr: "sum(foo) != on() group_right(instance) sum(vector(0))", + output: []string{"sum by(instance) (vector(0))"}, + }, } for _, tc := range testCases {