Skip to content

Commit

Permalink
Check alerts in rule/dependency
Browse files Browse the repository at this point in the history
  • Loading branch information
@prymitive
prymitive committed Feb 29, 2024
1 parent c6d2671 commit 32588e6
Show file tree
Hide file tree
Showing 3 changed files with 144 additions and 30 deletions.
6 changes: 6 additions & 0 deletions docs/changelog.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@

## v0.54.0

### Added

- [rule/dependency](checks/rule/dependency.md) check will now warn if an alerting rule
that's being removed in a pull request is being used inside `ALERTS{alertname="..."}`
or `ALERTS_FOR_STATE{alertname="..."}` queries.

### Changed

- A large part of rule parsing code was refactored and more problems will now be deduplicated.
Expand Down
83 changes: 59 additions & 24 deletions internal/checks/rule_dependency.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import (
"github.com/cloudflare/pint/internal/discovery"
"github.com/cloudflare/pint/internal/parser"
"github.com/cloudflare/pint/internal/parser/utils"

"github.com/prometheus/prometheus/model/labels"
)

const (
Expand Down Expand Up @@ -39,12 +41,9 @@ func (c RuleDependencyCheck) Reporter() string {
return RuleDependencyCheckName
}

func (c RuleDependencyCheck) Check(_ context.Context, path string, rule parser.Rule, entries []discovery.Entry) (problems []Problem) {
if rule.RecordingRule == nil {
return problems
}

var broken []brokenDependency
func (c RuleDependencyCheck) Check(_ context.Context, _ string, rule parser.Rule, entries []discovery.Entry) (problems []Problem) {
var broken []*brokenDependency
var dep *brokenDependency
for _, entry := range entries {
if entry.State == discovery.Removed {
continue
Expand All @@ -55,16 +54,16 @@ func (c RuleDependencyCheck) Check(_ context.Context, path string, rule parser.R
if entry.Rule.Error.Err != nil {
continue
}
if c.usesVector(entry, rule.RecordingRule.Record.Value) {
expr := entry.Rule.Expr()
dep := brokenDependency{
path: entry.ReportedPath,
line: expr.Value.Lines.First,
name: entry.Rule.Name(),
}
if rule.RecordingRule != nil {
dep = c.usesVector(entry, rule.RecordingRule.Record.Value)
}
if rule.AlertingRule != nil {
dep = c.usesAlert(entry, rule.AlertingRule.Alert.Value)
}
if dep != nil {
var found bool
for _, b := range broken {
if b.path == dep.path && b.line == dep.line && b.name == dep.name {
if b.kind == dep.kind && b.path == dep.path && b.line == dep.line && b.name == dep.name {
found = true
break
}
Expand All @@ -90,11 +89,13 @@ func (c RuleDependencyCheck) Check(_ context.Context, path string, rule parser.R
})

var details strings.Builder
details.WriteString("If you remove the recording rule generating `")
details.WriteString(rule.RecordingRule.Record.Value)
details.WriteString("If you remove the ")
details.WriteString(dep.kind)
details.WriteString(" rule generating `")
details.WriteString(dep.metric)
details.WriteString("`, and there is no other source of this metric, then any other rule depending on it will break.\n")
details.WriteString("List of found rules that are using `")
details.WriteString(rule.RecordingRule.Record.Value)
details.WriteString(dep.metric)
details.WriteString("`:\n\n")
for _, b := range broken {
details.WriteString("- `")
Expand All @@ -118,23 +119,57 @@ func (c RuleDependencyCheck) Check(_ context.Context, path string, rule parser.R
return problems
}

func (c RuleDependencyCheck) usesVector(entry discovery.Entry, name string) bool {
func (c RuleDependencyCheck) usesVector(entry discovery.Entry, name string) *brokenDependency {
expr := entry.Rule.Expr()
if expr.SyntaxError != nil {
return false
return nil
}

for _, vs := range utils.HasVectorSelector(expr.Query) {
if vs.Name == name {
return true
return &brokenDependency{
kind: "recording",
metric: name,
path: entry.ReportedPath,
line: expr.Value.Lines.First,
name: entry.Rule.Name(),
}
}
}

return nil
}

func (c RuleDependencyCheck) usesAlert(entry discovery.Entry, name string) *brokenDependency {
expr := entry.Rule.Expr()
if expr.SyntaxError != nil {
return nil
}

for _, vs := range utils.HasVectorSelector(expr.Query) {
if vs.Name != "ALERTS" && vs.Name != "ALERTS_FOR_STATE" {
continue
}
for _, lm := range vs.LabelMatchers {
if lm.Name == "alertname" && lm.Type == labels.MatchEqual && lm.Value == name {
return &brokenDependency{
kind: "alerting",
metric: vs.String(),
path: entry.ReportedPath,
line: expr.Value.Lines.First,
name: entry.Rule.Name(),
}
}
}
}

return false
return nil
}

type brokenDependency struct {
path string
name string
line int
kind string
metric string
path string
name string
line int
}
85 changes: 79 additions & 6 deletions internal/checks/rule_dependency_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ func textDependencyRule(nr int) string {
return fmt.Sprintf("Metric generated by this rule is used by %d other rule(s).", nr)
}

func detailsDependencyRule(name, broken string) string {
func detailsDependencyRule(kind, name, broken string) string {
return fmt.Sprintf(
"If you remove the recording rule generating `%s`, and there is no other source of this metric, then any other rule depending on it will break.\nList of found rules that are using `%s`:\n\n%s",
name, name, broken,
"If you remove the %s rule generating `%s`, and there is no other source of this metric, then any other rule depending on it will break.\nList of found rules that are using `%s`:\n\n%s",
kind, name, name, broken,
)
}

Expand Down Expand Up @@ -111,7 +111,7 @@ func TestRuleDependencyCheck(t *testing.T) {
},
Reporter: checks.RuleDependencyCheckName,
Text: textDependencyRule(1),
Details: detailsDependencyRule("foo", "- `alert` at `excluded.yaml:2`\n"),
Details: detailsDependencyRule("recording", "foo", "- `alert` at `excluded.yaml:2`\n"),
Severity: checks.Warning,
},
}
Expand All @@ -138,7 +138,7 @@ func TestRuleDependencyCheck(t *testing.T) {
},
Reporter: checks.RuleDependencyCheckName,
Text: textDependencyRule(1),
Details: detailsDependencyRule("foo", "- `alert` at `foo.yaml:2`\n"),
Details: detailsDependencyRule("recording", "foo", "- `alert` at `foo.yaml:2`\n"),
Severity: checks.Warning,
},
}
Expand Down Expand Up @@ -195,7 +195,7 @@ func TestRuleDependencyCheck(t *testing.T) {
},
Reporter: checks.RuleDependencyCheckName,
Text: textDependencyRule(5),
Details: detailsDependencyRule("foo", "- `alert` at `alice.yaml:4`\n- `alert` at `alice.yaml:6`\n- `alert` at `bar.yaml:2`\n- `xxx` at `bar.yaml:2`\n- `alert` at `foo.yaml:2`\n"),
Details: detailsDependencyRule("recording", "foo", "- `alert` at `alice.yaml:4`\n- `alert` at `alice.yaml:6`\n- `alert` at `bar.yaml:2`\n- `xxx` at `bar.yaml:2`\n- `alert` at `foo.yaml:2`\n"),
Severity: checks.Warning,
},
}
Expand All @@ -212,6 +212,79 @@ func TestRuleDependencyCheck(t *testing.T) {
parseWithState("- alert: alert\n expr: foo == 0\n", discovery.Noop, "symlink2.yaml", "foo.yaml")[0],
},
},
{
description: "warns about removed alert used in ALERTS{}",
content: "- alert: TargetIsDown\n expr: up == 0\n",
checker: func(_ *promapi.FailoverGroup) checks.RuleChecker {
return checks.NewRuleDependencyCheck()
},
prometheus: newSimpleProm,
problems: func(_ string) []checks.Problem {
return []checks.Problem{
{
Anchor: checks.AnchorBefore,
Lines: parser.LineRange{
First: 1,
Last: 2,
},
Reporter: checks.RuleDependencyCheckName,
Text: textDependencyRule(1),
Details: detailsDependencyRule("alerting", `ALERTS{alertname="TargetIsDown"}`, "- `alert:count` at `foo.yaml:3`\n"),
Severity: checks.Warning,
},
}
},
entries: []discovery.Entry{
parseWithState(`
- record: alert:count
expr: count(ALERTS{alertname="TargetIsDown"})
`, discovery.Noop, "foo.yaml", "foo.yaml")[0],
},
},
{
description: "warns about removed alert used in ALERTS_FOR_STATE{}",
content: "- alert: TargetIsDown\n expr: up == 0\n",
checker: func(_ *promapi.FailoverGroup) checks.RuleChecker {
return checks.NewRuleDependencyCheck()
},
prometheus: newSimpleProm,
problems: func(_ string) []checks.Problem {
return []checks.Problem{
{
Anchor: checks.AnchorBefore,
Lines: parser.LineRange{
First: 1,
Last: 2,
},
Reporter: checks.RuleDependencyCheckName,
Text: textDependencyRule(1),
Details: detailsDependencyRule("alerting", `ALERTS_FOR_STATE{alertname="TargetIsDown"}`, "- `alert:count` at `foo.yaml:3`\n"),
Severity: checks.Warning,
},
}
},
entries: []discovery.Entry{
parseWithState(`
- record: alert:count
expr: count(ALERTS_FOR_STATE{alertname="TargetIsDown"})
`, discovery.Noop, "foo.yaml", "foo.yaml")[0],
},
},
{
description: "ignores removed unused alert",
content: "- alert: TargetIsDown\n expr: up == 0\n",
checker: func(_ *promapi.FailoverGroup) checks.RuleChecker {
return checks.NewRuleDependencyCheck()
},
prometheus: newSimpleProm,
problems: noProblems,
entries: []discovery.Entry{
parseWithState(`
- record: alert:count
expr: count(ALERTS{alertname!="TargetIsDown"})
`, discovery.Noop, "foo.yaml", "foo.yaml")[0],
},
},
}

runTests(t, testCases)
Expand Down

0 comments on commit 32588e6

Please sign in to comment.