Skip to content

Commit

Permalink
Add promql/regexp check
Browse files Browse the repository at this point in the history
  • Loading branch information
prymitive committed Feb 10, 2022
1 parent dcf8585 commit caac98e
Show file tree
Hide file tree
Showing 18 changed files with 324 additions and 33 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Changelog

## v0.11.0

### Added

- Added `promql/regexp` check that will warn about unnecessary regexp matchers.

## v0.10.1

### Fixed
Expand Down
4 changes: 2 additions & 2 deletions cmd/pint/tests/0018_match_alerting.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ cmp stderr stderr.txt
level=info msg="Loading configuration file" path=.pint.hcl
level=info msg="File parsed" path=rules/0001.yml rules=2
level=debug msg="Found recording rule" lines=1-2 path=rules/0001.yml record=colo:recording
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile"] path=rules/0001.yml rule=colo:recording
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/regexp"] path=rules/0001.yml rule=colo:recording
level=debug msg="Found alerting rule" alert=colo:alerting lines=4-5 path=rules/0001.yml
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/aggregate(job:true)"] path=rules/0001.yml rule=colo:alerting
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/regexp","promql/aggregate(job:true)"] path=rules/0001.yml rule=colo:alerting
rules/0001.yml:5: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/aggregate)
expr: sum(bar) without(job)

Expand Down
4 changes: 2 additions & 2 deletions cmd/pint/tests/0019_match_recording.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ cmp stderr stderr.txt
level=info msg="Loading configuration file" path=.pint.hcl
level=info msg="File parsed" path=rules/0001.yml rules=2
level=debug msg="Found recording rule" lines=1-2 path=rules/0001.yml record=colo:recording
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/aggregate(job:true)"] path=rules/0001.yml rule=colo:recording
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/regexp","promql/aggregate(job:true)"] path=rules/0001.yml rule=colo:recording
level=debug msg="Found alerting rule" alert=colo:alerting lines=4-5 path=rules/0001.yml
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile"] path=rules/0001.yml rule=colo:alerting
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/regexp"] path=rules/0001.yml rule=colo:alerting
rules/0001.yml:2: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/aggregate)
expr: sum(foo) without(job)

Expand Down
4 changes: 2 additions & 2 deletions cmd/pint/tests/0020_ignore_kind.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ cmp stderr stderr.txt
level=info msg="Loading configuration file" path=.pint.hcl
level=info msg="File parsed" path=rules/0001.yml rules=2
level=debug msg="Found recording rule" lines=1-2 path=rules/0001.yml record=colo:recording
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/aggregate(job:true)"] path=rules/0001.yml rule=colo:recording
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/regexp","promql/aggregate(job:true)"] path=rules/0001.yml rule=colo:recording
level=debug msg="Found alerting rule" alert=colo:alerting lines=4-5 path=rules/0001.yml
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile"] path=rules/0001.yml rule=colo:alerting
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/regexp"] path=rules/0001.yml rule=colo:alerting
rules/0001.yml:2: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/aggregate)
expr: sum(foo) without(job)

Expand Down
8 changes: 4 additions & 4 deletions cmd/pint/tests/0023_enabled_checks.txt
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
pint.error -l debug --no-color lint rules
! stdout .
stderr 'level=debug msg="Configured checks for rule" enabled=\["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/rate\(prom\)","promql/series\(prom\)","promql/vector_matching\(prom\)"\] path=rules/1.yaml rule=one'
stderr 'level=debug msg="Configured checks for rule" enabled=\["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/rate\(prom\)","promql/series\(prom\)","promql/vector_matching\(prom\)"\] path=rules/1.yaml rule=two'
stderr 'level=debug msg="Configured checks for rule" enabled=\["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/rate\(prom\)","promql/series\(prom\)","promql/vector_matching\(prom\)"\] path=rules/2.yaml rule=one'
stderr 'level=debug msg="Configured checks for rule" enabled=\["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/rate\(prom\)","promql/series\(prom\)","promql/vector_matching\(prom\)"\] path=rules/2.yaml rule=two'
stderr 'level=debug msg="Configured checks for rule" enabled=\["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/regexp","promql/rate\(prom\)","promql/series\(prom\)","promql/vector_matching\(prom\)"\] path=rules/1.yaml rule=one'
stderr 'level=debug msg="Configured checks for rule" enabled=\["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/regexp","promql/rate\(prom\)","promql/series\(prom\)","promql/vector_matching\(prom\)"\] path=rules/1.yaml rule=two'
stderr 'level=debug msg="Configured checks for rule" enabled=\["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/regexp","promql/rate\(prom\)","promql/series\(prom\)","promql/vector_matching\(prom\)"\] path=rules/2.yaml rule=one'
stderr 'level=debug msg="Configured checks for rule" enabled=\["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/regexp","promql/rate\(prom\)","promql/series\(prom\)","promql/vector_matching\(prom\)"\] path=rules/2.yaml rule=two'

