Improve promql/regexp messages

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

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=

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()
+}

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",