Simplify rate check

cloudflare · Mar 17, 2022 · dda1f61 · dda1f61
1 parent 6612da2
commit dda1f61
Show file tree

Hide file tree

Showing 5 changed files with 10 additions and 100 deletions.
diff --git a/docs/changelog.md b/docs/changelog.md
@@ -8,6 +8,8 @@
 - `promql/series` check was refactored and will now detect a range of
   problems. See [promql/series](checks/promql/series.md) for details.
 - `promql/regexp` severity is now `Bug` instead of a `Warning`.
+- `promql/rate` check will no longer produce warnings, it will only
+  report issues that cause queries to never return anything.
 
 ## v0.14.0
 

diff --git a/docs/checks/promql/rate.md b/docs/checks/promql/rate.md
@@ -9,15 +9,7 @@ grand_parent: Documentation
 This check inspects `rate()` and `irate()` functions and warns if used duration
 is too low. It does so by first getting global `scrape_interval` value for selected
 Prometheus servers and comparing duration to it.
-Reported issue depends on a few factors:
-
-For `rate()` function:
-- If duration is less than 2x `scrape_interval` it will report a bug.
-- If duration is between 2x and 4x `scrape_interval` it will report a warning.
-
-For `irate()` function:
-- If duration is less than 2x `scrape_interval` it will report a bug.
-- If duration is between 2x and 3x `scrape_interval` it will report a warning.
+It will report a bug if duration is less than 2x `scrape_interval`.
 
 ## Configuration
 

diff --git a/docs/checks/promql/series.md b/docs/checks/promql/series.md
@@ -28,7 +28,7 @@ of different issues. Here are some usual cases.
 
 ## Your query is using ALERTS or ALERTS_FOR_STATE metrics
 
