Skip to content

Commit

Permalink
Improve promql/vector_matching check query rewrites
Browse files Browse the repository at this point in the history
  • Loading branch information
prymitive committed Feb 10, 2022
1 parent 6c2ea67 commit bb2d6a2
Show file tree
Hide file tree
Showing 5 changed files with 171 additions and 82 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Changelog

## v0.10.1

### Fixed

- Fixed a number of bug with `promql/vector_matching` check.

## v0.10.0

### Changed
Expand Down
64 changes: 2 additions & 62 deletions internal/checks/promql_vectormatching.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
promParser "github.com/prometheus/prometheus/promql/parser"

"github.com/cloudflare/pint/internal/parser"
"github.com/cloudflare/pint/internal/parser/utils"
"github.com/cloudflare/pint/internal/promapi"
)

Expand Down Expand Up @@ -54,7 +55,7 @@ func (c VectorMatchingCheck) Check(ctx context.Context, rule parser.Rule) (probl
}

func (c VectorMatchingCheck) checkNode(ctx context.Context, node *parser.PromQLNode) (problems []exprProblem) {
if n, ok := removeConditions(node.Node.String()).(*promParser.BinaryExpr); ok &&
if n, ok := utils.RemoveConditions(node.Node.String()).(*promParser.BinaryExpr); ok &&
n.VectorMatching != nil &&
n.Op != promParser.LOR &&
n.Op != promParser.LUNLESS {
Expand All @@ -65,13 +66,6 @@ func (c VectorMatchingCheck) checkNode(ctx context.Context, node *parser.PromQLN
return
}

if _, ok := n.LHS.(*promParser.BinaryExpr); ok {
goto NEXT
}
if _, ok := n.RHS.(*promParser.BinaryExpr); ok {
goto NEXT
}

ignored := []model.LabelName{model.MetricNameLabel}
if !n.VectorMatching.On {
for _, name := range n.VectorMatching.MatchingLabels {
Expand Down Expand Up @@ -176,57 +170,3 @@ func stringInSlice(sl []string, v string) bool {
func areStringSlicesEqual(sla, slb []string) bool {
return reflect.DeepEqual(sla, slb)
}

func removeConditions(source string) promParser.Node {
node, _ := promParser.ParseExpr(source)
switch n := node.(type) {
case *promParser.AggregateExpr:
n.Expr = removeConditions(n.Expr.String()).(promParser.Expr)
return n
case *promParser.BinaryExpr:
var ln, rn bool
if _, ok := n.LHS.(*promParser.NumberLiteral); ok {
ln = true
}
if _, ok := n.RHS.(*promParser.NumberLiteral); ok {
rn = true
}
if ln && rn {
return &promParser.ParenExpr{}
}
if ln {
return removeConditions(n.RHS.String())
}
if rn {
return removeConditions(n.LHS.String())
}
n.LHS = removeConditions(n.LHS.String()).(promParser.Expr)
n.RHS = removeConditions(n.RHS.String()).(promParser.Expr)
return n
case *promParser.Call:
ret := promParser.Expressions{}
for _, e := range n.Args {
ret = append(ret, removeConditions(e.String()).(promParser.Expr))
}
n.Args = ret
return n
case *promParser.SubqueryExpr:
n.Expr = removeConditions(n.Expr.String()).(promParser.Expr)
return n
case *promParser.ParenExpr:
n.Expr = removeConditions(n.Expr.String()).(promParser.Expr)
return n
case *promParser.UnaryExpr:
n.Expr = removeConditions(n.Expr.String()).(promParser.Expr)
return n
case *promParser.MatrixSelector:
return node
case *promParser.StepInvariantExpr:
n.Expr = removeConditions(n.Expr.String()).(promParser.Expr)
return n
case *promParser.NumberLiteral, *promParser.StringLiteral, *promParser.VectorSelector:
return n
default:
return node
}
}
56 changes: 36 additions & 20 deletions internal/checks/promql_vectormatching_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ func TestVectorMatchingCheck(t *testing.T) {
"count(foo_with_notfound / ignoring(notfound) foo)",
"count(foo / ignoring(notfound) foo_with_notfound)",
"count(foo / bar)",
"count(up and foo)":
"count(up and foo)",
"count(memory_bytes / ignoring(job) memory_limit)":
w.WriteHeader(200)
w.Header().Set("Content-Type", "application/json")
_, _ = w.Write([]byte(`{
Expand All @@ -43,12 +44,13 @@ func TestVectorMatchingCheck(t *testing.T) {
"count(foo / missing)",
"count(foo / ignoring(xxx) app_registry)",
"count(foo / on(notfound) bar)",
"count(memory_bytes / ignoring(job) (memory_limit))",
"count((memory_bytes / ignoring(job) (memory_limit)) * on(app_name) group_left(a, b, c) app_registry)",
"count((memory_bytes / ignoring(job) memory_limit) * on(app_name) group_left(a, b, c) app_registry)",
"count(foo / on(notfound) bar_with_notfound)",
"count(foo_with_notfound / on(notfound) bar)",
"count(foo_with_notfound / ignoring(notfound) bar)",
"count(min_over_time((foo_with_notfound)[30m:1m]) / bar)":
"count(min_over_time(foo_with_notfound[30m:1m]) / bar)",
"count((foo / ignoring(notfound) foo_with_notfound) / (foo / ignoring(notfound) foo_with_notfound))",
"count((foo / ignoring(notfound) foo_with_notfound) / (memory_bytes / ignoring(job) memory_limit))":
w.WriteHeader(200)
w.Header().Set("Content-Type", "application/json")
_, _ = w.Write([]byte(`{
Expand All @@ -58,7 +60,7 @@ func TestVectorMatchingCheck(t *testing.T) {
"result":[]
}
}`))
case "topk(1, foo)":
case "topk(1, foo)", "topk(1, (foo / ignoring(notfound) foo_with_notfound))":
w.WriteHeader(200)
w.Header().Set("Content-Type", "application/json")
_, _ = w.Write([]byte(`{
Expand All @@ -78,7 +80,7 @@ func TestVectorMatchingCheck(t *testing.T) {
"result":[{"metric":{"__name__": "bar", "instance": "instance1", "job": "bar_job"},"value":[1614859502.068,"1"]}]
}
}`))
case "topk(1, foo_with_notfound)", "topk(1, min_over_time((foo_with_notfound)[30m:1m]))":
case "topk(1, foo_with_notfound)", "topk(1, min_over_time(foo_with_notfound[30m:1m]))":
w.WriteHeader(200)
w.Header().Set("Content-Type", "application/json")
_, _ = w.Write([]byte(`{
Expand All @@ -98,34 +100,24 @@ func TestVectorMatchingCheck(t *testing.T) {
"result":[{"metric":{"__name__": "bar", "instance": "instance1", "job": "bar_job", "notfound": "xxx"},"value":[1614859502.068,"1"]}]
}
}`))
case "topk(1, (memory_bytes / ignoring(job) (memory_limit)))":
case "topk(1, memory_bytes)", "topk(1, (memory_bytes / ignoring(job) memory_limit))":
w.WriteHeader(200)
w.Header().Set("Content-Type", "application/json")
_, _ = w.Write([]byte(`{
"status":"success",
"data":{
"resultType":"vector",
"result":[{"metric":{"instance": "instance1", "job": "foo_job"},"value":[1614859502.068,"1"]}]
"result":[{"metric":{"__name__": "memory_bytes", "instance": "instance1", "job": "foo_job", "dev": "mem"},"value":[1614859502.068,"1"]}]
}
}`))
case "topk(1, memory_bytes)":
case "topk(1, memory_limit)":
w.WriteHeader(200)
w.Header().Set("Content-Type", "application/json")
_, _ = w.Write([]byte(`{
"status":"success",
"data":{
"resultType":"vector",
"result":[{"metric":{"__name__": "memory_bytes", "instance": "instance1", "job": "foo_job"},"value":[1614859502.068,"1"]}]
}
}`))
case "topk(1, (memory_limit))":
w.WriteHeader(200)
w.Header().Set("Content-Type", "application/json")
_, _ = w.Write([]byte(`{
"status":"success",
"data":{
"resultType":"vector",
"result":[{"metric":{"instance": "instance1", "job": "bar_job"},"value":[1614859502.068,"1"]}]
"result":[{"metric":{"instance": "instance1", "job": "bar_job", "dev": "mem"},"value":[1614859502.068,"1"]}]
}
}`))
case "topk(1, app_registry)":
Expand Down Expand Up @@ -358,6 +350,30 @@ func TestVectorMatchingCheck(t *testing.T) {
},
},
},
{
description: "scalar",
content: "- alert: foo\n expr: (100*(1024^2))\n",
checker: checks.NewVectorMatchingCheck(promapi.NewPrometheus("prom", srv.URL, time.Second*1)),
},
{
description: "binary expression on both sides / passing",
content: "- alert: foo\n expr: (foo / ignoring(notfound) foo_with_notfound) / (foo / ignoring(notfound) foo_with_notfound)\n",
checker: checks.NewVectorMatchingCheck(promapi.NewPrometheus("prom", srv.URL, time.Second*1)),
},
{
description: "binary expression on both sides / mismatch",
content: "- alert: foo\n expr: (foo / ignoring(notfound) foo_with_notfound) / (memory_bytes / ignoring(job) memory_limit)\n",
checker: checks.NewVectorMatchingCheck(promapi.NewPrometheus("prom", srv.URL, time.Second*1)),
problems: []checks.Problem{
{
Fragment: "(foo / ignoring(notfound) foo_with_notfound) / (memory_bytes / ignoring(job) memory_limit)",
Lines: []int{2},
Reporter: "promql/vector_matching",
Text: "both sides of the query have different labels: [instance job] != [dev instance job]",
Severity: checks.Bug,
},
},
},
}
runTests(t, testCases)
}
64 changes: 64 additions & 0 deletions internal/parser/utils/conditions.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
package utils

import (
promParser "github.com/prometheus/prometheus/promql/parser"
)

func RemoveConditions(source string) promParser.Node {
node, _ := promParser.ParseExpr(source)
switch n := node.(type) {
case *promParser.AggregateExpr:
n.Expr = RemoveConditions(n.Expr.String()).(promParser.Expr)
return n
case *promParser.BinaryExpr:
lhs := RemoveConditions(n.LHS.String())
rhs := RemoveConditions(n.RHS.String())
_, ln := lhs.(*promParser.NumberLiteral)
if v, ok := lhs.(*promParser.VectorSelector); ok && v.Name == "" {
ln = true
}
_, rn := rhs.(*promParser.NumberLiteral)
if v, ok := rhs.(*promParser.VectorSelector); ok && v.Name == "" {
rn = true
}
if ln && rn {
return &promParser.VectorSelector{}
}
if ln {
return rhs
}
if rn {
return lhs
}
n.LHS = lhs.(promParser.Expr)
n.RHS = rhs.(promParser.Expr)
return n
case *promParser.Call:
ret := promParser.Expressions{}
for _, e := range n.Args {
ret = append(ret, RemoveConditions(e.String()).(promParser.Expr))
}
n.Args = ret
return n
case *promParser.SubqueryExpr:
n.Expr = RemoveConditions(n.Expr.String()).(promParser.Expr)
return n
case *promParser.ParenExpr:
n.Expr = RemoveConditions(n.Expr.String()).(promParser.Expr)
switch n.Expr.(type) {
case *promParser.NumberLiteral, *promParser.StringLiteral, *promParser.VectorSelector, *promParser.MatrixSelector:
return n.Expr
}
return n
case *promParser.UnaryExpr:
n.Expr = RemoveConditions(n.Expr.String()).(promParser.Expr)
return n
case *promParser.StepInvariantExpr:
n.Expr = RemoveConditions(n.Expr.String()).(promParser.Expr)
return n
case *promParser.NumberLiteral, *promParser.StringLiteral, *promParser.VectorSelector, *promParser.MatrixSelector:
return node
default:
return node
}
}
63 changes: 63 additions & 0 deletions internal/parser/utils/conditions_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package utils_test

import (
"testing"

"github.com/stretchr/testify/assert"

"github.com/cloudflare/pint/internal/parser/utils"
)

func TestRemoveConditions(t *testing.T) {
type testCaseT struct {
input string
output string
}

testCases := []testCaseT{
{
input: "100",
output: "100",
},
{
input: "(100)",
output: "100",
},
{
input: "100 ^ 2",
output: "",
},
{
input: "(1024 ^ 2)",
output: "",
},
{
input: "(100*(1024^2))",
output: "",
},
{
input: "min_over_time((foo_with_notfound > 0)[30m:1m]) / bar",
output: "min_over_time(foo_with_notfound[30m:1m]) / bar",
},
{
input: "min_over_time(rate(http_requests_total[5m])[30m:1m])",
output: "min_over_time(rate(http_requests_total[5m])[30m:1m])",
},
{
input: "(memory_bytes / ignoring(job) (memory_limit > 0)) * on(app_name) group_left(a,b,c) app_registry",
output: "(memory_bytes / ignoring(job) memory_limit) * on(app_name) group_left(a, b, c) app_registry",
},
{
input: `(quantile_over_time(0.9, (rate(container_cpu_system_seconds_total{app_name="foo"}[5m]) + rate(container_cpu_user_seconds_total{app_name="foo"}[5m]))[5m:]) / on(instance) bar) > 0.65`,
output: `(quantile_over_time(0.9, (rate(container_cpu_system_seconds_total{app_name="foo"}[5m]) + rate(container_cpu_user_seconds_total{app_name="foo"}[5m]))[5m:]) / on(instance) bar)`,
},
}

for _, tc := range testCases {
t.Run(tc.input, func(t *testing.T) {
assert := assert.New(t)
output := utils.RemoveConditions(tc.input)
assert.Equalf(tc.output, output.String(), "input: %q", tc.input)
})
}
}

0 comments on commit bb2d6a2

Please sign in to comment.