Improve alerts/template messages

cloudflare · Oct 28, 2024 · 0e83d9a · 0e83d9a
1 parent cfb942f
commit 0e83d9a
Show file tree

Hide file tree

Showing 4 changed files with 179 additions and 35 deletions.
diff --git a/internal/checks/alerts_template.go b/internal/checks/alerts_template.go
@@ -35,11 +35,6 @@ If you're hoping to get instance specific labels this way and alert when some ta
 	TemplateCheckOnDetails = `Using [vector matching](https://prometheus.io/docs/prometheus/latest/querying/operators/#vector-matching) operations will impact which labels are available on the results of your query.
 When using ` + "`on()`" + `make sure that all labels you're trying to use in this templare match what the query can return.`
 	TemplateCheckLabelsDetails = `This query doesn't seem to be using any time series and so cannot have any labels.`
-
-	msgAggregation = "Template is using `%s` label but the query removes it."
-	msgAbsent      = "Template is using `%s` label but `absent()` is not passing it."
-	msgOn          = "Template is using `%s` label but the query uses `on(...)` without it being set there, this label will be missing from the query result."
-	msgVector      = "Template is using `%s` label but the query doesn't produce any labels."
 )
 
 var (
@@ -73,10 +68,10 @@ var (
 		"humanizeDuration":   dummyFuncMap,
 		"humanizePercentage": dummyFuncMap,
 		"humanizeTimestamp":  dummyFuncMap,
+		"toTime":             dummyFuncMap,
 		"pathPrefix":         dummyFuncMap,
 		"externalURL":        dummyFuncMap,
 		"parseDuration":      dummyFuncMap,
-		"toTime":             dummyFuncMap,
 	}
 )
 
@@ -456,11 +451,11 @@ func checkQueryLabels(labelName, labelValue string, src utils.Source) (problems
 				}
 				for _, s := range append(src.Alternatives, src) {
 					if s.FixedLabels && !slices.Contains(s.IncludedLabels, v[1]) {
-						problems = append(problems, textForProblem(v[1], s, Bug))
+						problems = append(problems, textForProblem(v[1], "", s, Bug))
 						goto NEXT
 					}
 					if slices.Contains(s.ExcludedLabels, v[1]) {
-						problems = append(problems, textForProblem(v[1], s, Bug))
+						problems = append(problems, textForProblem(v[1], v[1], s, Bug))
 						goto NEXT
 					}
 				}
@@ -473,19 +468,23 @@ func checkQueryLabels(labelName, labelValue string, src utils.Source) (problems
 	return problems
 }
 
-func textForProblem(label string, src utils.Source, severity Severity) exprProblem {
-	// FIXME add query fragment to the details
-
+func textForProblem(label, reasonLabel string, src utils.Source, severity Severity) exprProblem {
 	switch {
 	case src.Operation == "absent":
 		return exprProblem{
-			text:     fmt.Sprintf(msgAbsent, label),
+			text:     fmt.Sprintf("Template is using `%s` label but `%s` is not passing it.", label, src.Call.String()),
 			details:  TemplateCheckAbsentDetails,
 			severity: severity,
 		}
-	case src.Returns == promParser.ValueTypeScalar, src.Returns == promParser.ValueTypeString, src.Operation == "vector":
+	case src.Operation == "vector":
 		return exprProblem{
-			text:     fmt.Sprintf(msgVector, label),
+			text:     fmt.Sprintf("Template is using `%s` label but `%s` doesn't produce any labels.", label, src.Call.String()),
+			details:  TemplateCheckLabelsDetails,
+			severity: severity,
+		}
+	case src.Returns == promParser.ValueTypeScalar, src.Returns == promParser.ValueTypeString:
+		return exprProblem{
+			text:     fmt.Sprintf("Template is using `%s` label but the query doesn't produce any labels.", label),
 			details:  TemplateCheckLabelsDetails,
 			severity: severity,
 		}
@@ -495,14 +494,20 @@ func textForProblem(label string, src utils.Source, severity Severity) exprProbl
 		promParser.CardManyToMany.String(),
 		promParser.CardManyToOne.String(),
 	}, src.Operation):
+		msg, ok := src.ExcludeReason[reasonLabel]
+		if ok {
+			msg = fmt.Sprintf("Template is using `%s` label but the query results won't have this label. %s", label, msg)
+		} else {
+			msg = fmt.Sprintf("Template is using `%s` label but the query uses `on(...)` without it being set there, this label will be missing from the query result.", label)
+		}
 		return exprProblem{
-			text:     fmt.Sprintf(msgOn, label),
+			text:     msg,
 			details:  TemplateCheckOnDetails,
 			severity: severity,
 		}
 	default:
 		return exprProblem{
-			text:     fmt.Sprintf(msgAggregation, label),
+			text:     fmt.Sprintf("Template is using `%s` label but the query removes it.", label),
 			details:  TemplateCheckAggregationDetails,
 			severity: severity,
 		}

diff --git a/internal/checks/alerts_template_test.go b/internal/checks/alerts_template_test.go
@@ -496,7 +496,7 @@ func TestTemplateCheck(t *testing.T) {
 							Last:  5,
 						},
 						Reporter: checks.TemplateCheckName,
-						Text:     "Template is using `instance` label but `absent()` is not passing it.",
+						Text:     "Template is using `instance` label but `absent(foo{job=\"bar\"})` is not passing it.",
 						Details:  checks.TemplateCheckAbsentDetails,
 						Severity: checks.Bug,
 					},
