Merge pull request #1162 from cloudflare/smelly

Allow disabling smelly selector checks

cmd/pint/tests/0025_config.txt

@@ -135,6 +135,9 @@ level=INFO msg="Loading configuration file" path=.pint.hcl
         ".*_errors",
         ".*_errors_.*"
       ]
+    },
+    {
+      "smelly": false
     }
   ],
   "rules": [
@@ -270,6 +273,10 @@ check "promql/series" {
   ]
 }
 
+check "promql/regexp" {
+  smelly = false
+}
+
 rule{  }
 
 rule {

docs/changelog.md

@@ -7,6 +7,8 @@
 - [promql/fragile](checks/promql/fragile.md) will now warn when alerting rules are using
   one of the aggregation operation that can return different series on every evaluation,
   which can cause alert floppiness - [#820](https://github.com/cloudflare/pint/issues/820).
+- [promql/regexp](checks/promql/regexp.md) check now supports extra configuration options
+  to disable reports on smelly selector - [#1096](https://github.com/cloudflare/pint/issues/1096).
 
 ### Fixed
 

docs/checks/promql/regexp.md

@@ -141,7 +141,25 @@ And simple explicit queries:
 
 ## Configuration
 
-This check doesn't have any configuration options.
+This check supports setting extra configuration option to fine tune its behaviour.
+
+Syntax:
+
+```js
+check "promql/regexp" {
+  smelly = true|false
+}
+```
+
+- `smelly` - enable or disable reports about smelly selectors. This is enabled by default.
+
+Example:
+
+```js
+check "promql/regexp" {
+  smelly = false
+}
+```
 
 ## How to enable it
 

docs/examples/config.hcl

@@ -23,6 +23,11 @@ prometheus "dev" {
   timeout = "60s"
 }
 
+# Disable smelly selectors warning in promql/regexp check.
+check "promql/regexp" {
+  smelly = false
+}
+
 rule {
   # Disallow spaces in label/annotation keys, they're only allowed in values.
   reject ".* +.*" {

internal/checks/promql_regexp.go

@@ -22,6 +22,19 @@ const (
 	RegexpCheckDetails = `See [Prometheus documentation](https://prometheus.io/docs/prometheus/latest/querying/basics/#time-series-selectors) for details on how vector selectors work.`
 )
 
+type PromqlRegexpSettings struct {
+	Smelly        *bool `hcl:"smelly,optional" json:"smelly,omitempty"`
+	smellyEnabled bool
+}
+
+func (c *PromqlRegexpSettings) Validate() error {
+	c.smellyEnabled = true
+	if c.Smelly != nil {
+		c.smellyEnabled = *c.Smelly
+	}
+	return nil
+}
+
 func NewRegexpCheck() RegexpCheck {
 	return RegexpCheck{}
 }
@@ -48,12 +61,21 @@ func (c RegexpCheck) Reporter() string {
 	return RegexpCheckName
 }
 
-func (c RegexpCheck) Check(_ context.Context, _ discovery.Path, rule parser.Rule, _ []discovery.Entry) (problems []Problem) {
+func (c RegexpCheck) Check(ctx context.Context, _ discovery.Path, rule parser.Rule, _ []discovery.Entry) (problems []Problem) {
 	expr := rule.Expr()
 	if expr.SyntaxError != nil {
 		return nil
 	}
 
+	var settings *PromqlRegexpSettings
+	if s := ctx.Value(SettingsKey(c.Reporter())); s != nil {
+		settings = s.(*PromqlRegexpSettings)
+	}
+	if settings == nil {
+		settings = &PromqlRegexpSettings{}
+		_ = settings.Validate()
+	}
+
 	done := map[string]struct{}{}
 	for _, selector := range utils.HasVectorSelector(expr.Query) {
 		if _, ok := done[selector.String()]; ok {
@@ -142,7 +164,7 @@ func (c RegexpCheck) Check(_ context.Context, _ discovery.Path, rule parser.Rule
 				bad = append(bad, badMatcher{lm: lm, badAnchor: true})
 				isBad = true
 			}
-			if isSmelly {
+			if settings.smellyEnabled && isSmelly {
 				bad = append(bad, badMatcher{lm: lm, isSmelly: true})
 			}
 			if !isBad {

internal/checks/promql_regexp_test.go

@@ -1,6 +1,7 @@
 package checks_test
 
 import (
+	"context"
 	"testing"
 
 	"github.com/cloudflare/pint/internal/checks"
@@ -431,6 +432,55 @@ func TestRegexpCheck(t *testing.T) {
 			prometheus:  noProm,
 			problems:    noProblems,
 		},
+		{
+			description: "smelly selector / enabled",
+			content:     "- record: foo\n  expr: foo{job=~\"service_.*_prod\"}\n",
+			checker:     newRegexpCheck,
+			ctx: func(ctx context.Context, _ string) context.Context {
+				yes := true
+				s := checks.PromqlRegexpSettings{
+					Smelly: &yes,
+				}
+				if err := s.Validate(); err != nil {
+					t.Error(err)
+					t.FailNow()
+				}
+				return context.WithValue(ctx, checks.SettingsKey(checks.RegexpCheckName), &s)
+			},
+			prometheus: noProm,
+			problems: func(_ string) []checks.Problem {
+				return []checks.Problem{
+					{
+						Lines: parser.LineRange{
+							First: 2,
+							Last:  2,
+						},
+						Reporter: checks.RegexpCheckName,
+						Text:     "`{job=~\"service_.*_prod\"}` looks like a smelly selector that tries to extract substrings from the value, please consider breaking down the value of this label into multiple smaller labels",
+						Details:  checks.RegexpCheckDetails,
+						Severity: checks.Warning,
+					},
+				}
+			},
+		},
+		{
+			description: "smelly selector / disabled",
+			content:     "- record: foo\n  expr: foo{job=~\"service_.*_prod\"}\n",
+			checker:     newRegexpCheck,
+			ctx: func(ctx context.Context, _ string) context.Context {
+				no := false
+				s := checks.PromqlRegexpSettings{
+					Smelly: &no,
+				}
+				if err := s.Validate(); err != nil {
+					t.Error(err)
+					t.FailNow()
+				}
+				return context.WithValue(ctx, checks.SettingsKey(checks.RegexpCheckName), &s)
+			},
+			prometheus: noProm,
+			problems:   noProblems,
+		},
 	}
 	runTests(t, testCases)
 }

internal/config/check.go

@@ -27,6 +27,8 @@ func (c Check) Decode() (s CheckSettings, err error) {
 	switch c.Name {
 	case checks.SeriesCheckName:
 		s = &checks.PromqlSeriesSettings{}
+	case checks.RegexpCheckName:
+		s = &checks.PromqlRegexpSettings{}
 	default:
 		return nil, fmt.Errorf("unknown check %q", c.Name)
 	}