-Prometheus itself exposes metrics about active alerts [see docs here](https://prometheus.io/docs/prometheus/latest/configuration/alerting_rules/#inspecting-alerts-during-runtime.
+Prometheus itself exposes [metrics about active alerts](https://prometheus.io/docs/prometheus/latest/configuration/alerting_rules/#inspecting-alerts-during-runtime).
 And it's possible to use those metrics in recording or alerting rules.
 If pint finds a query using either `ALERTS{alertname="..."}` or
 `ALERTS_FOR_STATE{alertname="..."}` selector it will check if there's

diff --git a/internal/checks/promql_rate.go b/internal/checks/promql_rate.go
@@ -69,14 +69,11 @@ func (c RateCheck) Check(ctx context.Context, rule parser.Rule, entries []discov
 func (c RateCheck) checkNode(node *parser.PromQLNode, cfg *promapi.ConfigResult) (problems []exprProblem) {
 	if n, ok := node.Node.(*promParser.Call); ok && (n.Func.Name == "rate" || n.Func.Name == "irate") {
 		var minIntervals int
-		var recIntervals int
 		switch n.Func.Name {
 		case "rate":
 			minIntervals = 2
-			recIntervals = 4
 		case "irate":
 			minIntervals = 2
-			recIntervals = 3
 		}
 		for _, arg := range n.Args {
 			if m, ok := arg.(*promParser.MatrixSelector); ok {
@@ -88,14 +85,6 @@ func (c RateCheck) checkNode(node *parser.PromQLNode, cfg *promapi.ConfigResult)
 						severity: Bug,
 					}
 					problems = append(problems, p)
-				} else if m.Range < cfg.Config.Global.ScrapeInterval*time.Duration(recIntervals) {
-					p := exprProblem{
-						expr: node.Expr,
-						text: fmt.Sprintf("duration for %s() is recommended to be at least %d x scrape_interval, %s is using %s scrape_interval",
-							n.Func.Name, recIntervals, promText(c.prom.Name(), cfg.URI), output.HumanizeDuration(cfg.Config.Global.ScrapeInterval)),
-						severity: Warning,
-					}
-					problems = append(problems, p)
 				}
 			}
 		}

diff --git a/internal/checks/promql_rate_test.go b/internal/checks/promql_rate_test.go
@@ -16,10 +16,6 @@ func durationMustText(name, uri, fun, multi, using string) string {
 	return fmt.Sprintf(`duration for %s() must be at least %s x scrape_interval, prometheus %q at %s is using %s scrape_interval`, fun, multi, name, uri, using)
 }
 
-func durationRecommenedText(name, uri, fun, multi, using string) string {
-	return fmt.Sprintf("duration for %s() is recommended to be at least %s x scrape_interval, prometheus %q at %s is using %s scrape_interval", fun, multi, name, uri, using)
-}
-
 func TestRateCheck(t *testing.T) {
 	testCases := []checkTest{
 		{
@@ -54,17 +50,7 @@ func TestRateCheck(t *testing.T) {
 			description: "rate < 4x scrape_interval",
 			content:     "- record: foo\n  expr: rate(foo[3m])\n",
 			checker:     newRateCheck,
-			problems: func(uri string) []checks.Problem {
-				return []checks.Problem{
-					{
-						Fragment: "rate(foo[3m])",
-						Lines:    []int{2},
-						Reporter: "promql/rate",
-						Text:     durationRecommenedText("prom", uri, "rate", "4", "1m"),
-						Severity: checks.Warning,
-					},
-				}
-			},
+			problems:    noProblems,
 			mocks: []*prometheusMock{
 				{
 					conds: []requestCondition{requireConfigPath},
@@ -110,17 +96,7 @@ func TestRateCheck(t *testing.T) {
 			description: "irate < 3x scrape_interval",
 			content:     "- record: foo\n  expr: irate(foo[2m])\n",
 			checker:     newRateCheck,
-			problems: func(uri string) []checks.Problem {
-				return []checks.Problem{
-					{
-						Fragment: "irate(foo[2m])",
-						Lines:    []int{2},
-						Reporter: "promql/rate",
-						Text:     durationRecommenedText("prom", uri, "irate", "3", "1m"),
-						Severity: checks.Warning,
-					},
-				}
-			},
+			problems:    noProblems,
 			mocks: []*prometheusMock{
 				{
 					conds: []requestCondition{requireConfigPath},
@@ -164,18 +140,8 @@ func TestRateCheck(t *testing.T) {
 - record: foo
   expr: irate({__name__="foo"}[2m])
 `,
-			checker: newRateCheck,
-			problems: func(uri string) []checks.Problem {
-				return []checks.Problem{
-					{
-						Fragment: `irate({__name__="foo"}[2m])`,
-						Lines:    []int{3},
-						Reporter: "promql/rate",
-						Text:     durationRecommenedText("prom", uri, "irate", "3", "1m"),
-						Severity: checks.Warning,
-					},
-				}
-			},
+			checker:  newRateCheck,
+			problems: noProblems,
 			mocks: []*prometheusMock{
 				{
 					conds: []requestCondition{requireConfigPath},
@@ -189,18 +155,8 @@ func TestRateCheck(t *testing.T) {
 - record: foo
   expr: irate({__name__=~"(foo|bar)_total"}[2m])
 `,
-			checker: newRateCheck,
-			problems: func(uri string) []checks.Problem {
-				return []checks.Problem{
-					{
-						Fragment: `irate({__name__=~"(foo|bar)_total"}[2m])`,
-						Lines:    []int{3},
-						Reporter: "promql/rate",
-						Text:     durationRecommenedText("prom", uri, "irate", "3", "1m"),
-						Severity: checks.Warning,
-					},
-				}
-			},
+			checker:  newRateCheck,
+			problems: noProblems,
 			mocks: []*prometheusMock{
 				{
 					conds: []requestCondition{requireConfigPath},
@@ -238,13 +194,6 @@ func TestRateCheck(t *testing.T) {
 			checker:     newRateCheck,
 			problems: func(uri string) []checks.Problem {
 				return []checks.Problem{
-					{
-						Fragment: "rate(foo[3m])",
-						Lines:    []int{2},
-						Reporter: "promql/rate",
-						Text:     durationRecommenedText("prom", uri, "rate", "4", "1m"),
-						Severity: checks.Warning,
-					},
 					{
 						Fragment: "rate(bar[1m])",
 						Lines:    []int{2},
@@ -358,28 +307,6 @@ func TestRateCheck(t *testing.T) {
 				},
 			},
 		},
-		{
-			description: "irate < 3 x default 1m",
-			content:     "- record: foo\n  expr: irate(foo[2m])\n",
-			checker:     newRateCheck,
-			problems: func(uri string) []checks.Problem {
-				return []checks.Problem{
-					{
-						Fragment: "irate(foo[2m])",
-						Lines:    []int{2},
-						Reporter: "promql/rate",
-						Text:     durationRecommenedText("prom", uri, "irate", "3", "1m"),
-						Severity: checks.Warning,
-					},
-				}
-			},
-			mocks: []*prometheusMock{
-				{
-					conds: []requestCondition{requireConfigPath},
-					resp:  configResponse{yaml: "global: {}\n"},
-				},
-			},
-		},
 	}
 	runTests(t, testCases)
 }