@@ -506,7 +506,7 @@ func TestTemplateCheck(t *testing.T) {
 							Last:  7,
 						},
 						Reporter: checks.TemplateCheckName,
-						Text:     "Template is using `instance` label but `absent()` is not passing it.",
+						Text:     "Template is using `instance` label but `absent(foo{job=\"bar\"})` is not passing it.",
 						Details:  checks.TemplateCheckAbsentDetails,
 						Severity: checks.Bug,
 					},
@@ -516,7 +516,7 @@ func TestTemplateCheck(t *testing.T) {
 							Last:  7,
 						},
 						Reporter: checks.TemplateCheckName,
-						Text:     "Template is using `foo` label but `absent()` is not passing it.",
+						Text:     "Template is using `foo` label but `absent(foo{job=\"bar\"})` is not passing it.",
 						Details:  checks.TemplateCheckAbsentDetails,
 						Severity: checks.Bug,
 					},
@@ -526,7 +526,7 @@ func TestTemplateCheck(t *testing.T) {
 							Last:  8,
 						},
 						Reporter: checks.TemplateCheckName,
-						Text:     "Template is using `xxx` label but `absent()` is not passing it.",
+						Text:     "Template is using `xxx` label but `absent(foo{job=\"bar\"})` is not passing it.",
 						Details:  checks.TemplateCheckAbsentDetails,
 						Severity: checks.Bug,
 					},
@@ -551,7 +551,7 @@ func TestTemplateCheck(t *testing.T) {
 							Last:  5,
 						},
 						Reporter: checks.TemplateCheckName,
-						Text:     "Template is using `instance` label but `absent()` is not passing it.",
+						Text:     "Template is using `instance` label but `absent(sum by (job, instance) (foo))` is not passing it.",
 						Details:  checks.TemplateCheckAbsentDetails,
 						Severity: checks.Bug,
 					},
@@ -561,7 +561,7 @@ func TestTemplateCheck(t *testing.T) {
 							Last:  5,
 						},
 						Reporter: checks.TemplateCheckName,
-						Text:     "Template is using `job` label but `absent()` is not passing it.",
+						Text:     "Template is using `job` label but `absent(sum by (job, instance) (foo))` is not passing it.",
 						Details:  checks.TemplateCheckAbsentDetails,
 						Severity: checks.Bug,
 					},
@@ -586,7 +586,7 @@ func TestTemplateCheck(t *testing.T) {
 							Last:  5,
 						},
 						Reporter: checks.TemplateCheckName,
-						Text:     "Template is using `instance` label but `absent()` is not passing it.",
+						Text:     "Template is using `instance` label but `absent(sum by (job) (foo))` is not passing it.",
 						Details:  checks.TemplateCheckAbsentDetails,
 						Severity: checks.Bug,
 					},
@@ -596,7 +596,7 @@ func TestTemplateCheck(t *testing.T) {
 							Last:  5,
 						},
 						Reporter: checks.TemplateCheckName,
-						Text:     "Template is using `job` label but `absent()` is not passing it.",
+						Text:     "Template is using `job` label but `absent(sum by (job) (foo))` is not passing it.",
 						Details:  checks.TemplateCheckAbsentDetails,
 						Severity: checks.Bug,
 					},
@@ -621,7 +621,7 @@ func TestTemplateCheck(t *testing.T) {
 							Last:  5,
 						},
 						Reporter: checks.TemplateCheckName,
-						Text:     "Template is using `job` label but `absent()` is not passing it.",
+						Text:     "Template is using `job` label but `absent({job=~\".+\"})` is not passing it.",
 						Details:  checks.TemplateCheckAbsentDetails,
 						Severity: checks.Bug,
 					},
