diff --git a/CHANGELOG.md b/CHANGELOG.md index ae851f3d..c296b34e 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/cmd/pint/lint.go b/cmd/pint/lint.go index 7d5622ee..b5ab592d 100644 --- a/cmd/pint/lint.go +++ b/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...) diff --git a/cmd/pint/main.go b/cmd/pint/main.go index 039e6766..be86e513 100644 --- a/cmd/pint/main.go +++ b/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", + }, }, }, { diff --git a/cmd/pint/tests/0007_alerts.txt b/cmd/pint/tests/0007_alerts.txt index 551e3eaa..90b82f6e 100644 --- a/cmd/pint/tests/0007_alerts.txt +++ b/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) diff --git a/internal/checks/alerts_template.go b/internal/checks/alerts_template.go index 6d2efc17..8b19c436 100644 --- a/internal/checks/alerts_template.go +++ b/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,7 +187,7 @@ 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)...) @@ -168,8 +195,8 @@ func checkForValueInLabels(name, text string) (msgs []string) { 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 +} diff --git a/internal/checks/alerts_template_test.go b/internal/checks/alerts_template_test.go index 3d7327de..6b1e158c 100644 --- a/internal/checks/alerts_template_test.go +++ b/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) } diff --git a/internal/checks/base.go b/internal/checks/base.go index 26e5a597..79b63bfb 100644 --- a/internal/checks/base.go +++ b/internal/checks/base.go @@ -22,6 +22,13 @@ var ( TemplateCheckName, VectorMatchingCheckName, } + OnlineChecks = []string{ + AlertsCheckName, + CostCheckName, + RateCheckName, + SeriesCheckName, + VectorMatchingCheckName, + } ) // Severity of the problem reported diff --git a/internal/checks/promql_comparison.go b/internal/checks/promql_comparison.go index 693a9e8b..a263a0ab 100644 --- a/internal/checks/promql_comparison.go +++ b/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, }) diff --git a/internal/checks/promql_comparison_test.go b/internal/checks/promql_comparison_test.go index 75450a5d..b3edf95e 100644 --- a/internal/checks/promql_comparison_test.go +++ b/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, }, diff --git a/internal/config/config.go b/internal/config/config.go index 15adb07a..a6ba76ea 100644 --- a/internal/config/config.go +++ b/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 { diff --git a/internal/parser/decode.go b/internal/parser/decode.go index 34ae29da..e75642f8 100644 --- a/internal/parser/decode.go +++ b/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 } diff --git a/internal/parser/models.go b/internal/parser/models.go index d4dc82ec..197a52f3 100644 --- a/internal/parser/models.go +++ b/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 diff --git a/internal/parser/utils/aggregation.go b/internal/parser/utils/aggregation.go new file mode 100644 index 00000000..1ed79381 --- /dev/null +++ b/internal/parser/utils/aggregation.go @@ -0,0 +1,45 @@ +package utils + +import ( + "github.com/cloudflare/pint/internal/parser" + + promParser "github.com/prometheus/prometheus/promql/parser" + "github.com/rs/zerolog/log" +) + +func HasOuterAggregation(node *parser.PromQLNode) *promParser.AggregateExpr { + if n, ok := node.Node.(*promParser.AggregateExpr); ok { + switch n.Op { + case promParser.SUM: + case promParser.MIN: + case promParser.MAX: + case promParser.AVG: + case promParser.GROUP: + case promParser.STDDEV: + case promParser.STDVAR: + case promParser.COUNT: + case promParser.COUNT_VALUES: + case promParser.BOTTOMK: + goto NEXT + case promParser.TOPK: + goto NEXT + case promParser.QUANTILE: + default: + log.Warn().Str("op", n.Op.String()).Msg("Unsupported aggregation operation") + } + return n + } + +NEXT: + if n, ok := node.Node.(*promParser.BinaryExpr); ok && n.VectorMatching != nil { + return HasOuterAggregation(node.Children[0]) + } + + for _, child := range node.Children { + if a := HasOuterAggregation(child); a != nil { + return a + } + } + + return nil +} diff --git a/internal/parser/utils/aggregation_test.go b/internal/parser/utils/aggregation_test.go new file mode 100644 index 00000000..cd12dced --- /dev/null +++ b/internal/parser/utils/aggregation_test.go @@ -0,0 +1,70 @@ +package utils_test + +import ( + "testing" + + "github.com/cloudflare/pint/internal/parser" + "github.com/cloudflare/pint/internal/parser/utils" + "github.com/google/go-cmp/cmp" +) + +func TestHasOuterAggregation(t *testing.T) { + type testCaseT struct { + expr string + output string + } + + testCases := []testCaseT{ + { + expr: "foo", + }, + { + expr: "sum(foo)", + output: "sum(foo)", + }, + { + expr: "sum(foo) by(job)", + output: "sum by(job) (foo)", + }, + { + expr: "sum(foo) without(job)", + output: "sum without(job) (foo)", + }, + { + expr: "1 + sum(foo)", + output: "sum(foo)", + }, + { + expr: "sum(foo) + sum(bar)", + output: "sum(foo)", + }, + { + expr: "foo / on(bbb) sum(bar)", + }, + { + expr: "sum(foo) / on(bbb) sum(bar)", + output: "sum(foo)", + }, + } + + for _, tc := range testCases { + t.Run(tc.expr, func(t *testing.T) { + n, err := parser.DecodeExpr(tc.expr) + if err != nil { + t.Error(err) + t.FailNow() + } + output := utils.HasOuterAggregation(n) + if output == nil { + if tc.output != "" { + t.Errorf("HasOuterAggregation() returned nil, expected %q", tc.output) + } + } else { + if diff := cmp.Diff(tc.output, output.String()); diff != "" { + t.Errorf("HasOuterAggregation() returned wrong result (-want +got):\n%s", diff) + return + } + } + }) + } +}