-- rules/1.yaml --
- record: one
Expand Down
1 change: 1 addition & 0 deletions cmd/pint/tests/0025_config.txt
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ level=info msg="Loading configuration file" path=.pint.hcl
"promql/comparison",
"promql/fragile",
"promql/rate",
"promql/regexp",
"promql/syntax",
"promql/vector_matching",
"query/cost",
Expand Down
6 changes: 3 additions & 3 deletions cmd/pint/tests/0039_prom_selected_path.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ cmp stderr stderr.txt
level=info msg="Loading configuration file" path=.pint.hcl
level=info msg="File parsed" path=rules/0001.yml rules=3
level=debug msg="Found alerting rule" alert=first lines=1-3 path=rules/0001.yml
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile"] path=rules/0001.yml rule=first
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/regexp"] path=rules/0001.yml rule=first
level=debug msg="Found recording rule" lines=5-6 path=rules/0001.yml record=second
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/aggregate(job:true)"] path=rules/0001.yml rule=second
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/regexp","promql/aggregate(job:true)"] path=rules/0001.yml rule=second
level=debug msg="Found alerting rule" alert=third lines=8-9 path=rules/0001.yml
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile"] path=rules/0001.yml rule=third
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/regexp"] path=rules/0001.yml rule=third
rules/0001.yml:6: job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...) (promql/aggregate)
expr: sum(bar)

Expand Down
8 changes: 4 additions & 4 deletions cmd/pint/tests/0040_rule_match_label.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@ cmp stderr stderr.txt
level=info msg="Loading configuration file" path=.pint.hcl
level=info msg="File parsed" path=rules/rules.yml rules=4
level=debug msg="Found recording rule" lines=1-2 path=rules/rules.yml record=ignore
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile"] path=rules/rules.yml rule=ignore
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/regexp"] path=rules/rules.yml rule=ignore
level=debug msg="Found recording rule" lines=4-7 path=rules/rules.yml record=match
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/aggregate(job:true)"] path=rules/rules.yml rule=match
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/regexp","promql/aggregate(job:true)"] path=rules/rules.yml rule=match
level=debug msg="Found alerting rule" alert=ignore lines=9-10 path=rules/rules.yml
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile"] path=rules/rules.yml rule=ignore
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/regexp"] path=rules/rules.yml rule=ignore
level=debug msg="Found alerting rule" alert=match lines=12-15 path=rules/rules.yml
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/aggregate(job:true)"] path=rules/rules.yml rule=match
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/regexp","promql/aggregate(job:true)"] path=rules/rules.yml rule=match
rules/rules.yml:5: job label is required and should be preserved when aggregating "^.*$" rules, use by(job, ...) (promql/aggregate)
expr: sum(foo)

Expand Down
2 changes: 2 additions & 0 deletions cmd/pint/tests/0042_watch_metrics.txt
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,8 @@ pint_check_duration_seconds_sum{check="promql/comparison"}
pint_check_duration_seconds_count{check="promql/comparison"}
pint_check_duration_seconds_sum{check="promql/fragile"}
pint_check_duration_seconds_count{check="promql/fragile"}
pint_check_duration_seconds_sum{check="promql/regexp"}
pint_check_duration_seconds_count{check="promql/regexp"}
pint_check_duration_seconds_sum{check="promql/syntax"}
pint_check_duration_seconds_count{check="promql/syntax"}
# HELP pint_check_iterations_total Total number of completed check iterations since pint start
Expand Down
4 changes: 2 additions & 2 deletions cmd/pint/tests/0052_match_multiple.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ cmp stderr stderr.txt
level=info msg="Loading configuration file" path=.pint.hcl
level=info msg="File parsed" path=rules/0001.yml rules=2
level=debug msg="Found recording rule" lines=1-2 path=rules/0001.yml record=colo:recording
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/aggregate(job:true)"] path=rules/0001.yml rule=colo:recording
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/regexp","promql/aggregate(job:true)"] path=rules/0001.yml rule=colo:recording
level=debug msg="Found alerting rule" alert=colo:alerting lines=4-5 path=rules/0001.yml
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/aggregate(job:true)"] path=rules/0001.yml rule=colo:alerting
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/regexp","promql/aggregate(job:true)"] path=rules/0001.yml rule=colo:alerting
rules/0001.yml:2: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/aggregate)
expr: sum(foo) without(job)