@@ -646,7 +646,7 @@ func TestTemplateCheck(t *testing.T) {
 							Last:  5,
 						},
 						Reporter: checks.TemplateCheckName,
-						Text:     "Template is using `job` label but `absent()` is not passing it.",
+						Text:     "Template is using `job` label but `absent(bar)` is not passing it.",
 						Details:  checks.TemplateCheckAbsentDetails,
 						Severity: checks.Bug,
 					},
@@ -683,7 +683,7 @@ func TestTemplateCheck(t *testing.T) {
 							Last:  5,
 						},
 						Reporter: checks.TemplateCheckName,
-						Text:     "Template is using `cluster` label but `absent()` is not passing it.",
+						Text:     "Template is using `cluster` label but `absent(foo{job=\"xxx\"})` is not passing it.",
 						Details:  checks.TemplateCheckAbsentDetails,
 						Severity: checks.Bug,
 					},
@@ -693,7 +693,7 @@ func TestTemplateCheck(t *testing.T) {
 							Last:  5,
 						},
 						Reporter: checks.TemplateCheckName,
-						Text:     "Template is using `env` label but `absent()` is not passing it.",
+						Text:     "Template is using `env` label but `absent(foo{job=\"xxx\"})` is not passing it.",
 						Details:  checks.TemplateCheckAbsentDetails,
 						Severity: checks.Bug,
 					},
@@ -730,7 +730,7 @@ func TestTemplateCheck(t *testing.T) {
 							Last:  5,
 						},
 						Reporter: checks.TemplateCheckName,
-						Text:     "Template is using `cluster` label but `absent()` is not passing it.",
+						Text:     "Template is using `cluster` label but `absent(foo{job=\"xxx\"})` is not passing it.",
 						Details:  checks.TemplateCheckAbsentDetails,
 						Severity: checks.Bug,
 					},
@@ -740,7 +740,7 @@ func TestTemplateCheck(t *testing.T) {
 							Last:  5,
 						},
 						Reporter: checks.TemplateCheckName,
-						Text:     "Template is using `env` label but `absent()` is not passing it.",
+						Text:     "Template is using `env` label but `absent(foo{job=\"xxx\"})` is not passing it.",
 						Details:  checks.TemplateCheckAbsentDetails,
 						Severity: checks.Bug,
 					},
@@ -1216,7 +1216,7 @@ func TestTemplateCheck(t *testing.T) {
 							Last:  4,
 						},
 						Reporter: checks.TemplateCheckName,
-						Text:     "Template is using `instance` label but the query doesn't produce any labels.",
+						Text:     "Template is using `instance` label but `vector(1)` doesn't produce any labels.",
 						Details:  checks.TemplateCheckLabelsDetails,
 						Severity: checks.Bug,
 					},
@@ -1236,7 +1236,7 @@ func TestTemplateCheck(t *testing.T) {
 							Last:  4,
 						},
 						Reporter: checks.TemplateCheckName,
-						Text:     "Template is using `instance` label but the query doesn't produce any labels.",
+						Text:     "Template is using `instance` label but `vector(1)` doesn't produce any labels.",
 						Details:  checks.TemplateCheckLabelsDetails,
 						Severity: checks.Bug,
 					},
@@ -1292,7 +1292,7 @@ func TestTemplateCheck(t *testing.T) {
 							Last:  6,
 						},
 						Reporter: checks.TemplateCheckName,
-						Text:     "Template is using `job_name` label but the query uses `on(...)` without it being set there, this label will be missing from the query result.",
+						Text:     "Template is using `job_name` label but the query results won't have this label. Query is using one-to-one vector matching with `on(instance, app_name)`, only labels included inside `on(...)` will be present on the results.",
 						Details:  checks.TemplateCheckOnDetails,
 						Severity: checks.Bug,
 					},
@@ -1302,7 +1302,7 @@ func TestTemplateCheck(t *testing.T) {
 							Last:  4,
 						},
 						Reporter: checks.TemplateCheckName,
-						Text:     "Template is using `app_type` label but the query uses `on(...)` without it being set there, this label will be missing from the query result.",
+						Text:     "Template is using `app_type` label but the query results won't have this label. Query is using one-to-one vector matching with `on(instance, app_name)`, only labels included inside `on(...)` will be present on the results.",
 						Details:  checks.TemplateCheckOnDetails,
 						Severity: checks.Bug,
 					},

