Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow disabling smelly selector checks #1162

Merged
merged 1 commit into from
Oct 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions cmd/pint/tests/0025_config.txt
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,9 @@ level=INFO msg="Loading configuration file" path=.pint.hcl
".*_errors",
".*_errors_.*"
]
},
{
"smelly": false
}
],
"rules": [
Expand Down Expand Up @@ -270,6 +273,10 @@ check "promql/series" {
]
}

check "promql/regexp" {
smelly = false
}

rule{ }

rule {
Expand Down
2 changes: 2 additions & 0 deletions docs/changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
20 changes: 19 additions & 1 deletion docs/checks/promql/regexp.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
5 changes: 5 additions & 0 deletions docs/examples/config.hcl
Original file line number Diff line number Diff line change
Expand Up @@ -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 ".* +.*" {
Expand Down
26 changes: 24 additions & 2 deletions internal/checks/promql_regexp.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
50 changes: 50 additions & 0 deletions internal/checks/promql_regexp_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package checks_test

import (
"context"
"testing"

"github.com/cloudflare/pint/internal/checks"
Expand Down Expand Up @@ -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)
}
2 changes: 2 additions & 0 deletions internal/config/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down