Merge pull request cloudflare#76 from cloudflare/check-labels

Check labels used on annotations

CHANGELOG.md

@@ -1,5 +1,24 @@
 # Changelog
 
+## v0.2.0
+
+### Added
+
+- `--offline` flag for `pint lint` command. When passed only checks that don't send
+  any live queries to Prometheus server will be run.
+- `alerts/template` check will now warn if template if referencing a label that is being
+  stripped by aggregation.
+  Example:
+
+  ```
+  - alert: Foo
+    expr: count(up) without(instance) == 0
+    annotations:
+      summary: 'foo is down on {{ $labels.instance }}'
+  ```
+
+  Would generate a warning since `instance` label is being stripped by `without(instance)`.
+
 ## v0.1.5
 
 ### Fixed

cmd/pint/lint.go

@@ -29,7 +29,7 @@ func actionLint(c *cli.Context) (err error) {
 		return fmt.Errorf("failed to load config file %q: %w", c.Path(configFlag), err)
 	}
 
-	cfg.SetDisabledChecks(c.StringSlice(disabledFlag))
+	cfg.SetDisabledChecks(c.Bool(offlineFlag), c.StringSlice(disabledFlag))
 
 	d := discovery.NewGlobFileFinder()
 	toScan, err := d.Find(paths...)

cmd/pint/main.go

@@ -15,6 +15,7 @@ const (
 	configFlag   = "config"
 	logLevelFlag = "log-level"
 	disabledFlag = "disabled"
+	offlineFlag  = "offline"
 )
 
 var (
@@ -56,6 +57,12 @@ func newApp() *cli.App {
 						Value:   cli.NewStringSlice(),
 						Usage:   "List of checks to disable (example: promql/cost)",
 					},
+					&cli.BoolFlag{
+						Name:    offlineFlag,
+						Aliases: []string{"o"},
+						Value:   false,
+						Usage:   "Disable all check that send live queries to Prometheus servers",
+					},
 				},
 			},
 			{

cmd/pint/tests/0007_alerts.txt

@@ -14,7 +14,7 @@ rules/0001.yml:1-2: severity label is required (rule/label)
 - alert: Always
   expr: up
 
-rules/0001.yml:2: alert query doesn't have any condition, it will always fire if the metric exists (alerts/count)
+rules/0001.yml:2: alert query doesn't have any condition, it will always fire if the metric exists (promql/comparison)
   expr: up
 
 rules/0001.yml:9-10: url annotation is required (alerts/annotation)

internal/checks/alerts_template.go

@@ -10,6 +10,7 @@ import (
 	"time"
 
 	"github.com/cloudflare/pint/internal/parser"
+	"github.com/cloudflare/pint/internal/parser/utils"
 	"github.com/prometheus/common/model"
 	"github.com/prometheus/prometheus/pkg/timestamp"
 	promTemplate "github.com/prometheus/prometheus/template"
@@ -74,6 +75,8 @@ func (c TemplateCheck) Check(rule parser.Rule) (problems []Problem) {
 		return nil
 	}
 
+	aggr := utils.HasOuterAggregation(rule.AlertingRule.Expr.Query)
+
 	data := promTemplate.AlertTemplateData(map[string]string{}, map[string]string{}, "", 0)
 
 	if rule.AlertingRule.Labels != nil {
@@ -107,6 +110,18 @@ func (c TemplateCheck) Check(rule parser.Rule) (problems []Problem) {
 					Severity: c.severity,
 				})
 			}
+
+			if aggr != nil {
+				for _, msg := range checkMetricLabels(label.Key.Value, label.Value.Value, aggr.Grouping, aggr.Without) {
+					problems = append(problems, Problem{
+						Fragment: fmt.Sprintf("%s: %s", label.Key.Value, label.Value.Value),
+						Lines:    label.Lines(),
+						Reporter: TemplateCheckName,
+						Text:     msg,
+						Severity: c.severity,
+					})
+				}
+			}
 		}
 	}
 
@@ -121,6 +136,18 @@ func (c TemplateCheck) Check(rule parser.Rule) (problems []Problem) {
 					Severity: c.severity,
 				})
 			}
+
+			if aggr != nil {
+				for _, msg := range checkMetricLabels(annotation.Key.Value, annotation.Value.Value, aggr.Grouping, aggr.Without) {
+					problems = append(problems, Problem{
+						Fragment: fmt.Sprintf("%s: %s", annotation.Key.Value, annotation.Value.Value),
+						Lines:    annotation.Lines(),
+						Reporter: TemplateCheckName,
+						Text:     msg,
+						Severity: c.severity,
+					})
+				}
+			}
 		}
 	}
 
@@ -160,16 +187,16 @@ func checkForValueInLabels(name, text string) (msgs []string) {
 	}
 
 	var aliases = aliasMap{aliases: map[string]map[string]struct{}{}}