diff --git a/internal/parser/utils/source.go b/internal/parser/utils/source.go
@@ -1,7 +1,9 @@
 package utils
 
 import (
+	"fmt"
 	"slices"
+	"strings"
 
 	"github.com/cloudflare/pint/internal/parser"
 
@@ -23,6 +25,7 @@ const (
 type Source struct {
 	Selector         *promParser.VectorSelector
 	Call             *promParser.Call
+	ExcludeReason    map[string]string // Reason why a label was excluded
 	Operation        string
 	Returns          promParser.ValueType
 	IncludedLabels   []string // Labels that are included by filters, they will be present if exist on source series (by).
@@ -71,6 +74,8 @@ func walkNode(node promParser.Node) (s Source) {
 			// Param is the label to store the count value in.
 			s.GuaranteedLabels = appendToSlice(s.GuaranteedLabels, n.Param.(*promParser.StringLiteral).Val)
 			s.IncludedLabels = appendToSlice(s.IncludedLabels, n.Param.(*promParser.StringLiteral).Val)
+			s.ExcludedLabels = removeFromSlice(s.ExcludedLabels, n.Param.(*promParser.StringLiteral).Val)
+			delete(s.ExcludeReason, n.Param.(*promParser.StringLiteral).Val)
 		case promParser.QUANTILE:
 			s = parseAggregation(n)
 			s.Operation = "quantile"
@@ -106,6 +111,10 @@ func walkNode(node promParser.Node) (s Source) {
 			s = walkNode(n.LHS)
 			if n.VectorMatching.On {
 				s.FixedLabels = true
+				s.ExcludeReason = setInMap(s.ExcludeReason, "", fmt.Sprintf(
+					"Query is using %s vector matching with `on(%s)`, only labels included inside `on(...)` will be present on the results.",
+					n.VectorMatching.Card, strings.Join(n.VectorMatching.MatchingLabels, ", "),
+				))
 			}
 		case n.VectorMatching.Card == promParser.CardOneToMany:
 			s = walkNode(n.RHS)
@@ -125,8 +134,15 @@ func walkNode(node promParser.Node) (s Source) {
 			// on=true when using on(), on=false when using ignore()
 			if n.VectorMatching.On {
 				s.IncludedLabels = appendToSlice(s.IncludedLabels, n.VectorMatching.MatchingLabels...)
+				s.ExcludedLabels = removeFromSlice(s.ExcludedLabels, n.VectorMatching.MatchingLabels...)
+				for _, name := range n.VectorMatching.MatchingLabels {
+					delete(s.ExcludeReason, name)
+				}
 			}
 			s.ExcludedLabels = removeFromSlice(s.ExcludedLabels, n.VectorMatching.Include...)
+			for _, name := range n.VectorMatching.Include {
+				delete(s.ExcludeReason, name)
+			}
 		}
 
 	case *promParser.Call:
@@ -191,6 +207,14 @@ func appendToSlice(dst []string, values ...string) []string {
 	return dst
 }
 
+func setInMap(dst map[string]string, key, val string) map[string]string {
+	if dst == nil {
+		dst = map[string]string{}
+	}
+	dst[key] = val
+	return dst
+}
+
 func guaranteedLabelsFromSelector(selector *promParser.VectorSelector) (names []string) {
 	// Any label used in positive filters is gurnateed to be present.
 	for _, lm := range selector.LabelMatchers {
@@ -210,6 +234,14 @@ func parseAggregation(n *promParser.AggregateExpr) (s Source) {
 		s.ExcludedLabels = appendToSlice(s.ExcludedLabels, n.Grouping...)
 		s.IncludedLabels = removeFromSlice(s.IncludedLabels, n.Grouping...)
 		s.GuaranteedLabels = removeFromSlice(s.GuaranteedLabels, n.Grouping...)
+		for _, name := range n.Grouping {
+			s.ExcludeReason = setInMap(
+				s.ExcludeReason,
+				name,
+				fmt.Sprintf("Query is using aggregation with `without(%s)`, all labels included inside `without(...)` will be removed from the results.",
+					strings.Join(n.Grouping, ", ")),
+			)
+		}
 	} else {
 		s.FixedLabels = true
 		if len(n.Grouping) == 0 {
@@ -220,6 +252,12 @@ func parseAggregation(n *promParser.AggregateExpr) (s Source) {
 			for _, name := range n.Grouping {
 				s.ExcludedLabels = removeFromSlice(s.ExcludedLabels, name)
 			}
+			s.ExcludeReason = setInMap(
+				s.ExcludeReason,
+				"",
+				fmt.Sprintf("Query is using aggregation with `by(%s)`, only labels included inside `by(...)` will be present on the results.",
+					strings.Join(n.Grouping, ", ")),
+			)
 		}
 	}
 	s.Type = AggregateSource