Skip to content

Commit

Permalink
Merge pull request cloudflare#78 from cloudflare/improve-labels
Browse files Browse the repository at this point in the history
Improve template label checks
  • Loading branch information
prymitive authored Nov 25, 2021
2 parents 8b79532 + b92428f commit cca7a93
Show file tree
Hide file tree
Showing 4 changed files with 102 additions and 23 deletions.
6 changes: 3 additions & 3 deletions internal/checks/alerts_template.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ func (c TemplateCheck) Check(rule parser.Rule) (problems []Problem) {
return nil
}

aggr := utils.HasOuterAggregation(rule.AlertingRule.Expr.Query)
aggrs := utils.HasOuterAggregation(rule.AlertingRule.Expr.Query)

data := promTemplate.AlertTemplateData(map[string]string{}, map[string]string{}, "", 0)

Expand Down Expand Up @@ -111,7 +111,7 @@ func (c TemplateCheck) Check(rule parser.Rule) (problems []Problem) {
})
}

if aggr != nil {
for _, aggr := range aggrs {
for _, msg := range checkMetricLabels(label.Key.Value, label.Value.Value, aggr.Grouping, aggr.Without) {
problems = append(problems, Problem{
Fragment: fmt.Sprintf("%s: %s", label.Key.Value, label.Value.Value),
Expand All @@ -137,7 +137,7 @@ func (c TemplateCheck) Check(rule parser.Rule) (problems []Problem) {
})
}

if aggr != nil {
for _, aggr := range aggrs {
for _, msg := range checkMetricLabels(annotation.Key.Value, annotation.Value.Value, aggr.Grouping, aggr.Without) {
problems = append(problems, Problem{
Fragment: fmt.Sprintf("%s: %s", annotation.Key.Value, annotation.Value.Value),
Expand Down
28 changes: 28 additions & 0 deletions internal/checks/alerts_template_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,34 @@ func TestTemplateCheck(t *testing.T) {
},
},
},
{
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",
checker: checks.NewTemplateCheck(checks.Bug),
problems: []checks.Problem{
{
Fragment: `summary: {{ .Labels.job }}`,
Lines: []int{4},
Reporter: "alerts/template",
Text: `template is using "job" label but the query doesn't preseve it`,
Severity: checks.Bug,
},
},
},
{
description: "annotation label missing from metrics (1+)",
content: "- alert: Foo Is Down\n expr: 1 + sum(foo) by(job) + sum(foo) by(notjob)\n annotations:\n summary: '{{ .Labels.job }}'\n",
checker: checks.NewTemplateCheck(checks.Bug),
problems: []checks.Problem{
{
Fragment: `summary: {{ .Labels.job }}`,
Lines: []int{4},
Reporter: "alerts/template",
Text: `template is using "job" label but the query doesn't preseve it`,
Severity: checks.Bug,
},
},
},
}
runTests(t, testCases)
}
31 changes: 23 additions & 8 deletions internal/parser/utils/aggregation.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import (
"github.com/rs/zerolog/log"
)

func HasOuterAggregation(node *parser.PromQLNode) *promParser.AggregateExpr {
func HasOuterAggregation(node *parser.PromQLNode) (aggs []*promParser.AggregateExpr) {
if n, ok := node.Node.(*promParser.AggregateExpr); ok {
switch n.Op {
case promParser.SUM:
Expand All @@ -27,19 +27,34 @@ func HasOuterAggregation(node *parser.PromQLNode) *promParser.AggregateExpr {
default:
log.Warn().Str("op", n.Op.String()).Msg("Unsupported aggregation operation")
}
return n
aggs = append(aggs, n)
return aggs
}

NEXT:
if n, ok := node.Node.(*promParser.BinaryExpr); ok && n.VectorMatching != nil {
return HasOuterAggregation(node.Children[0])
if n, ok := node.Node.(*promParser.BinaryExpr); ok {
if n.Op.IsComparisonOperator() {
for _, child := range node.Children {
return HasOuterAggregation(child)
}
} else {
switch n.Op {
case promParser.LOR:
for _, child := range node.Children {
aggs = append(aggs, HasOuterAggregation(child)...)
}
return aggs
case promParser.DIV, promParser.LUNLESS, promParser.LAND:
for _, child := range node.Children {
return HasOuterAggregation(child)
}
}
}
}

for _, child := range node.Children {
if a := HasOuterAggregation(child); a != nil {
return a
}
aggs = append(aggs, HasOuterAggregation(child)...)
}

return nil
return aggs
}
60 changes: 48 additions & 12 deletions internal/parser/utils/aggregation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
func TestHasOuterAggregation(t *testing.T) {
type testCaseT struct {
expr string
output string
output []string
}

testCases := []testCaseT{
Expand All @@ -20,30 +20,62 @@ func TestHasOuterAggregation(t *testing.T) {
},
{
expr: "sum(foo)",
output: "sum(foo)",
output: []string{"sum(foo)"},
},
{
expr: "sum(foo) by(job)",
output: "sum by(job) (foo)",
output: []string{"sum by(job) (foo)"},
},
{
expr: "sum(foo) without(job)",
output: "sum without(job) (foo)",
output: []string{"sum without(job) (foo)"},
},
{
expr: "1 + sum(foo)",
output: "sum(foo)",
output: []string{"sum(foo)"},
},
{
expr: "vector(0) or sum(foo)",
output: []string{"sum(foo)"},
},
{
expr: "sum(foo) or vector(0)",
output: []string{"sum(foo)"},
},
{
expr: "sum(foo) + sum(bar)",
output: "sum(foo)",
output: []string{"sum(foo)", "sum(bar)"},
},
{
expr: "foo / on(bbb) sum(bar)",
},
{
expr: "sum(foo) / on(bbb) sum(bar)",
output: "sum(foo)",
output: []string{"sum(foo)"},
},
{
expr: "sum(foo) OR sum(bar) by(job)",
output: []string{"sum(foo)", "sum by(job) (bar)"},
},
{
expr: "foo OR sum(foo) OR sum(bar) by(job)",
output: []string{"sum(foo)", "sum by(job) (bar)"},
},
{
expr: "1 + sum(foo) by(job) + sum(foo) by(notjob)",
output: []string{"sum by(job) (foo)", "sum by(notjob) (foo)"},
},
{
expr: "sum(foo) by (job) > count(bar)",
output: []string{"sum by(job) (foo)"},
},
{
expr: "sum(foo) by (job) > count(foo) / 2 or sum(bar) by (job) > count(bar)",
output: []string{"sum by(job) (foo)", "sum by(job) (bar)"},
},
{
expr: "(foo unless on(instance, version, package) bar) and on(instance) (sum(enabled) by(instance) > 0)",
output: []string{},
},
}

Expand All @@ -54,13 +86,17 @@ func TestHasOuterAggregation(t *testing.T) {
t.Error(err)
t.FailNow()
}
output := utils.HasOuterAggregation(n)
if output == nil {
if tc.output != "" {
t.Errorf("HasOuterAggregation() returned nil, expected %q", tc.output)
aggs := utils.HasOuterAggregation(n)
if len(aggs) == 0 {
if len(tc.output) > 0 {
t.Errorf("HasOuterAggregation() returned nil, expected %s", tc.output)
}
} else {
if diff := cmp.Diff(tc.output, output.String()); diff != "" {
var output = []string{}
for _, a := range aggs {
output = append(output, a.String())
}
if diff := cmp.Diff(tc.output, output); diff != "" {
t.Errorf("HasOuterAggregation() returned wrong result (-want +got):\n%s", diff)
return
}
Expand Down

0 comments on commit cca7a93

Please sign in to comment.