Merge pull request cloudflare#79 from cloudflare/parse

Improve alerts/template parsing logic
linode-obs · Nov 26, 2021 · 2aeebeb · 2aeebeb
2 parents cca7a93 + 3b6c7e8
commit 2aeebeb
Show file tree

Hide file tree

Showing 6 changed files with 85 additions and 5 deletions.
diff --git a/cmd/pint/debug.go → cmd/pint/parse.go b/cmd/pint/debug.go → 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
@@ -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
@@ -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:

diff --git 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
@@ -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
@@ -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 {