diff --git a/internal/config/config.go b/internal/config/config.go index f47983bd..82a7a895 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -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()) } @@ -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). diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 2ff573dd..087f5736 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -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 { @@ -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 @@ -219,16 +217,142 @@ 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) @@ -236,7 +360,7 @@ prometheus "ignore" { for _, c := range checks { checkNames = append(checkNames, c.String()) } - assert.Equal(checkNames, tc.checks) + assert.Equal(tc.checks, checkNames) }) } } diff --git a/internal/config/rule.go b/internal/config/rule.go index 7b4d5a5d..5398a0b9 100644 --- a/internal/config/rule.go +++ b/internal/config/rule.go @@ -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 != "" { @@ -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 != "" { @@ -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) @@ -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 { @@ -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 {