Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add dead code detection #1182

Merged
merged 1 commit into from
Oct 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/changelog.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Changelog

## v0.67.2

### Fixed

- Improved the accuracy of [alerts/template](checks/alerts/template.md) check.

## v0.67.1

### Fixed
Expand Down
3 changes: 3 additions & 0 deletions internal/checks/alerts_template.go
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,9 @@ func checkQueryLabels(query, labelName, labelValue string, src []utils.Source) (
continue
}
for _, s := range src {
if s.IsDead {
continue
}
if s.FixedLabels && !slices.Contains(s.IncludedLabels, v[1]) {
problems = append(problems, textForProblem(query, v[1], "", s, Bug))
goto NEXT
Expand Down
27 changes: 2 additions & 25 deletions internal/checks/alerts_template_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1439,7 +1439,7 @@ func TestTemplateCheck(t *testing.T) {
},
},
{
description: "",
description: "ignore dead code",
content: `
- alert: Foo
expr: sum by (region, target, colo_name) (sum_over_time(probe_success{job="abc"}[5m]) or vector(1)) == 0
Expand All @@ -1449,30 +1449,7 @@ func TestTemplateCheck(t *testing.T) {
`,
checker: newTemplateCheck,
prometheus: noProm,
problems: func(_ string) []checks.Problem {
return []checks.Problem{
{
Lines: parser.LineRange{
First: 6,
Last: 6,
},
Reporter: checks.TemplateCheckName,
Text: "Template is using `region` label but the query results won't have this label.",
Details: "Calling `vector()` will return a vector value with no labels.\nQuery fragment causing this problem: `vector(1)`.",
Severity: checks.Bug,
},
{
Lines: parser.LineRange{
First: 6,
Last: 6,
},
Reporter: checks.TemplateCheckName,
Text: "Template is using `target` label but the query results won't have this label.",
Details: "Calling `vector()` will return a vector value with no labels.\nQuery fragment causing this problem: `vector(1)`.",
Severity: checks.Bug,
},
}
},
problems: noProblems,
},
}
runTests(t, testCases)
Expand Down
106 changes: 89 additions & 17 deletions internal/parser/utils/source.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package utils

