Use Source{} for humanize checks

cloudflare · Oct 29, 2024 · c6c6cd4 · c6c6cd4
1 parent 870a7cd
commit c6c6cd4
Show file tree

Hide file tree

Showing 5 changed files with 19 additions and 19 deletions.
diff --git a/internal/checks/alerts_absent.go b/internal/checks/alerts_absent.go
@@ -59,7 +59,7 @@ func (c AlertsAbsentCheck) Check(ctx context.Context, _ discovery.Path, rule par
 	}
 
 	var hasAbsent bool
-	src := utils.LabelsSource(rule.AlertingRule.Expr.Value.Value, rule.AlertingRule.Expr.Query)
+	src := utils.LabelsSource(rule.AlertingRule.Expr.Value.Value, rule.AlertingRule.Expr.Query.Expr)
 	for _, s := range append(src.Alternatives, src) {
 		if s.Operation == "absent" {
 			hasAbsent = true

diff --git a/internal/checks/alerts_template.go b/internal/checks/alerts_template.go
@@ -109,7 +109,7 @@ func (c TemplateCheck) Check(ctx context.Context, _ discovery.Path, rule parser.
 		return nil
 	}
 
-	src := utils.LabelsSource(rule.AlertingRule.Expr.Value.Value, rule.AlertingRule.Expr.Query)
+	src := utils.LabelsSource(rule.AlertingRule.Expr.Value.Value, rule.AlertingRule.Expr.Query.Expr)
 	data := promTemplate.AlertTemplateData(map[string]string{}, map[string]string{}, "", promql.Sample{})
 
 	if rule.AlertingRule.Labels != nil {
@@ -182,7 +182,7 @@ func (c TemplateCheck) Check(ctx context.Context, _ discovery.Path, rule parser.
 			}
 
 			if hasValue(annotation.Key.Value, annotation.Value.Value) && !hasHumanize(annotation.Key.Value, annotation.Value.Value) {
-				for _, problem := range c.checkHumanizeIsNeeded(rule.AlertingRule.Expr.Query) {
+				for _, problem := range c.checkHumanizeIsNeeded(src) {
 					problems = append(problems, Problem{
 						Lines: parser.LineRange{
 							First: annotation.Key.Lines.First,
@@ -200,13 +200,15 @@ func (c TemplateCheck) Check(ctx context.Context, _ discovery.Path, rule parser.
 	return problems
 }
 
-func (c TemplateCheck) checkHumanizeIsNeeded(node *parser.PromQLNode) (problems []exprProblem) {
-	for _, call := range utils.HasOuterRate(node) {
-		problems = append(problems, exprProblem{
-			text:     fmt.Sprintf("Using the value of `%s` inside this annotation might be hard to read, consider using one of humanize template functions to make it more human friendly.", call),
-			details:  "[Click here](https://prometheus.io/docs/prometheus/latest/configuration/template_reference/) for a full list of all available template functions.",
-			severity: Information,
-		})
+func (c TemplateCheck) checkHumanizeIsNeeded(src utils.Source) (problems []exprProblem) {
+	for _, s := range append(src.Alternatives, src) {
+		if s.Operation == "rate" || s.Operation == "irate" || s.Operation == "deriv" {
+			problems = append(problems, exprProblem{
+				text:     fmt.Sprintf("Using the value of `%s` inside this annotation might be hard to read, consider using one of humanize template functions to make it more human friendly.", s.Call),
+				details:  "[Click here](https://prometheus.io/docs/prometheus/latest/configuration/template_reference/) for a full list of all available template functions.",
+				severity: Information,
+			})
+		}
 	}
 	return problems
 }

diff --git a/internal/checks/promql_fragile.go b/internal/checks/promql_fragile.go
@@ -64,7 +64,7 @@ func (c FragileCheck) Check(_ context.Context, _ discovery.Path, rule parser.Rul
 	}
 
 	if rule.AlertingRule != nil {
-		for _, problem := range c.checkSampling(expr.Value.Value, expr.Query) {
+		for _, problem := range c.checkSampling(expr.Value.Value, expr.Query.Expr) {
 			problems = append(problems, Problem{
 				Lines:    expr.Value.Lines,
 				Reporter: c.Reporter(),
@@ -126,7 +126,7 @@ NEXT:
 	return problems
 }
 
-func (c FragileCheck) checkSampling(expr string, node *parser.PromQLNode) (problems []exprProblem) {
+func (c FragileCheck) checkSampling(expr string, node promParser.Node) (problems []exprProblem) {
 	s := utils.LabelsSource(expr, node)
 	for _, src := range append(s.Alternatives, s) {
 		if src.Type != utils.AggregateSource {

diff --git a/internal/parser/utils/source.go b/internal/parser/utils/source.go
@@ -5,8 +5,6 @@ import (
 	"slices"
 	"strings"
 
-	"github.com/cloudflare/pint/internal/parser"
-
 	"github.com/prometheus/prometheus/model/labels"
 	promParser "github.com/prometheus/prometheus/promql/parser"
 	"github.com/prometheus/prometheus/promql/parser/posrange"
@@ -42,8 +40,8 @@ type Source struct {
 	FixedLabels      bool // Labels are fixed and only allowed labels can be present.
 }
 
-func LabelsSource(expr string, node *parser.PromQLNode) Source {
-	return walkNode(expr, node.Expr)
+func LabelsSource(expr string, node promParser.Node) Source {
+	return walkNode(expr, node)
 }
 
 func walkNode(expr string, node promParser.Node) (s Source) {

diff --git a/internal/parser/utils/source_test.go b/internal/parser/utils/source_test.go
@@ -1616,7 +1616,7 @@ or avg without(router, colo_id, instance) (router_anycast_prefix_enabled{cidr_us
 				t.Error(err)
 				t.FailNow()
 			}
-			output := utils.LabelsSource(tc.expr, n)
+			output := utils.LabelsSource(tc.expr, n.Expr)
 			require.EqualExportedValues(t, tc.output, output)
 		})
 	}
@@ -1655,7 +1655,7 @@ func TestLabelsSourceCallCoverage(t *testing.T) {
 				t.Error(err)
 				t.FailNow()
 			}
-			output := utils.LabelsSource(b.String(), n)
+			output := utils.LabelsSource(b.String(), n.Expr)
 			require.NotNil(t, output.Call, "no call detected in: %q ~> %+v", b.String(), output)
 			require.Equal(t, name, output.Operation)
 			require.Equal(t, def.ReturnType, output.Returns, "incorrect return type on Source{}")
@@ -1671,6 +1671,6 @@ func TestLabelsSourceCallCoverageFail(t *testing.T) {
 			},
 		},
 	}
-	output := utils.LabelsSource("fake_call()", n)
+	output := utils.LabelsSource("fake_call()", n.Expr)
 	require.Nil(t, output.Call, "no call should have been detected in fake function")
 }