Skip to content

Commit

Permalink
Merge pull request #211 from cloudflare/binex
Browse files Browse the repository at this point in the history 
Make absent() checks aware of binary expressions
  • Loading branch information
@prymitive
prymitive authored Apr 1, 2022
2 parents 40b7cf8 + f08676c commit 597f07d
Show file tree
Hide file tree
Showing 5 changed files with 195 additions and 50 deletions.
7 changes: 7 additions & 0 deletions docs/changelog.md
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
# Changelog

## v0.16.1

### Fixed

- Fixed false positive reports from `alerts/template` check when `absent()` function
is receiving labels from a binary expression.

## v0.16.0

### Added
Expand Down
22 changes: 18 additions & 4 deletions internal/checks/alerts_template.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/prometheus/common/model"
"github.com/prometheus/prometheus/model/labels"
"github.com/prometheus/prometheus/model/timestamp"
promParser "github.com/prometheus/prometheus/promql/parser"
promTemplate "github.com/prometheus/prometheus/template"

"github.com/cloudflare/pint/internal/discovery"
Expand Down Expand Up @@ -140,7 +141,7 @@ func (c TemplateCheck) Check(ctx context.Context, rule parser.Rule, entries []di
}

for _, call := range absentCalls {
if len(utils.HasOuterAggregation(call)) > 0 {
if len(utils.HasOuterAggregation(call.Fragment)) > 0 {
continue
}
for _, msg := range checkMetricLabels(msgAbsent, label.Key.Value, label.Value.Value, absentLabels(call), false) {
Expand Down Expand Up @@ -181,7 +182,14 @@ func (c TemplateCheck) Check(ctx context.Context, rule parser.Rule, entries []di
}

for _, call := range absentCalls {
if len(utils.HasOuterAggregation(call)) > 0 {
if len(utils.HasOuterAggregation(call.Fragment)) > 0 {
continue
}
if call.BinExpr != nil &&
call.BinExpr.VectorMatching != nil &&
(call.BinExpr.VectorMatching.Card == promParser.CardManyToOne ||
call.BinExpr.VectorMatching.Card == promParser.CardOneToMany) &&
len(call.BinExpr.VectorMatching.Include) == 0 {
continue
}
for _, msg := range checkMetricLabels(msgAbsent, annotation.Key.Value, annotation.Value.Value, absentLabels(call), false) {
Expand Down Expand Up @@ -350,10 +358,10 @@ func checkMetricLabels(msg, name, text string, metricLabels []string, excludeLab
return
}

func absentLabels(node *parser.PromQLNode) []string {
func absentLabels(f utils.PromQLFragment) []string {
labelMap := map[string]struct{}{}

for _, child := range node.Children {
for _, child := range f.Fragment.Children {
for _, v := range utils.HasVectorSelector(child) {
for _, lm := range v.LabelMatchers {
if lm.Type == labels.MatchEqual {
Expand All @@ -363,6 +371,12 @@ func absentLabels(node *parser.PromQLNode) []string {
}
}

if f.BinExpr != nil && f.BinExpr.VectorMatching != nil {
for _, name := range f.BinExpr.VectorMatching.Include {
labelMap[name] = struct{}{}
}
}

names := make([]string, 0, len(labelMap))
for name := range labelMap {
names = append(names, name)
Expand Down
60 changes: 60 additions & 0 deletions internal/checks/alerts_template_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,22 @@ func TestTemplateCheck(t *testing.T) {
}
},
},
{
description: "label missing from metrics (without)",
content: "- alert: Foo Is Down\n expr: sum(foo) without(job) > 0\n labels:\n summary: '{{ $labels.job }}'\n",
checker: newTemplateCheck,
problems: func(uri string) []checks.Problem {
return []checks.Problem{
{
Fragment: `summary: {{ $labels.job }}`,
Lines: []int{2, 4},
Reporter: checks.TemplateCheckName,
Text: `template is using "job" label but the query removes it`,
Severity: checks.Bug,
},
}
},
},
{
description: "annotation label missing from metrics (or)",
content: "- alert: Foo Is Down\n expr: sum(foo) by(job) or sum(bar)\n annotations:\n summary: '{{ .Labels.job }}'\n",
Expand Down Expand Up @@ -516,6 +532,50 @@ func TestTemplateCheck(t *testing.T) {
}
},
},
{
description: "absent() * on() group_left(...) foo",
content: `
- alert: Foo
expr: absent(foo{job="xxx"}) * on() group_left(cluster, env) bar
annotations:
summary: '{{ .Labels.job }} in cluster {{$labels.cluster}}/{{ $labels.env }} is missing'
`,
checker: newTemplateCheck,
problems: noProblems,
},
{
description: "absent() * on() group_left() bar",
content: `
- alert: Foo
expr: absent(foo{job="xxx"}) * on() group_left() bar
annotations:
summary: '{{ .Labels.job }} in cluster {{$labels.cluster}}/{{ $labels.env }} is missing'
`,
checker: newTemplateCheck,
problems: noProblems,
},
{
description: "bar * on() group_right(...) absent()",
content: `
- alert: Foo
expr: bar * on() group_right(cluster, env) absent(foo{job="xxx"})
annotations:
summary: '{{ .Labels.job }} in cluster {{$labels.cluster}}/{{ $labels.env }} is missing'
`,
checker: newTemplateCheck,
problems: noProblems,
},
{
description: "bar * on() group_right() absent()",
content: `
- alert: Foo
expr: bar * on() group_right() absent(foo{job="xxx"})
annotations:
summary: '{{ .Labels.job }} in cluster {{$labels.cluster}}/{{ $labels.env }} is missing'
`,
checker: newTemplateCheck,
problems: noProblems,
},
}
runTests(t, testCases)
}
62 changes: 31 additions & 31 deletions internal/parser/utils/absent.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,51 +7,51 @@ import (
"github.com/rs/zerolog/log"
)

func HasOuterAbsent(node *parser.PromQLNode) (calls []*parser.PromQLNode) {
type PromQLFragment struct {
Fragment *parser.PromQLNode
BinExpr *promParser.BinaryExpr
}

func HasOuterAbsent(node *parser.PromQLNode) (calls []PromQLFragment) {
if n, ok := node.Node.(*promParser.Call); ok && n.Func.Name == "absent" {
calls = append(calls, node)
calls = append(calls, PromQLFragment{Fragment: node})
return calls
}

if n, ok := node.Node.(*promParser.BinaryExpr); ok {
if n.VectorMatching != nil {
switch n.VectorMatching.Card {
// bar / absent(foo)
// absent(foo) / bar
case promParser.CardOneToOne:
case promParser.CardOneToMany:
for i, child := range node.Children {
if i == len(node.Children)-1 {
return HasOuterAbsent(child)
}
// CardManyToOne:
// absent(foo{job="bar"}) * on(job) group_left(xxx) bar
// bar * on() group_left(xxx) absent(foo{job="bar"})
// CardOneToMany
// bar * on() group_right(xxx) absent(foo{job="bar"})
// absent(foo{job="bar"}) * on(job) group_right(xxx) bar
case promParser.CardManyToOne, promParser.CardOneToMany:
if ln, ok := n.LHS.(*promParser.Call); ok && ln.Func.Name == "absent" {
calls = append(calls, PromQLFragment{
Fragment: node.Children[0],
BinExpr: n,
})
}
case promParser.CardManyToOne:
return HasOuterAbsent(node.Children[0])
case promParser.CardManyToMany:
default:
log.Warn().Str("matching", n.VectorMatching.Card.String()).Msg("Unsupported VectorMatching operation")
}
}

if n.Op.IsComparisonOperator() {
for i, child := range node.Children {
if n.VectorMatching != nil {
return HasOuterAbsent(child)
if rn, ok := n.RHS.(*promParser.Call); ok && rn.Func.Name == "absent" {
calls = append(calls, PromQLFragment{
Fragment: node.Children[1],
BinExpr: n,
})
}
if i == 0 {
return HasOuterAbsent(child)
}
}
} else {
switch n.Op {
case promParser.LOR:
// bar AND absent(foo{job="bar"})
case promParser.CardManyToMany:
for _, child := range node.Children {
calls = append(calls, HasOuterAbsent(child)...)
}
return calls
case promParser.DIV, promParser.LUNLESS, promParser.LAND:
for _, child := range node.Children {
return HasOuterAbsent(child)
}
default:
log.Warn().Str("matching", n.VectorMatching.Card.String()).Msg("Unsupported VectorMatching operation")
}
return
}
}

Expand Down
94 changes: 79 additions & 15 deletions internal/parser/utils/absent_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,14 @@ import (
)

func TestHasOuterAbsent(t *testing.T) {
type callT struct {
call string
binExpr string
}

type testCaseT struct {
expr string
output []string
output []callT
}

testCases := []testCaseT{
Expand All @@ -21,31 +26,83 @@ func TestHasOuterAbsent(t *testing.T) {
},
{
expr: "absent(foo)",
output: []string{"absent(foo)"},
output: []callT{{call: "absent(foo)"}},
},
{
expr: `absent(foo{job="bar"})`,
output: []string{`absent(foo{job="bar"})`},
output: []callT{{call: `absent(foo{job="bar"})`}},
},
{
expr: `absent(foo{job="bar"}) AND on(job) bar`,
output: []callT{{
call: `absent(foo{job="bar"})`,
}},
},
{
expr: `vector(1) or absent(foo{job="bar"}) AND on(job) bar`,
output: []callT{
{call: `absent(foo{job="bar"})`},
},
},
{
expr: `up == 0 or absent(foo{job="bar"}) AND on(job) bar`,
output: []callT{{
call: `absent(foo{job="bar"})`,
}},
},
{
expr: `up == 0 or absent(foo{job="bar"}) or absent(bar)`,
output: []callT{
{call: `absent(foo{job="bar"})`},
{call: `absent(bar)`},
},
},
{
expr: `absent(sum(nonexistent{job="myjob"}))`,
output: []callT{
{call: `absent(sum(nonexistent{job="myjob"}))`},
},
},
{
expr: `up == 0 or absent(foo{job="bar"}) * on(job) group_left(xxx) bar`,
output: []callT{{
call: `absent(foo{job="bar"})`,
binExpr: `absent(foo{job="bar"}) * on(job) group_left(xxx) bar`,
}},
},
{
expr: `absent(foo{job="bar"}) AND on(job) bar`,
output: []string{`absent(foo{job="bar"})`},
expr: `bar * on() group_left(xxx) absent(foo{job="bar"})`,
output: []callT{{
call: `absent(foo{job="bar"})`,
binExpr: `bar * on() group_left(xxx) absent(foo{job="bar"})`,
}},
},
{
expr: `vector(1) or absent(foo{job="bar"}) AND on(job) bar`,
output: []string{`absent(foo{job="bar"})`},
expr: `up == 0 or absent(foo{job="bar"}) * on(job) group_left() bar`,
output: []callT{{
call: `absent(foo{job="bar"})`,
binExpr: `absent(foo{job="bar"}) * on(job) group_left() bar`,
}},
},
{
expr: `up == 0 or absent(foo{job="bar"}) AND on(job) bar`,
output: []string{`absent(foo{job="bar"})`},
expr: `bar * on() group_right(xxx) absent(foo{job="bar"})`,
output: []callT{{
call: `absent(foo{job="bar"})`,
binExpr: `bar * on() group_right(xxx) absent(foo{job="bar"})`,
}},
},
{
expr: `up == 0 or absent(foo{job="bar"}) or absent(bar)`,
output: []string{`absent(foo{job="bar"})`, `absent(bar)`},
expr: `absent(foo{job="bar"}) * on(job) group_right(xxx) bar`,
output: []callT{{
call: `absent(foo{job="bar"})`,
binExpr: `absent(foo{job="bar"}) * on(job) group_right(xxx) bar`,
}},
},
{
expr: `absent(sum(nonexistent{job="myjob"}))`,
output: []string{`absent(sum(nonexistent{job="myjob"}))`},
expr: `absent(foo{job="bar"}) OR bar`,
output: []callT{{
call: `absent(foo{job="bar"})`,
}},
},
}

Expand All @@ -62,9 +119,16 @@ func TestHasOuterAbsent(t *testing.T) {
t.Errorf("HasOuterAbsent() returned nil, expected %s", tc.output)
}
} else {
output := []string{}
output := []callT{}
for _, a := range calls {
output = append(output, a.Node.String())
var c callT
if a.Fragment != nil {
c.call = a.Fragment.Node.String()
}
if a.BinExpr != nil {
c.binExpr = a.BinExpr.String()
}
output = append(output, c)
}
require.Equal(t, tc.output, output, "HasOuterAbsent() returned wrong output")
}
Expand Down

0 comments on commit 597f07d

Please sign in to comment.