Merge pull request cloudflare#44 from cloudflare/OBS-868

Ignore right hand side of aggregation in by/without checks
linode-obs · Sep 9, 2021 · e93061d · e93061d
2 parents 9af3658 + 01aa6cf
commit e93061d
Show file tree

Hide file tree

Showing 4 changed files with 92 additions and 2 deletions.
diff --git a/internal/checks/by.go b/internal/checks/by.go
@@ -5,6 +5,7 @@ import (
 	"regexp"
 
 	"github.com/cloudflare/pint/internal/parser"
+	"github.com/rs/zerolog/log"
 
 	promParser "github.com/prometheus/prometheus/promql/parser"
 )
@@ -60,7 +61,26 @@ func (c ByCheck) Check(rule parser.Rule) (problems []Problem) {
 }
 
 func (c ByCheck) checkNode(node *parser.PromQLNode) (problems []exprProblem) {
-	if n, ok := node.Node.(*promParser.AggregateExpr); ok && !n.Without && n.Op != promParser.TOPK {
+	if n, ok := node.Node.(*promParser.AggregateExpr); ok && !n.Without {
+		switch n.Op {
+		case promParser.SUM:
+		case promParser.MIN:
+		case promParser.MAX:
+		case promParser.AVG:
+		case promParser.GROUP:
+		case promParser.STDDEV:
+		case promParser.STDVAR:
+		case promParser.COUNT:
+		case promParser.COUNT_VALUES:
+		case promParser.BOTTOMK:
+			goto NEXT
+		case promParser.TOPK:
+			goto NEXT
+		case promParser.QUANTILE:
+		default:
+			log.Warn().Str("op", n.Op.String()).Msg("Unsupported aggregation operation")
+		}
+
 		var found bool
 		for _, g := range n.Grouping {
 			if g == c.label {
@@ -84,6 +104,12 @@ func (c ByCheck) checkNode(node *parser.PromQLNode) (problems []exprProblem) {
 		}
 	}
 
+NEXT:
+	if n, ok := node.Node.(*promParser.BinaryExpr); ok && n.Op == promParser.LAND {
+		problems = append(problems, c.checkNode(node.Children[0])...)
+		return
+	}
+
 	for _, child := range node.Children {
 		problems = append(problems, c.checkNode(child)...)
 	}

diff --git a/internal/checks/by_test.go b/internal/checks/by_test.go
@@ -85,6 +85,25 @@ func TestByCheck(t *testing.T) {
 				},
 			},
 		},
+		{
+			description: "Right hand side of AND is ignored",
+			content:     "- record: foo\n  expr: foo AND on(instance) max(bar)\n",
+			checker:     checks.NewByCheck(regexp.MustCompile("^.+$"), "job", true, checks.Warning),
+		},
+		{
+			description: "Left hand side of AND is checked",
+			content:     "- record: foo\n  expr: max (foo) by(instance) AND on(instance) bar\n",
+			checker:     checks.NewByCheck(regexp.MustCompile("^.+$"), "job", true, checks.Warning),
+			problems: []checks.Problem{
+				{
+					Fragment: "max by(instance) (foo)",
+					Lines:    []int{2},
+					Reporter: "promql/by",
+					Text:     "job label is required and should be preserved when aggregating \"^.+$\" rules, use by(job, ...)",
+					Severity: checks.Warning,
+				},
+			},
+		},
 	}
 	runTests(t, testCases)
 }
diff --git a/internal/checks/without.go b/internal/checks/without.go
@@ -5,6 +5,7 @@ import (
 	"regexp"
 
 	"github.com/cloudflare/pint/internal/parser"
+	"github.com/rs/zerolog/log"
 
 	promParser "github.com/prometheus/prometheus/promql/parser"
 )
@@ -61,7 +62,26 @@ func (c WithoutCheck) Check(rule parser.Rule) (problems []Problem) {
 }
 
 func (c WithoutCheck) checkNode(node *parser.PromQLNode) (problems []exprProblem) {
-	if n, ok := node.Node.(*promParser.AggregateExpr); ok && n.Without && n.Op != promParser.TOPK {
+	if n, ok := node.Node.(*promParser.AggregateExpr); ok && n.Without {
+		switch n.Op {
+		case promParser.SUM:
+		case promParser.MIN:
+		case promParser.MAX:
+		case promParser.AVG:
+		case promParser.GROUP:
+		case promParser.STDDEV:
+		case promParser.STDVAR:
+		case promParser.COUNT:
+		case promParser.COUNT_VALUES:
+		case promParser.BOTTOMK:
+			goto NEXT
+		case promParser.TOPK:
+			goto NEXT
+		case promParser.QUANTILE:
+		default:
+			log.Warn().Str("op", n.Op.String()).Msg("Unsupported aggregation operation")
+		}
+
 		var found bool
 		for _, g := range n.Grouping {
 			if g == c.label {
@@ -91,6 +111,12 @@ func (c WithoutCheck) checkNode(node *parser.PromQLNode) (problems []exprProblem
 		}
 	}
 
+NEXT:
+	if n, ok := node.Node.(*promParser.BinaryExpr); ok && n.Op == promParser.LAND {
+		problems = append(problems, c.checkNode(node.Children[0])...)
+		return
+	}
+
 	for _, child := range node.Children {
 		problems = append(problems, c.checkNode(child)...)
 	}

diff --git a/internal/checks/without_test.go b/internal/checks/without_test.go
@@ -130,6 +130,25 @@ func TestWithoutCheck(t *testing.T) {
 				},
 			},
 		},
+		{
+			description: "Right hand side of AND is ignored",
+			content:     "- record: foo\n  expr: foo AND on(instance) max(bar) without()\n",
+			checker:     checks.NewWithoutCheck(regexp.MustCompile("^.+$"), "job", true, checks.Warning),
+		},
+		{
+			description: "Left hand side of AND is checked",
+			content:     "- record: foo\n  expr: max (foo) without(job) AND on(instance) bar\n",
+			checker:     checks.NewWithoutCheck(regexp.MustCompile("^.+$"), "job", true, checks.Warning),
+			problems: []checks.Problem{
+				{
+					Fragment: "max without(job) (foo)",
+					Lines:    []int{2},
+					Reporter: "promql/without",
+					Text:     "job label is required and should be preserved when aggregating \"^.+$\" rules, remove job from without()",
+					Severity: checks.Warning,
+				},
+			},
+		},
 	}
 	runTests(t, testCases)
 }