Skip to content

Commit

Permalink
Fix check test logic
Browse files Browse the repository at this point in the history
  • Loading branch information
@prymitive
prymitive committed Jan 17, 2022
1 parent 361dbf7 commit 65a632a
Show file tree
Hide file tree
Showing 3 changed files with 166 additions and 30 deletions.
23 changes: 13 additions & 10 deletions internal/config/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,19 +71,19 @@ func (cfg Config) String() string {
func (cfg *Config) GetChecksForRule(path string, r parser.Rule) []checks.RuleChecker {
enabled := []checks.RuleChecker{}

if isEnabled(cfg.Checks.Enabled, cfg.Checks.Disabled, checks.SyntaxCheckName, r) {
if isEnabled(cfg.Checks.Enabled, cfg.Checks.Disabled, checks.SyntaxCheckName, r, nil) {
enabled = append(enabled, checks.NewSyntaxCheck())
}

if isEnabled(cfg.Checks.Enabled, cfg.Checks.Disabled, checks.AlertForCheckName, r) {
if isEnabled(cfg.Checks.Enabled, cfg.Checks.Disabled, checks.AlertForCheckName, r, nil) {
enabled = append(enabled, checks.NewAlertsForCheck())
}

if isEnabled(cfg.Checks.Enabled, cfg.Checks.Disabled, checks.ComparisonCheckName, r) {
if isEnabled(cfg.Checks.Enabled, cfg.Checks.Disabled, checks.ComparisonCheckName, r, nil) {
enabled = append(enabled, checks.NewComparisonCheck())
}

if isEnabled(cfg.Checks.Enabled, cfg.Checks.Disabled, checks.TemplateCheckName, r) {
if isEnabled(cfg.Checks.Enabled, cfg.Checks.Disabled, checks.TemplateCheckName, r, nil) {
enabled = append(enabled, checks.NewTemplateCheck())
}

Expand All @@ -95,30 +95,33 @@ func (cfg *Config) GetChecksForRule(path string, r parser.Rule) []checks.RuleChe
for _, p := range cfg.prometheusServers {
if p.Name() == prom.Name {
proms = append(proms, p)
break
}
}
}

if isEnabled(cfg.Checks.Enabled, cfg.Checks.Disabled, checks.RateCheckName, r) {
for _, prom := range proms {
for _, prom := range proms {
if isEnabled(cfg.Checks.Enabled, cfg.Checks.Disabled, checks.RateCheckName, r, prom) {
enabled = append(enabled, checks.NewRateCheck(prom))
}
}

if isEnabled(cfg.Checks.Enabled, cfg.Checks.Disabled, checks.SeriesCheckName, r) {
for _, prom := range proms {
for _, prom := range proms {
if isEnabled(cfg.Checks.Enabled, cfg.Checks.Disabled, checks.SeriesCheckName, r, prom) {

enabled = append(enabled, checks.NewSeriesCheck(prom))
}
}

if isEnabled(cfg.Checks.Enabled, cfg.Checks.Disabled, checks.VectorMatchingCheckName, r) {
for _, prom := range proms {
for _, prom := range proms {
if isEnabled(cfg.Checks.Enabled, cfg.Checks.Disabled, checks.VectorMatchingCheckName, r, prom) {
enabled = append(enabled, checks.NewVectorMatchingCheck(prom))
}
}

for _, rule := range cfg.Rules {
for _, c := range rule.resolveChecks(path, r, cfg.Checks.Enabled, cfg.Checks.Disabled, proms) {
// FIXME redundant ?
if r.HasComment(fmt.Sprintf("disable %s", removeRedundantSpaces(c.String()))) {
log.Debug().
Str("path", path).
Expand Down
138 changes: 131 additions & 7 deletions internal/config/config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ func TestSetDisabledChecks(t *testing.T) {
cfg.SetDisabledChecks([]string{checks.SyntaxCheckName})
cfg.SetDisabledChecks([]string{checks.SyntaxCheckName})
cfg.SetDisabledChecks([]string{checks.RateCheckName})
assert.Equal(cfg.Checks.Disabled, []string{checks.SyntaxCheckName, checks.RateCheckName})
assert.Equal([]string{checks.SyntaxCheckName, checks.RateCheckName}, cfg.Checks.Disabled)
}

func newRule(t *testing.T, content string) parser.Rule {
Expand All @@ -111,8 +111,6 @@ func newRule(t *testing.T, content string) parser.Rule {
}

func TestGetChecksForRule(t *testing.T) {
assert := assert.New(t)

type testCaseT struct {
title string
config string
Expand Down Expand Up @@ -219,24 +217,150 @@ prometheus "ignore" {
checks.VectorMatchingCheckName + "(prom)",
},
},
{
title: "single empty rule",
config: "rule{}\n",
path: "rules.yml",
rule: newRule(t, "- record: foo\n expr: sum(foo)\n"),
checks: []string{
checks.SyntaxCheckName,
checks.AlertForCheckName,
checks.ComparisonCheckName,
checks.TemplateCheckName,
},
},
{
title: "rule with aggregate checks",
config: `
rule {
aggregate ".+" {
severity = "bug"
keep = ["job"]
}
aggregate ".+" {
severity = "bug"
strip = ["instance", "rack"]
}
}`,
path: "rules.yml",
rule: newRule(t, "- record: foo\n expr: sum(foo)\n"),
checks: []string{
checks.SyntaxCheckName,
checks.AlertForCheckName,
checks.ComparisonCheckName,
checks.TemplateCheckName,
checks.AggregationCheckName + "(job:true)",
checks.AggregationCheckName + "(instance:false)",
checks.AggregationCheckName + "(rack:false)",
},
},
{
title: "multiple checks and disable comment",
config: `
rule {
aggregate ".+" {
severity = "bug"
keep = ["job"]
}
aggregate ".+" {
severity = "bug"
strip = ["instance", "rack"]
}
}`,
path: "rules.yml",
rule: newRule(t, `
- record: foo
# pint disable promql/aggregate(instance:false)
expr: sum(foo)
`),
checks: []string{
checks.SyntaxCheckName,
checks.AlertForCheckName,
checks.ComparisonCheckName,
checks.TemplateCheckName,
checks.AggregationCheckName + "(job:true)",
checks.AggregationCheckName + "(rack:false)",
},
},
{
title: "prometheus check without prometheus server",
config: `
rule {
cost {
bytesPerSample = 4096
}
}
`,
path: "rules.yml",
rule: newRule(t, "- record: foo\n expr: sum(foo)\n"),
checks: []string{
checks.SyntaxCheckName,
checks.AlertForCheckName,
checks.ComparisonCheckName,
checks.TemplateCheckName,
},
},
{
title: "prometheus check with prometheus servers and disable comment",
config: `
rule {
cost {
bytesPerSample = 4096
}
}
prometheus "prom1" {
uri = "http://localhost"
timeout = "1s"
paths = [ "rules.yml" ]
}
prometheus "prom2" {
uri = "http://localhost"
timeout = "1s"
paths = [ "rules.yml" ]
}
`,
path: "rules.yml",
rule: newRule(t, `
# pint disable query/series(prom1)
# pint disable query/cost(prom2)
- record: foo
# pint disable promql/rate(prom2)
# pint disable promql/vector_matching(prom1)
expr: sum(foo)
`),
checks: []string{
checks.SyntaxCheckName,
checks.AlertForCheckName,
checks.ComparisonCheckName,
checks.TemplateCheckName,
checks.RateCheckName + "(prom1)",
checks.SeriesCheckName + "(prom2)",
checks.VectorMatchingCheckName + "(prom2)",
checks.CostCheckName + "(prom1)",
},
},
}

dir := t.TempDir()
for i, tc := range testCases {
t.Run(tc.title, func(t *testing.T) {
assert := assert.New(t)

path := path.Join(dir, fmt.Sprintf("%d.hcl", i))
err := ioutil.WriteFile(path, []byte(tc.config), 0644)
assert.NoError(err)
if tc.config != "" {
err := ioutil.WriteFile(path, []byte(tc.config), 0644)
assert.NoError(err)
}

cfg, err := config.Load(path, true)
cfg, err := config.Load(path, false)
assert.NoError(err)

checks := cfg.GetChecksForRule(tc.path, tc.rule)
checkNames := make([]string, 0, len(checks))
for _, c := range checks {
checkNames = append(checkNames, c.String())
}
assert.Equal(checkNames, tc.checks)
assert.Equal(tc.checks, checkNames)
})
}
}
35 changes: 22 additions & 13 deletions internal/config/rule.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,26 +175,26 @@ func (rule Rule) resolveChecks(path string, r parser.Rule, enabledChecks, disabl
}
severity := aggr.getSeverity(checks.Warning)
for _, label := range aggr.Keep {
if isEnabled(enabledChecks, disabledChecks, checks.AggregationCheckName, r) {
if isEnabled(enabledChecks, disabledChecks, checks.AggregationCheckName, r, nil) {
enabled = append(enabled, checks.NewAggregationCheck(nameRegex, label, true, severity))
}
}
for _, label := range aggr.Strip {
if isEnabled(enabledChecks, disabledChecks, checks.AggregationCheckName, r) {
if isEnabled(enabledChecks, disabledChecks, checks.AggregationCheckName, r, nil) {
enabled = append(enabled, checks.NewAggregationCheck(nameRegex, label, false, severity))
}
}
}
}

if rule.Cost != nil && isEnabled(enabledChecks, disabledChecks, checks.CostCheckName, r) {
if rule.Cost != nil && isEnabled(enabledChecks, disabledChecks, checks.CostCheckName, r, nil) {
severity := rule.Cost.getSeverity(checks.Bug)
for _, prom := range prometheusServers {
enabled = append(enabled, checks.NewCostCheck(prom, rule.Cost.BytesPerSample, rule.Cost.MaxSeries, severity))
}
}

if len(rule.Annotation) > 0 && isEnabled(enabledChecks, disabledChecks, checks.AnnotationCheckName, r) {
if len(rule.Annotation) > 0 && isEnabled(enabledChecks, disabledChecks, checks.AnnotationCheckName, r, nil) {
for _, ann := range rule.Annotation {
var valueRegex *regexp.Regexp
if ann.Value != "" {
Expand All @@ -204,7 +204,7 @@ func (rule Rule) resolveChecks(path string, r parser.Rule, enabledChecks, disabl
enabled = append(enabled, checks.NewAnnotationCheck(ann.Key, valueRegex, ann.Required, severity))
}
}
if len(rule.Label) > 0 && isEnabled(enabledChecks, disabledChecks, checks.LabelCheckName, r) {
if len(rule.Label) > 0 && isEnabled(enabledChecks, disabledChecks, checks.LabelCheckName, r, nil) {
for _, lab := range rule.Label {
var valueRegex *regexp.Regexp
if lab.Value != "" {
Expand All @@ -215,7 +215,7 @@ func (rule Rule) resolveChecks(path string, r parser.Rule, enabledChecks, disabl
}
}

if rule.Alerts != nil && isEnabled(enabledChecks, disabledChecks, checks.AlertsCheckName, r) {
if rule.Alerts != nil && isEnabled(enabledChecks, disabledChecks, checks.AlertsCheckName, r, nil) {
qRange := time.Hour * 24
if rule.Alerts.Range != "" {
qRange, _ = parseDuration(rule.Alerts.Range)
Expand All @@ -233,7 +233,7 @@ func (rule Rule) resolveChecks(path string, r parser.Rule, enabledChecks, disabl
}
}

if len(rule.Reject) > 0 && isEnabled(enabledChecks, disabledChecks, checks.RejectCheckName, r) {
if len(rule.Reject) > 0 && isEnabled(enabledChecks, disabledChecks, checks.RejectCheckName, r, nil) {
for _, reject := range rule.Reject {
severity := reject.getSeverity(checks.Bug)
if reject.LabelKeys {
Expand All @@ -258,12 +258,21 @@ func (rule Rule) resolveChecks(path string, r parser.Rule, enabledChecks, disabl
return enabled
}

func isEnabled(enabledChecks, disabledChecks []string, name string, rule parser.Rule) bool {
if rule.HasComment(fmt.Sprintf("disable %s", removeRedundantSpaces(name))) {
log.Debug().
Str("check", name).
Msg("Check disabled by comment")
return false
func isEnabled(enabledChecks, disabledChecks []string, name string, rule parser.Rule, prom *promapi.Prometheus) bool {
comments := []string{
fmt.Sprintf("disable %s", removeRedundantSpaces(name)),
}
if prom != nil {
comments = append(comments, fmt.Sprintf("disable %s(%s)", removeRedundantSpaces(name), prom.Name()))
}
for _, comment := range comments {
if rule.HasComment(comment) {
log.Debug().
Str("check", name).
Str("comment", comment).
Msg("Check disabled by comment")
return false
}
}

for _, c := range disabledChecks {
Expand Down

0 comments on commit 65a632a

Please sign in to comment.