From 741d22ce8bb875b239991ac30b02dae67cd17bc5 Mon Sep 17 00:00:00 2001 From: Lukasz Mierzwa Date: Fri, 16 Aug 2024 12:00:57 +0100 Subject: [PATCH] Improve promql/regexp messages --- go.mod | 4 +- go.sum | 4 +- internal/checks/promql_regexp.go | 100 +++++++++++++++++++------- internal/checks/promql_regexp_test.go | 96 +++++++++++++++++++++++++ 4 files changed, 173 insertions(+), 31 deletions(-) diff --git a/go.mod b/go.mod index f85e7aba..d6976b70 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/cloudflare/pint -go 1.22.0 +go 1.23.0 require ( github.com/cespare/xxhash/v2 v2.3.0 @@ -15,7 +15,7 @@ require ( github.com/prometheus/client_model v0.6.1 github.com/prometheus/common v0.55.0 github.com/prometheus/prometheus v0.54.0 - github.com/prymitive/current v0.1.0 + github.com/prymitive/current v0.1.1 github.com/rogpeppe/go-internal v1.12.0 github.com/stretchr/testify v1.9.0 github.com/urfave/cli/v2 v2.27.4 diff --git a/go.sum b/go.sum index e42f802a..f23bc1cd 100644 --- a/go.sum +++ b/go.sum @@ -138,8 +138,8 @@ github.com/prometheus/procfs v0.15.1 h1:YagwOFzUgYfKKHX6Dr+sHT7km/hxC76UB0leargg github.com/prometheus/procfs v0.15.1/go.mod h1:fB45yRUv8NstnjriLhBQLuOUt+WW4BsoGhij/e3PBqk= github.com/prometheus/prometheus v0.54.0 h1:6+VmEkohHcofl3W5LyRlhw1Lfm575w/aX6ZFyVAmzM0= github.com/prometheus/prometheus v0.54.0/go.mod h1:xlLByHhk2g3ycakQGrMaU8K7OySZx98BzeCR99991NY= -github.com/prymitive/current v0.1.0 h1:j0qvhMUKEz4rZE7YgftTYnBcaujmv6RVGvvmEC+p+6E= -github.com/prymitive/current v0.1.0/go.mod h1:ZKbTBHjDMGAM3YPcnkA2I4L5U/vYfbXyVTKZJWhTCoc= +github.com/prymitive/current v0.1.1 h1:RNMYkRQV0M9H1ndtnvGIeUHG6P7vwRxoHn01KvHKuIs= +github.com/prymitive/current v0.1.1/go.mod h1:Zjs6a6tU1nIrtp5pefJXg2NKES2L/eqH3Lf05/wr5R4= github.com/rogpeppe/go-internal v1.9.0/go.mod h1:WtVeX8xhTBvf0smdhujwtBcq4Qrzq/fJaraNFVN+nFs= github.com/rogpeppe/go-internal v1.12.0 h1:exVL4IDcn6na9z1rAb56Vxr+CgyK3nn3O+epU5NdKM8= github.com/rogpeppe/go-internal v1.12.0/go.mod h1:E+RYuTGaKKdloAfM02xzb0FW3Paa99yedzYV+kq4uf4= diff --git a/internal/checks/promql_regexp.go b/internal/checks/promql_regexp.go index 1a257198..4f7f88cb 100644 --- a/internal/checks/promql_regexp.go +++ b/internal/checks/promql_regexp.go @@ -5,6 +5,8 @@ import ( "fmt" "regexp" "regexp/syntax" + "slices" + "strings" "github.com/prometheus/common/model" "github.com/prometheus/prometheus/model/labels" @@ -57,6 +59,10 @@ func (c RegexpCheck) Check(_ context.Context, _ discovery.Path, rule parser.Rule if _, ok := done[selector.String()]; ok { continue } + + good := make([]*labels.Matcher, 0, len(selector.LabelMatchers)) + bad := make([]badMatcher, 0, len(selector.LabelMatchers)) + var name string for _, lm := range selector.LabelMatchers { if lm.Name == model.MetricNameLabel && lm.Type == labels.MatchEqual { @@ -67,6 +73,7 @@ func (c RegexpCheck) Check(_ context.Context, _ discovery.Path, rule parser.Rule done[selector.String()] = struct{}{} for _, lm := range selector.LabelMatchers { if lm.Type != labels.MatchRegexp && lm.Type != labels.MatchNotRegexp { + good = append(good, lm) continue } @@ -78,7 +85,7 @@ func (c RegexpCheck) Check(_ context.Context, _ discovery.Path, rule parser.Rule re = "^(?:" + re + ")$" } - var hasFlags, isUseful, isWildcard, isLiteral bool + var hasFlags, isUseful, isWildcard, isLiteral, isBad bool var beginText, endText int r, _ := syntax.Parse(re, syntax.Perl) for _, s := range r.Sub { @@ -123,37 +130,76 @@ func (c RegexpCheck) Check(_ context.Context, _ discovery.Path, rule parser.Rule case labels.MatchNotRegexp: op = labels.MatchNotEqual } - var text string - switch { - case isWildcard && op == labels.MatchEqual: - text = fmt.Sprintf("Unnecessary wildcard regexp, simply use `%s` if you want to match on all `%s` values.", name, lm.Name) - case isWildcard && op == labels.MatchNotEqual: - text = fmt.Sprintf("Unnecessary wildcard regexp, simply use `%s{%s=\"\"}` if you want to match on all time series for `%s` without the `%s` label.", name, lm.Name, name, lm.Name) - default: - text = fmt.Sprintf("Unnecessary regexp match on static string `%s`, use `%s%s%q` instead.", lm, lm.Name, op, lm.Value) - - } - problems = append(problems, Problem{ - Lines: expr.Value.Lines, - Reporter: c.Reporter(), - Text: text, - Details: RegexpCheckDetails, - Severity: Bug, - }) + bad = append(bad, badMatcher{lm: lm, op: op, isWildcard: isWildcard}) + isBad = true } if beginText > 1 || endText > 1 { - problems = append(problems, Problem{ - Lines: expr.Value.Lines, - Reporter: c.Reporter(), - Text: fmt.Sprintf("Prometheus regexp matchers are automatically fully anchored so match for `%s` will result in `%s%s\"^%s$\"`, remove regexp anchors `^` and/or `$`.", - lm, lm.Name, lm.Type, lm.Value, - ), - Details: RegexpCheckDetails, - Severity: Bug, - }) + bad = append(bad, badMatcher{lm: lm, badAnchor: true}) + isBad = true + } + if !isBad { + good = append(good, lm) } } + for _, b := range bad { + var text string + switch { + case b.badAnchor: + text = fmt.Sprintf("Prometheus regexp matchers are automatically fully anchored so match for `%s` will result in `%s%s\"^%s$\"`, remove regexp anchors `^` and/or `$`.", + b.lm, b.lm.Name, b.lm.Type, b.lm.Value, + ) + case b.isWildcard && b.op == labels.MatchEqual: + text = fmt.Sprintf("Unnecessary wildcard regexp, simply use `%s` if you want to match on all `%s` values.", + makeLabel(name, good...), b.lm.Name) + case b.isWildcard && b.op == labels.MatchNotEqual: + text = fmt.Sprintf("Unnecessary wildcard regexp, simply use `%s` if you want to match on all time series for `%s` without the `%s` label.", + makeLabel(name, slices.Concat(good, []*labels.Matcher{{Type: labels.MatchEqual, Name: b.lm.Name, Value: ""}})...), name, b.lm.Name) + default: + text = fmt.Sprintf("Unnecessary regexp match on static string `%s`, use `%s%s%q` instead.", + b.lm, b.lm.Name, b.op, b.lm.Value) + + } + problems = append(problems, Problem{ + Lines: expr.Value.Lines, + Reporter: c.Reporter(), + Text: text, + Details: RegexpCheckDetails, + Severity: Bug, + }) + } } return problems } + +type badMatcher struct { + lm *labels.Matcher + op labels.MatchType + isWildcard bool + badAnchor bool +} + +func makeLabel(name string, matchers ...*labels.Matcher) string { + filtered := make([]*labels.Matcher, 0, len(matchers)) + for _, m := range matchers { + if m.Type == labels.MatchEqual && m.Name == labels.MetricName { + continue + } + filtered = append(filtered, m) + } + if len(filtered) == 0 { + return name + } + + var b strings.Builder + b.WriteString(name) + b.WriteRune('{') + for i, m := range filtered { + if i > 0 { + b.WriteString(", ") + } + b.WriteString(m.String()) + } + b.WriteRune('}') + return b.String() +} diff --git a/internal/checks/promql_regexp_test.go b/internal/checks/promql_regexp_test.go index 36744c1a..9870ca3c 100644 --- a/internal/checks/promql_regexp_test.go +++ b/internal/checks/promql_regexp_test.go @@ -233,6 +233,102 @@ func TestRegexpCheck(t *testing.T) { } }, }, + { + description: "unnecessary wildcard regexp / many filters", + content: "- record: foo\n expr: foo{job=~\".*\", cluster=~\".*\", instance=\"bob\"}\n", + checker: newRegexpCheck, + prometheus: noProm, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Lines: parser.LineRange{ + First: 2, + Last: 2, + }, + Reporter: checks.RegexpCheckName, + Text: "Unnecessary wildcard regexp, simply use `foo{instance=\"bob\"}` if you want to match on all `job` values.", + Details: checks.RegexpCheckDetails, + Severity: checks.Bug, + }, + { + Lines: parser.LineRange{ + First: 2, + Last: 2, + }, + Reporter: checks.RegexpCheckName, + Text: "Unnecessary wildcard regexp, simply use `foo{instance=\"bob\"}` if you want to match on all `cluster` values.", + Details: checks.RegexpCheckDetails, + Severity: checks.Bug, + }, + } + }, + }, + { + description: "unnecessary wildcard regexp / many filters / no name", + content: `- record: foo + expr: | + {job=~".*", cluster=~".*", instance="bob"} +`, + checker: newRegexpCheck, + prometheus: noProm, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Lines: parser.LineRange{ + First: 2, + Last: 3, + }, + Reporter: checks.RegexpCheckName, + Text: "Unnecessary wildcard regexp, simply use `{instance=\"bob\"}` if you want to match on all `job` values.", + Details: checks.RegexpCheckDetails, + Severity: checks.Bug, + }, + { + Lines: parser.LineRange{ + First: 2, + Last: 3, + }, + Reporter: checks.RegexpCheckName, + Text: "Unnecessary wildcard regexp, simply use `{instance=\"bob\"}` if you want to match on all `cluster` values.", + Details: checks.RegexpCheckDetails, + Severity: checks.Bug, + }, + } + }, + }, + { + description: "unnecessary wildcard regexp / many filters / regexp name", + content: `- record: foo + expr: | + {job=~".*", __name__=~"foo|bar", cluster=~".*", instance="bob"} +`, + checker: newRegexpCheck, + prometheus: noProm, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Lines: parser.LineRange{ + First: 2, + Last: 3, + }, + Reporter: checks.RegexpCheckName, + Text: "Unnecessary wildcard regexp, simply use `{__name__=~\"foo|bar\", instance=\"bob\"}` if you want to match on all `job` values.", + Details: checks.RegexpCheckDetails, + Severity: checks.Bug, + }, + { + Lines: parser.LineRange{ + First: 2, + Last: 3, + }, + Reporter: checks.RegexpCheckName, + Text: "Unnecessary wildcard regexp, simply use `{__name__=~\"foo|bar\", instance=\"bob\"}` if you want to match on all `cluster` values.", + Details: checks.RegexpCheckDetails, + Severity: checks.Bug, + }, + } + }, + }, { description: "greedy wildcard regexp", content: "- record: foo\n expr: foo{job=~\".+\"}\n",