diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e6eaeba..bff827bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,15 +1,34 @@ # Changelog -## v0.3.1 +## v0.4.0 -### Fixed +### Added -- Fixed `template` check panic when alert query had a syntax error. +- `comparison` check will now warn when alert query uses + [bool](https://prometheus.io/docs/prometheus/latest/querying/operators/#comparison-binary-operators) + modifier after condition, which can cause alert to always fire. + Example: + + ``` + - alert: Foo + expr: rate(error_count[5m]) > bool 5 + ``` + + Having `bool` as part of `> 5` condition means that the query will return value `1` when condition + is met, and `0` when it's not. Rather than returning value of `rate(error_count[5m])` only when + that value is `> 5`. Since all results of an alerting rule `expr` are considered alerts such alert + rule could always fire, regardless of the value returned by `rate(error_count[5m])`. + +## v0.3.1 ### Added - `--offline` flag for `pint ci` command. +### Fixed + +- Fixed `template` check panic when alert query had a syntax error. + ## v0.3.0 ### Added diff --git a/internal/checks/promql_comparison.go b/internal/checks/promql_comparison.go index a263a0ab..29c96b88 100644 --- a/internal/checks/promql_comparison.go +++ b/internal/checks/promql_comparison.go @@ -31,7 +31,16 @@ func (c ComparisonCheck) Check(rule parser.Rule) (problems []Problem) { return } - if hasComparision(rule.Expr().Query) { + if expr := hasComparision(rule.Expr().Query); expr != nil { + if expr.ReturnBool { + problems = append(problems, Problem{ + Fragment: rule.AlertingRule.Expr.Value.Value, + Lines: rule.AlertingRule.Expr.Lines(), + Reporter: ComparisonCheckName, + Text: "alert query uses bool modifier for comparison, this means it will always return a result and the alert will always fire", + Severity: c.severity, + }) + } return } @@ -46,21 +55,21 @@ func (c ComparisonCheck) Check(rule parser.Rule) (problems []Problem) { return } -func hasComparision(n *parser.PromQLNode) bool { +func hasComparision(n *parser.PromQLNode) *promParser.BinaryExpr { if node, ok := n.Node.(*promParser.BinaryExpr); ok { if node.Op.IsComparisonOperator() { - return true + return node } if node.Op == promParser.LUNLESS { - return true + return node } } for _, child := range n.Children { - if hasComparision(child) { - return true + if node := hasComparision(child); node != nil { + return node } } - return false + return nil } diff --git a/internal/checks/promql_comparison_test.go b/internal/checks/promql_comparison_test.go index b3edf95e..f1b2c717 100644 --- a/internal/checks/promql_comparison_test.go +++ b/internal/checks/promql_comparison_test.go @@ -30,7 +30,8 @@ func TestComparisonCheck(t *testing.T) { { description: "alert expr with == condition", content: "- alert: Foo Is Down\n for: 10m\n expr: up{job=\"foo\"} == 1\n", - checker: checks.NewComparisonCheck(checks.Bug)}, + checker: checks.NewComparisonCheck(checks.Bug), + }, { description: "alert expr without any condition", content: "- alert: Foo Is Down\n expr: up{job=\"foo\"}\n", @@ -79,6 +80,25 @@ func TestComparisonCheck(t *testing.T) { content: "- alert: Foo Is Down\n for: 10m\n expr: foo unless bar\n", checker: checks.NewComparisonCheck(checks.Bug), }, + { + description: "alert expr with bool", + content: "- alert: Error rate is high\n expr: rate(error_count[5m]) > bool 5\n", + checker: checks.NewComparisonCheck(checks.Warning), + problems: []checks.Problem{ + { + Fragment: "rate(error_count[5m]) > bool 5", + Lines: []int{2}, + Reporter: "promql/comparison", + Text: "alert query uses bool modifier for comparison, this means it will always return a result and the alert will always fire", + Severity: checks.Warning, + }, + }, + }, + { + description: "alert expr with bool and condition", + content: "- alert: Error rate is high\n expr: rate(error_count[5m]) > bool 5 == 1\n", + checker: checks.NewComparisonCheck(checks.Warning), + }, } runTests(t, testCases)