-	var vars []string
+	var vars = [][]string{}
 	for _, node := range t.Root.Nodes {
 		getAliases(node, &aliases)
 		vars = append(vars, getVariables(node)...)
 	}
 	var valAliases = aliases.varAliases(".Value")
 	for _, v := range vars {
 		for _, a := range valAliases {
-			if v == a {
-				msg := fmt.Sprintf("using %s in labels will generate a new alert on every value change, move it to annotations", v)
+			if v[0] == a {
+				msg := fmt.Sprintf("using %s in labels will generate a new alert on every value change, move it to annotations", v[0])
 				msgs = append(msgs, msg)
 			}
 		}
@@ -200,10 +227,10 @@ func getAliases(node parse.Node, aliases *aliasMap) {
 					for _, k := range getVariables(arg) {
 						for _, d := range n.Pipe.Decl {
 							for _, v := range getVariables(d) {
-								if _, ok := aliases.aliases[k]; !ok {
-									aliases.aliases[k] = map[string]struct{}{}
+								if _, ok := aliases.aliases[k[0]]; !ok {
+									aliases.aliases[k[0]] = map[string]struct{}{}
 								}
-								aliases.aliases[k][v] = struct{}{}
+								aliases.aliases[k[0]][v[0]] = struct{}{}
 							}
 						}
 					}
@@ -213,7 +240,7 @@ func getAliases(node parse.Node, aliases *aliasMap) {
 	}
 }
 
-func getVariables(node parse.Node) (vars []string) {
+func getVariables(node parse.Node) (vars [][]string) {
 	switch n := node.(type) {
 	case *parse.ActionNode:
 		if len(n.Pipe.Decl) == 0 && len(n.Pipe.Cmds) > 0 {
@@ -224,12 +251,51 @@ func getVariables(node parse.Node) (vars []string) {
 			vars = append(vars, getVariables(arg)...)
 		}
 	case *parse.FieldNode:
-		for _, i := range n.Ident {
-			vars = append(vars, "."+i)
-		}
+		n.Ident[0] = "." + n.Ident[0]
+		vars = append(vars, n.Ident)
 	case *parse.VariableNode:
-		vars = append(vars, n.Ident...)
+		vars = append(vars, n.Ident)
 	}
 
 	return vars
 }
+
+func checkMetricLabels(name, text string, metricLabels []string, excludeLabels bool) (msgs []string) {
+	t, err := textTemplate.
+		New(name).
+		Funcs(templateFuncMap).
+		Option("missingkey=zero").
+		Parse(strings.Join(append(templateDefs, text), ""))
+	if err != nil {
+		// no need to double report errors
+		return nil
+	}
+
+	var aliases = aliasMap{aliases: map[string]map[string]struct{}{}}
+	var vars = [][]string{}
+	for _, node := range t.Root.Nodes {
+		getAliases(node, &aliases)
+		vars = append(vars, getVariables(node)...)
+	}
+
+	var labelsAliases = aliases.varAliases(".Labels")
+	for _, v := range vars {
+		for _, a := range labelsAliases {
+			if len(v) > 1 && v[0] == a {
+				var found bool
+				for _, l := range metricLabels {
+					if len(v) > 1 && v[1] == l {
+						found = true
+					}
+				}
+				if found == excludeLabels {
+					msg := fmt.Sprintf("template is using %q label but the query doesn't preseve it", v[1])
+					msgs = append(msgs, msg)
+
+				}
+			}
+		}
+	}
+
+	return
+}

internal/checks/alerts_template_test.go

@@ -226,22 +226,62 @@ func TestTemplateCheck(t *testing.T) {
 				},
 			},
 		},
-		/*
-			{
-				description: "annotation label missing from metrics",
-				content:     "- alert: Foo Is Down\n  expr: up{job=\"foo\"} == 0\n  annotations:\n    summary: '{{ $labels.job }}'\n",
-				checker:     checks.NewTemplateCheck(checks.Bug),
-				problems: []checks.Problem{
-					{
-						Fragment: `summary: '{{ $labels.job }}`,
-						Lines:    []int{2, 3},
-						Reporter: "alerts/count",
-						Text:     "query using prom would trigger 1 alert(s) in the last 1d",
-						Severity: checks.Information,
-					},
-				},
-			},
-		*/
+		{
+			description: "annotation label missing from metrics (by)",
+			content:     "- alert: Foo Is Down\n  expr: sum(foo) > 0\n  annotations:\n    summary: '{{ $labels.job }}'\n",
+			checker:     checks.NewTemplateCheck(checks.Bug),
+			problems: []checks.Problem{
+				{
+					Fragment: `summary: {{ $labels.job }}`,
+					Lines:    []int{4},
+					Reporter: "alerts/template",
+					Text:     `template is using "job" label but the query doesn't preseve it`,
+					Severity: checks.Bug,
+				},
+			},
+		},
+		{
+			description: "annotation label missing from metrics (by)",
+			content:     "- alert: Foo Is Down\n  expr: sum(foo) > 0\n  annotations:\n    summary: '{{ .Labels.job }}'\n",
+			checker:     checks.NewTemplateCheck(checks.Bug),
+			problems: []checks.Problem{
+				{
+					Fragment: `summary: {{ .Labels.job }}`,
+					Lines:    []int{4},
+					Reporter: "alerts/template",
+					Text:     `template is using "job" label but the query doesn't preseve it`,
+					Severity: checks.Bug,
+				},
+			},
+		},
+		{
+			description: "annotation label missing from metrics (without)",
+			content:     "- alert: Foo Is Down\n  expr: sum(foo) without(job) > 0\n  annotations:\n    summary: '{{ $labels.job }}'\n",
+			checker:     checks.NewTemplateCheck(checks.Bug),
+			problems: []checks.Problem{
+				{
+					Fragment: `summary: {{ $labels.job }}`,
+					Lines:    []int{4},
+					Reporter: "alerts/template",
+					Text:     `template is using "job" label but the query doesn't preseve it`,
+					Severity: checks.Bug,
+				},
+			},
+		},
+		{
+			description: "annotation label missing from metrics (without)",
+			content:     "- alert: Foo Is Down\n  expr: sum(foo) without(job) > 0\n  annotations:\n    summary: '{{ .Labels.job }}'\n",
+			checker:     checks.NewTemplateCheck(checks.Bug),
+			problems: []checks.Problem{
+				{
+					Fragment: `summary: {{ .Labels.job }}`,
+					Lines:    []int{4},
+					Reporter: "alerts/template",
+					Text:     `template is using "job" label but the query doesn't preseve it`,
+					Severity: checks.Bug,
+				},
+			},
+		},
 	}
 	runTests(t, testCases)
 }

internal/checks/base.go

@@ -22,6 +22,13 @@ var (
 		TemplateCheckName,
 		VectorMatchingCheckName,
 	}
+	OnlineChecks = []string{
+		AlertsCheckName,
+		CostCheckName,
+		RateCheckName,
+		SeriesCheckName,
+		VectorMatchingCheckName,
+	}
 )
 
 // Severity of the problem reported

