From 3b6c7e83e70780ee2a8c353642dd77cc564fb92c Mon Sep 17 00:00:00 2001
From: Lukasz Mierzwa <lukasz@cloudflare.com>
Date: Fri, 26 Nov 2021 11:22:20 +0000
Subject: [PATCH] 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 {