import (
"fmt"
"math"
"slices"
"strings"

Expand Down Expand Up @@ -32,11 +33,14 @@ type Source struct {
ExcludeReason map[string]ExcludedLabel // Reason why a label was excluded
Operation string
Returns promParser.ValueType
IncludedLabels []string // Labels that are included by filters, they will be present if exist on source series (by).
ExcludedLabels []string // Labels guaranteed to be excluded from the results (without).
GuaranteedLabels []string // Labels guaranteed to be present on the results (matchers).
ReturnedNumbers []float64 // If AlwaysReturns=true this is the number that's returned
IncludedLabels []string // Labels that are included by filters, they will be present if exist on source series (by).
ExcludedLabels []string // Labels guaranteed to be excluded from the results (without).
GuaranteedLabels []string // Labels guaranteed to be present on the results (matchers).
Type SourceType
FixedLabels bool // Labels are fixed and only allowed labels can be present.
IsDead bool // True if this source cannot be reached and is dead code.
AlwaysReturns bool // True if this source always returns results.
}

func LabelsSource(expr string, node promParser.Node) (src []Source) {
Expand Down Expand Up @@ -65,9 +69,11 @@ func walkNode(expr string, node promParser.Node) (src []Source) {
case *promParser.NumberLiteral:
s.Type = NumberSource
s.Returns = promParser.ValueTypeScalar
s.ReturnedNumbers = append(s.ReturnedNumbers, n.Val)
s.IncludedLabels = nil
s.GuaranteedLabels = nil
s.FixedLabels = true
s.AlwaysReturns = true
s.ExcludeReason = setInMap(
s.ExcludeReason,
"",
Expand All @@ -87,6 +93,7 @@ func walkNode(expr string, node promParser.Node) (src []Source) {
s.IncludedLabels = nil
s.GuaranteedLabels = nil
s.FixedLabels = true
s.AlwaysReturns = true
s.ExcludeReason = setInMap(
s.ExcludeReason,
"",
Expand Down Expand Up @@ -405,6 +412,7 @@ If you're hoping to get instance specific labels this way and alert when some ta
// Otherwise no change to labels.
if len(s.Call.Args) == 0 {
s.FixedLabels = true
s.AlwaysReturns = true
s.IncludedLabels = nil
s.GuaranteedLabels = nil
s.ExcludeReason = setInMap(
Expand Down Expand Up @@ -451,6 +459,7 @@ If you're hoping to get instance specific labels this way and alert when some ta
s.IncludedLabels = nil
s.GuaranteedLabels = nil
s.FixedLabels = true
s.AlwaysReturns = true
s.ExcludeReason = setInMap(
s.ExcludeReason,
"",
Expand All @@ -465,6 +474,7 @@ If you're hoping to get instance specific labels this way and alert when some ta
s.IncludedLabels = nil
s.GuaranteedLabels = nil
s.FixedLabels = true
s.AlwaysReturns = true
s.ExcludeReason = setInMap(
s.ExcludeReason,
"",
Expand All @@ -483,6 +493,7 @@ If you're hoping to get instance specific labels this way and alert when some ta
s.IncludedLabels = nil
s.GuaranteedLabels = nil
s.FixedLabels = true
s.AlwaysReturns = true
s.ExcludeReason = setInMap(
s.ExcludeReason,
"",
Expand All @@ -502,6 +513,8 @@ If you're hoping to get instance specific labels this way and alert when some ta
s.IncludedLabels = nil
s.GuaranteedLabels = nil
s.FixedLabels = true
s.AlwaysReturns = true
s.ReturnedNumbers = append(s.ReturnedNumbers, n.Args[0].(*promParser.NumberLiteral).Val)
s.ExcludeReason = setInMap(
s.ExcludeReason,
"",
Expand All @@ -522,25 +535,34 @@ If you're hoping to get instance specific labels this way and alert when some ta
func parseBinOps(expr string, n *promParser.BinaryExpr) (src []Source) {
var s Source
switch {

// foo{} + 1
// 1 + foo{}
// foo{} > 1
// 1 > foo{}
case n.VectorMatching == nil:
var ok bool
for _, s = range walkNode(expr, n.LHS) {
if s.Returns != promParser.ValueTypeScalar && s.Returns != promParser.ValueTypeString {
ok = true
src = append(src, s)
}
}
if !ok {
for _, rs := range walkNode(expr, n.RHS) {
if rs.Returns != promParser.ValueTypeScalar && rs.Returns != promParser.ValueTypeString {
ok = true
lhs := walkNode(expr, n.LHS)
rhs := walkNode(expr, n.RHS)
for _, ls := range lhs {
for _, rs := range rhs {
switch {
case ls.AlwaysReturns && rs.AlwaysReturns:
// Both sides always return something
for i, lv := range ls.ReturnedNumbers {
for _, rv := range rs.ReturnedNumbers {
ls.ReturnedNumbers[i], ls.IsDead = calculateStaticReturn(lv, rv, n.Op, ls.IsDead)
}
}
src = append(src, ls)
case ls.Returns == promParser.ValueTypeVector, ls.Returns == promParser.ValueTypeMatrix:
// Use labels from LHS
src = append(src, ls)
case rs.Returns == promParser.ValueTypeVector, rs.Returns == promParser.ValueTypeMatrix:
// Use labels from RHS
src = append(src, rs)
}
}
}
if !ok {
src = append(src, s)
}

// foo{} + bar{}
// foo{} + on(...) bar{}
Expand Down Expand Up @@ -646,6 +668,7 @@ func parseBinOps(expr string, n *promParser.BinaryExpr) (src []Source) {
// foo{} and on(...) bar{}
// foo{} and ignoring(...) bar{}
case n.VectorMatching.Card == promParser.CardManyToMany:
var lhsCanBeEmpty bool // true if any of the LHS query can produce empty results.
for _, s = range walkNode(expr, n.LHS) {
if n.VectorMatching.On {
s.IncludedLabels = appendToSlice(s.IncludedLabels, n.VectorMatching.MatchingLabels...)
Expand All @@ -656,16 +679,65 @@ func parseBinOps(expr string, n *promParser.BinaryExpr) (src []Source) {
if s.Operation == "" {
s.Operation = n.VectorMatching.Card.String()
}
if !s.AlwaysReturns {
lhsCanBeEmpty = true
}
src = append(src, s)
}
if n.Op == promParser.LOR {
for _, s = range walkNode(expr, n.RHS) {
if s.Operation == "" {
s.Operation = n.VectorMatching.Card.String()
}
// If LHS can NOT be empty then RHS is dead code.
if !lhsCanBeEmpty {
s.IsDead = true
}
src = append(src, s)
}
}
}
return src
}

func calculateStaticReturn(lv, rv float64, op promParser.ItemType, isDead bool) (float64, bool) {
switch op {
case promParser.EQLC:
if lv != rv {
return lv, true
}
case promParser.NEQ:
if lv == rv {
return lv, true
}
case promParser.LTE:
if lv > rv {
return lv, true
}
case promParser.LSS:
if lv >= rv {
return lv, true
}
case promParser.GTE:
if lv < rv {
return lv, true
}
case promParser.GTR:
if lv <= rv {
return lv, true
}
case promParser.ADD:
return lv + rv, isDead
case promParser.SUB:
return lv - rv, isDead
case promParser.MUL:
return lv * rv, isDead
case promParser.DIV:
return lv / rv, isDead
case promParser.MOD:
return math.Mod(lv, rv), isDead
case promParser.POW:
return math.Pow(lv, rv), isDead
}
return lv, isDead
}
Loading