Expand Down
4 changes: 2 additions & 2 deletions cmd/pint/tests/0053_ignore_multiple.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ cmp stderr stderr.txt
level=info msg="Loading configuration file" path=.pint.hcl
level=info msg="File parsed" path=rules/0001.yml rules=2
level=debug msg="Found recording rule" lines=1-2 path=rules/0001.yml record=colo:recording
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile"] path=rules/0001.yml rule=colo:recording
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/regexp"] path=rules/0001.yml rule=colo:recording
level=debug msg="Found alerting rule" alert=colo:alerting lines=4-5 path=rules/0001.yml
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile"] path=rules/0001.yml rule=colo:alerting
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/fragile","promql/regexp"] path=rules/0001.yml rule=colo:alerting
-- rules/0001.yml --
- record: "colo:recording"
expr: sum(foo) without(job)
Expand Down
22 changes: 22 additions & 0 deletions docs/CONFIGURATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -764,6 +764,28 @@ by adding labels we use in `by()` to `on()`:
errors / on(cluster, instance) sum(requests) by(cluster, instance)
```

## promql/regexp

This check will warn if metric selector uses a regexp match but the regexp query
doesn't have any patterns and so a simple string equality match can be used.
Since regexp checks are more expensive using a simple equality check if
preferable.

Example of a query that would trigger this warning:

```
foo{job=~"bar"}
```

`job=~"bar"` uses a regexp match but since it matches `job` value to a static string
there's no need to use a regexp here and `job="bar"` should be used instead.

Example of a query that wouldn't trigger this warning:

```
foo{job=~"bar|baz"}
```

# Ignoring selected lines or files

While parsing files pint will look for special comment blocks and use them to
Expand Down
1 change: 1 addition & 0 deletions internal/checks/base.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ var (
ComparisonCheckName,
FragileCheckName,
RateCheckName,
RegexpCheckName,
SyntaxCheckName,
VectorMatchingCheckName,
CostCheckName,
Expand Down
74 changes: 74 additions & 0 deletions internal/checks/promql_regexp.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package checks

import (
"context"
"fmt"
"regexp/syntax"

"github.com/cloudflare/pint/internal/parser"
)

const (
RegexpCheckName = "promql/regexp"
)

func NewRegexpCheck() RegexpCheck {
return RegexpCheck{}
}

type RegexpCheck struct{}

func (c RegexpCheck) String() string {
return RegexpCheckName
}

func (c RegexpCheck) Reporter() string {
return RegexpCheckName
}

func (c RegexpCheck) Check(ctx context.Context, rule parser.Rule) (problems []Problem) {
expr := rule.Expr()
if expr.SyntaxError != nil {
return nil
}

for _, selector := range getSelectors(expr.Query) {
for _, lm := range selector.LabelMatchers {
if s := lm.GetRegexString(); s != "" {
var isUseful, isEmpty bool
r, _ := syntax.Parse(s, syntax.Perl)
for _, s := range r.Sub {
switch s.Op {
case syntax.OpBeginText:
continue
case syntax.OpEndText:
continue
case syntax.OpLiteral:
isEmpty = false
case syntax.OpEmptyMatch:
isEmpty = true
default:
isUseful = true
}
}
if !isUseful {
var text string
if isEmpty {
text = fmt.Sprintf("unnecessary regexp match on empty string: %s", lm)
} else {
text = fmt.Sprintf("unnecessary regexp match on static string: %s", lm)
}
problems = append(problems, Problem{
Fragment: selector.String(),
Lines: expr.Lines(),
Reporter: c.Reporter(),
Text: text,
Severity: Warning,
})
}
}
}
}

return
}
66 changes: 66 additions & 0 deletions internal/checks/promql_regexp_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
package checks_test

import (
"testing"

"github.com/cloudflare/pint/internal/checks"
)

func TestRegexpCheck(t *testing.T) {
testCases := []checkTest{
{
description: "ignores rules with syntax errors",
content: "- alert: foo\n expr: sum(foo) without(\n",
checker: checks.NewRegexpCheck(),
},
{
description: "static match",
content: "- record: foo\n expr: foo{job=\"bar\"}\n",
checker: checks.NewRegexpCheck(),
},
{
description: "valid regexp",
content: "- record: foo\n expr: foo{job=~\"bar.+\"}\n",
checker: checks.NewRegexpCheck(),
},
{
description: "valid regexp",
content: "- record: foo\n expr: foo{job!~\"(.*)\"}\n",
checker: checks.NewRegexpCheck(),
},
{
description: "valid regexp",
content: "- record: foo\n expr: foo{job=~\"a|b|c\"}\n",
checker: checks.NewRegexpCheck(),
},
{
description: "unnecessary regexp",
content: "- record: foo\n expr: foo{job=~\"bar\"}\n",
checker: checks.NewRegexpCheck(),
problems: []checks.Problem{
{
Fragment: `foo{job=~"bar"}`,
Lines: []int{2},
Reporter: "promql/regexp",
Text: `unnecessary regexp match on static string: job=~"bar"`,
Severity: checks.Warning,
},
},
},
{
description: "empty regexp",
content: "- record: foo\n expr: foo{job=~\"\"}\n",
checker: checks.NewRegexpCheck(),
problems: []checks.Problem{
{
Fragment: `foo{job=~""}`,
Lines: []int{2},
Reporter: "promql/regexp",
Text: `unnecessary regexp match on empty string: job=~""`,
Severity: checks.Warning,
},
},
},
}
runTests(t, testCases)
}
Loading

0 comments on commit caac98e

Please sign in to comment.