internal/checks/promql_comparison.go

@@ -38,7 +38,7 @@ func (c ComparisonCheck) Check(rule parser.Rule) (problems []Problem) {
 	problems = append(problems, Problem{
 		Fragment: rule.AlertingRule.Expr.Value.Value,
 		Lines:    rule.AlertingRule.Expr.Lines(),
-		Reporter: AlertsCheckName,
+		Reporter: ComparisonCheckName,
 		Text:     "alert query doesn't have any condition, it will always fire if the metric exists",
 		Severity: c.severity,
 	})

internal/checks/promql_comparison_test.go

@@ -39,7 +39,7 @@ func TestComparisonCheck(t *testing.T) {
 				{
 					Fragment: `up{job="foo"}`,
 					Lines:    []int{2},
-					Reporter: "alerts/count",
+					Reporter: "promql/comparison",
 					Text:     "alert query doesn't have any condition, it will always fire if the metric exists",
 					Severity: checks.Warning,
 				},
@@ -68,7 +68,7 @@ func TestComparisonCheck(t *testing.T) {
 				{
 					Fragment: `quantile_over_time(0.7,(irate(udp_packets_drops[2m]))[10m:2m]) AND ON (instance) rate(node_netstat_Udp_RcvbufErrors[5m])+rate(node_netstat_Udp6_RcvbufErrors[5m])`,
 					Lines:    []int{3},
-					Reporter: "alerts/count",
+					Reporter: "promql/comparison",
 					Text:     "alert query doesn't have any condition, it will always fire if the metric exists",
 					Severity: checks.Warning,
 				},

internal/config/config.go

@@ -23,8 +23,13 @@ type Config struct {
 	Rules      []Rule             `hcl:"rule,block"`
 }
 
-func (cfg *Config) SetDisabledChecks(l []string) {
+func (cfg *Config) SetDisabledChecks(offline bool, l []string) {
 	disabled := map[string]struct{}{}
+	if offline {
+		for _, name := range checks.OnlineChecks {
+			disabled[name] = struct{}{}
+		}
+	}
 	for _, s := range l {
 		re := strictRegex(s)
 		for _, name := range checks.CheckNames {

internal/parser/decode.go

@@ -6,7 +6,7 @@ import (
 	promparser "github.com/prometheus/prometheus/promql/parser"
 )
 
-func decodeExpr(expr string) (*PromQLNode, error) {
+func DecodeExpr(expr string) (*PromQLNode, error) {
 	node, err := promparser.ParseExpr(expr)
 	if err != nil {
 		pqe := PromQLError{Err: err}
@@ -30,7 +30,7 @@ func decodeExpr(expr string) (*PromQLNode, error) {
 	}
 
 	for _, child := range promparser.Children(node) {
-		c, err := decodeExpr(child.String())
+		c, err := DecodeExpr(child.String())
 		if err != nil {
 			return nil, err
 		}

internal/parser/models.go

@@ -208,7 +208,7 @@ func newPromQLExpr(key, val *yaml.Node) *PromQLExpr {
 		Value: newYamlNodeWithParent(key, val),
 	}
 
-	qlNode, err := decodeExpr(val.Value)
+	qlNode, err := DecodeExpr(val.Value)
 	if err != nil {
 		expr.SyntaxError = err
 		return &expr