From 8c3a9ab251938b73a27c586dbcddab7a45bad42a Mon Sep 17 00:00:00 2001 From: Lukasz Mierzwa Date: Fri, 23 Aug 2024 17:49:14 +0100 Subject: [PATCH] Move error handing out of scan workers --- cmd/pint/scan.go | 150 ++++--------------------- cmd/pint/tests/0184_ci_file_ignore.txt | 1 - docs/configuration.md | 5 +- internal/checks/base.go | 5 +- internal/checks/error.go | 119 ++++++++++++++++++++ internal/config/config.go | 21 +++- internal/config/config_test.go | 2 +- internal/config/match.go | 9 +- internal/config/rule.go | 4 + 9 files changed, 176 insertions(+), 140 deletions(-) create mode 100644 internal/checks/error.go diff --git a/cmd/pint/scan.go b/cmd/pint/scan.go index 466b436e..e7fed9d7 100644 --- a/cmd/pint/scan.go +++ b/cmd/pint/scan.go @@ -2,8 +2,6 @@ package main import ( "context" - "errors" - "fmt" "log/slog" "sync" "time" @@ -11,23 +9,12 @@ import ( "go.uber.org/atomic" "github.com/cloudflare/pint/internal/checks" - "github.com/cloudflare/pint/internal/comments" "github.com/cloudflare/pint/internal/config" "github.com/cloudflare/pint/internal/discovery" - "github.com/cloudflare/pint/internal/parser" "github.com/cloudflare/pint/internal/promapi" "github.com/cloudflare/pint/internal/reporter" ) -const ( - yamlParseReporter = "yaml/parse" - ignoreFileReporter = "ignore/file" - pintCommentReporter = "pint/comment" - - yamlDetails = `This Prometheus rule is not valid. -This usually means that it's missing some required fields.` -) - func checkRules(ctx context.Context, workers int, isOffline bool, gen *config.PrometheusGenerator, cfg config.Config, entries []discovery.Entry) (summary reporter.Summary, err error) { if isOffline { slog.Info("Offline mode, skipping Prometheus discovery") @@ -84,7 +71,7 @@ func checkRules(ctx context.Context, workers int, isOffline bool, gen *config.Pr continue case entry.Rule.Error.Err != nil && entry.State == discovery.Removed: continue - case entry.PathError == nil && entry.Rule.Error.Err == nil: + default: if entry.Rule.RecordingRule != nil { rulesParsedTotal.WithLabelValues(config.RecordingRuleType).Inc() slog.Debug("Found recording rule", @@ -101,9 +88,16 @@ func checkRules(ctx context.Context, workers int, isOffline bool, gen *config.Pr slog.String("lines", entry.Rule.Lines.String()), ) } + if entry.Rule.Error.Err != nil { + slog.Debug("Found invalid rule", + slog.String("path", entry.Path.Name), + slog.String("lines", entry.Rule.Lines.String()), + ) + rulesParsedTotal.WithLabelValues(config.InvalidRuleType).Inc() + } checkedEntriesCount.Inc() - checkList := cfg.GetChecksForRule(ctx, gen, entry) + checkList := cfg.GetChecksForEntry(ctx, gen, entry) for _, check := range checkList { checkIterationChecks.Inc() if check.Meta().IsOnline { @@ -113,15 +107,6 @@ func checkRules(ctx context.Context, workers int, isOffline bool, gen *config.Pr } jobs <- scanJob{entry: entry, allEntries: entries, check: check} } - default: - if entry.Rule.Error.Err != nil { - slog.Debug("Found invalid rule", - slog.String("path", entry.Path.Name), - slog.String("lines", entry.Rule.Lines.String()), - ) - rulesParsedTotal.WithLabelValues(config.InvalidRuleType).Inc() - } - jobs <- scanJob{entry: entry, allEntries: entries, check: nil} } } defer close(jobs) @@ -154,72 +139,19 @@ func scanWorker(ctx context.Context, jobs <-chan scanJob, results chan<- reporte case <-ctx.Done(): return default: - var commentErr comments.CommentError - var ignoreErr discovery.FileIgnoreError - switch { - case errors.As(job.entry.PathError, &ignoreErr): - results <- reporter.Report{ - Path: discovery.Path{ - Name: job.entry.Path.Name, - SymlinkTarget: job.entry.Path.SymlinkTarget, - }, - ModifiedLines: job.entry.ModifiedLines, - Problem: checks.Problem{ - Lines: parser.LineRange{ - First: ignoreErr.Line, - Last: ignoreErr.Line, - }, - Reporter: ignoreFileReporter, - Text: ignoreErr.Error(), - Severity: checks.Information, - }, - Owner: job.entry.Owner, - } - case errors.As(job.entry.PathError, &commentErr): - results <- reporter.Report{ - Path: discovery.Path{ - Name: job.entry.Path.Name, - SymlinkTarget: job.entry.Path.SymlinkTarget, - }, - ModifiedLines: job.entry.ModifiedLines, - Problem: checks.Problem{ - Lines: parser.LineRange{ - First: commentErr.Line, - Last: commentErr.Line, - }, - Reporter: pintCommentReporter, - Text: "This comment is not a valid pint control comment: " + commentErr.Error(), - Severity: checks.Warning, - }, - Owner: job.entry.Owner, - } - case job.entry.PathError != nil: - results <- reporter.Report{ - Path: discovery.Path{ - Name: job.entry.Path.Name, - SymlinkTarget: job.entry.Path.SymlinkTarget, - }, - ModifiedLines: job.entry.ModifiedLines, - Problem: checks.Problem{ - Lines: parser.LineRange{ - First: 1, - Last: 1, - }, - Reporter: yamlParseReporter, - Text: fmt.Sprintf("YAML parser returned an error when reading this file: `%s`.", job.entry.PathError), - Details: `pint cannot read this file because YAML parser returned an error. -This usually means that you have an indention error or the file doesn't have the YAML structure required by Prometheus for [recording](https://prometheus.io/docs/prometheus/latest/configuration/recording_rules/) and [alerting](https://prometheus.io/docs/prometheus/latest/configuration/alerting_rules/) rules. -If this file is a template that will be rendered into valid YAML then you can instruct pint to ignore some lines using comments, see [pint docs](https://cloudflare.github.io/pint/ignoring.html). -`, - Severity: checks.Fatal, - }, - Owner: job.entry.Owner, - } - case job.entry.Rule.Error.Err != nil: - details := yamlDetails - if job.entry.Rule.Error.Details != "" { - details = job.entry.Rule.Error.Details - } + if job.entry.State == discovery.Unknown { + slog.Warn( + "Bug: unknown rule state", + slog.String("path", job.entry.Path.String()), + slog.Int("line", job.entry.Rule.Lines.First), + slog.String("name", job.entry.Rule.Name()), + ) + } + + start := time.Now() + problems := job.check.Check(ctx, job.entry.Path, job.entry.Rule, job.allEntries) + checkDuration.WithLabelValues(job.check.Reporter()).Observe(time.Since(start).Seconds()) + for _, problem := range problems { results <- reporter.Report{ Path: discovery.Path{ Name: job.entry.Path.Name, @@ -227,42 +159,8 @@ If this file is a template that will be rendered into valid YAML then you can in }, ModifiedLines: job.entry.ModifiedLines, Rule: job.entry.Rule, - Problem: checks.Problem{ - Lines: parser.LineRange{ - First: job.entry.Rule.Error.Line, - Last: job.entry.Rule.Error.Line, - }, - Reporter: yamlParseReporter, - Text: fmt.Sprintf("This rule is not a valid Prometheus rule: `%s`.", job.entry.Rule.Error.Err.Error()), - Details: details, - Severity: checks.Fatal, - }, - Owner: job.entry.Owner, - } - default: - if job.entry.State == discovery.Unknown { - slog.Warn( - "Bug: unknown rule state", - slog.String("path", job.entry.Path.String()), - slog.Int("line", job.entry.Rule.Lines.First), - slog.String("name", job.entry.Rule.Name()), - ) - } - - start := time.Now() - problems := job.check.Check(ctx, job.entry.Path, job.entry.Rule, job.allEntries) - checkDuration.WithLabelValues(job.check.Reporter()).Observe(time.Since(start).Seconds()) - for _, problem := range problems { - results <- reporter.Report{ - Path: discovery.Path{ - Name: job.entry.Path.Name, - SymlinkTarget: job.entry.Path.SymlinkTarget, - }, - ModifiedLines: job.entry.ModifiedLines, - Rule: job.entry.Rule, - Problem: problem, - Owner: job.entry.Owner, - } + Problem: problem, + Owner: job.entry.Owner, } } } diff --git a/cmd/pint/tests/0184_ci_file_ignore.txt b/cmd/pint/tests/0184_ci_file_ignore.txt index 01c4dbc2..93068a35 100644 --- a/cmd/pint/tests/0184_ci_file_ignore.txt +++ b/cmd/pint/tests/0184_ci_file_ignore.txt @@ -23,7 +23,6 @@ cmp stderr ../stderr.txt -- stderr.txt -- level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check on current git branch" base=main -level=INFO msg="No rules found, skipping Prometheus discovery" -- src/rules.yml -- # pint ignore/file - record: rule1 diff --git a/docs/configuration.md b/docs/configuration.md index 7ccbd8b7..1b7fba3e 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -520,7 +520,7 @@ Syntax: rule { match { path = "(.+)" - state = [ "any|added|modified|renamed|unmodified", ... ] + state = [ "any|added|modified|renamed|removed|unmodified", ... ] name = "(.+)" kind = "alerting|recording" command = "ci|lint|watch" @@ -536,7 +536,7 @@ rule { match { ... } ignore { path = "(.+)" - state = [ "any|added|modified|renamed|unmodified", ... ] + state = [ "any|added|modified|renamed|removed|unmodified", ... ] name = "(.+)" kind = "alerting|recording" command = "ci|lint|watch" @@ -566,6 +566,7 @@ rule { - `added` - a rule is being added in a git commit, a rule can only be in this state when running `pint ci` on a pull request. - `modified` - a rule is being modified in a git commit, a rule can only be in this state when running `pint ci` on a pull request. - `renamed` - a rule is being modified in a git commit, a rule can only be in this state when running `pint ci` on a pull request. + - `removed` - a rule is being removed in a git commit, a rule can only be in this state when running `pint ci` on a pull request. - `unmodified` - a rule is not being modified in a git commit when running `pint ci` or other pint command was run that doesn't try to detect which rules were modified. - `match:name` - only rules with names (`record` for recording rules and `alert` for alerting rules) matching this pattern will be checked rule. diff --git a/internal/checks/base.go b/internal/checks/base.go index b4ded357..070f3747 100644 --- a/internal/checks/base.go +++ b/internal/checks/base.go @@ -118,8 +118,9 @@ type Problem struct { } type CheckMeta struct { - States []discovery.ChangeType - IsOnline bool + States []discovery.ChangeType + IsOnline bool + AlwaysEnabled bool } type RuleChecker interface { diff --git a/internal/checks/error.go b/internal/checks/error.go new file mode 100644 index 00000000..fa54e7b2 --- /dev/null +++ b/internal/checks/error.go @@ -0,0 +1,119 @@ +package checks + +import ( + "context" + "errors" + "fmt" + "log/slog" + + "github.com/cloudflare/pint/internal/comments" + "github.com/cloudflare/pint/internal/discovery" + "github.com/cloudflare/pint/internal/parser" +) + +const ( + ErrorCheckName = "error" + + yamlParseReporter = "yaml/parse" + ignoreFileReporter = "ignore/file" + pintCommentReporter = "pint/comment" + + yamlDetails = `This Prometheus rule is not valid. +This usually means that it's missing some required fields.` +) + +func NewErrorCheck(err error) ErrorCheck { + return ErrorCheck{err: err} +} + +type ErrorCheck struct { + err error +} + +func (c ErrorCheck) Meta() CheckMeta { + return CheckMeta{ + States: []discovery.ChangeType{ + discovery.Noop, + discovery.Added, + discovery.Modified, + discovery.Moved, + discovery.Removed, + }, + IsOnline: false, + AlwaysEnabled: true, + } +} + +func (c ErrorCheck) String() string { + return ErrorCheckName +} + +func (c ErrorCheck) Reporter() string { + return ErrorCheckName +} + +func (c ErrorCheck) Check(_ context.Context, _ discovery.Path, rule parser.Rule, _ []discovery.Entry) (problems []Problem) { + var commentErr comments.CommentError + var ignoreErr discovery.FileIgnoreError + + switch { + case errors.As(c.err, &ignoreErr): + slog.Debug("ignore/file report", slog.Any("err", ignoreErr)) + problems = append(problems, Problem{ + Lines: parser.LineRange{ + First: ignoreErr.Line, + Last: ignoreErr.Line, + }, + Reporter: ignoreFileReporter, + Text: ignoreErr.Error(), + Severity: Information, + }) + + case errors.As(c.err, &commentErr): + slog.Debug("invalid comment report", slog.Any("err", commentErr)) + problems = append(problems, Problem{ + Lines: parser.LineRange{ + First: commentErr.Line, + Last: commentErr.Line, + }, + Reporter: pintCommentReporter, + Text: "This comment is not a valid pint control comment: " + commentErr.Error(), + Severity: Warning, + }) + + case c.err != nil: + slog.Debug("yaml syntax report", slog.Any("err", c.err)) + problems = append(problems, Problem{ + Lines: parser.LineRange{ + First: 1, + Last: 1, + }, + Reporter: yamlParseReporter, + Text: fmt.Sprintf("YAML parser returned an error when reading this file: `%s`.", c.err), + Details: `pint cannot read this file because YAML parser returned an error. +This usually means that you have an indention error or the file doesn't have the YAML structure required by Prometheus for [recording](https://prometheus.io/docs/prometheus/latest/configuration/recording_rules/) and [alerting](https://prometheus.io/docs/prometheus/latest/configuration/alerting_rules/) rules. +If this file is a template that will be rendered into valid YAML then you can instruct pint to ignore some lines using comments, see [pint docs](https://cloudflare.github.io/pint/ignoring.html). +`, + Severity: Fatal, + }) + + case rule.Error.Err != nil: + slog.Debug("rule error report", slog.Any("err", rule.Error.Err)) + details := yamlDetails + if rule.Error.Details != "" { + details = rule.Error.Details + } + problems = append(problems, Problem{ + Lines: parser.LineRange{ + First: rule.Error.Line, + Last: rule.Error.Line, + }, + Reporter: yamlParseReporter, + Text: fmt.Sprintf("This rule is not a valid Prometheus rule: `%s`.", rule.Error.Err.Error()), + Details: details, + Severity: Fatal, + }) + } + + return problems +} diff --git a/internal/config/config.go b/internal/config/config.go index 74c13f7c..abe59a2a 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -80,18 +80,27 @@ func (cfg Config) String() string { return string(content) } -func (cfg *Config) GetChecksForRule(ctx context.Context, gen *PrometheusGenerator, entry discovery.Entry) []checks.RuleChecker { +func (cfg *Config) GetChecksForEntry(ctx context.Context, gen *PrometheusGenerator, entry discovery.Entry) []checks.RuleChecker { enabled := []checks.RuleChecker{} defaultMatch := []Match{{State: defaultMatchStates(ctx.Value(CommandKey).(ContextCommandVal))}} proms := gen.ServersForPath(entry.Path.Name) - for _, pr := range baseRules(proms, defaultMatch) { - enabled = pr.entryChecks(ctx, cfg.Checks.Enabled, cfg.Checks.Disabled, enabled, entry) - } - for _, rule := range cfg.Rules { - for _, pr := range parseRule(rule, proms, defaultMatch) { + + if entry.PathError != nil || entry.Rule.Error.Err != nil { + enabled = parsedRule{ + match: defaultMatch, + name: checks.ErrorCheckName, + check: checks.NewErrorCheck(entry.PathError), + }.entryChecks(ctx, cfg.Checks.Enabled, cfg.Checks.Disabled, enabled, entry) + } else { + for _, pr := range baseRules(proms, defaultMatch) { enabled = pr.entryChecks(ctx, cfg.Checks.Enabled, cfg.Checks.Disabled, enabled, entry) } + for _, rule := range cfg.Rules { + for _, pr := range parseRule(rule, proms, defaultMatch) { + enabled = pr.entryChecks(ctx, cfg.Checks.Enabled, cfg.Checks.Disabled, enabled, entry) + } + } } el := []string{} diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 46786c61..5fc70ed7 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -2061,7 +2061,7 @@ rule { defer gen.Stop() require.NoError(t, gen.GenerateStatic()) - checks := cfg.GetChecksForRule(ctx, gen, tc.entry) + checks := cfg.GetChecksForEntry(ctx, gen, tc.entry) checkNames := make([]string, 0, len(checks)) for _, c := range checks { checkNames = append(checkNames, c.String()) diff --git a/internal/config/match.go b/internal/config/match.go index d50980e9..c7ac8b12 100644 --- a/internal/config/match.go +++ b/internal/config/match.go @@ -21,6 +21,7 @@ const ( StateAdded = "added" StateModified = "modified" StateRenamed = "renamed" + StateRemoved = "removed" StateUnmodified = "unmodified" ) @@ -30,7 +31,7 @@ var ( LintCommand ContextCommandVal = "lint" WatchCommand ContextCommandVal = "watch" - CIStates = []string{StateAdded, StateModified, StateRenamed} + CIStates = []string{StateAdded, StateModified, StateRenamed, StateRemoved} AnyStates = []string{StateAny} ) @@ -89,7 +90,7 @@ func (m Match) validate(allowEmpty bool) error { for _, s := range m.State { switch s { - case StateAny, StateAdded, StateModified, StateRenamed, StateUnmodified: + case StateAny, StateAdded, StateModified, StateRenamed, StateRemoved, StateUnmodified: // valid values default: return fmt.Errorf("unknown rule state: %s", s) @@ -340,6 +341,10 @@ func stateMatches(states []string, state discovery.ChangeType) bool { if state == discovery.Moved { return true } + case StateRemoved: + if state == discovery.Removed { + return true + } case StateUnmodified: if state == discovery.Noop { return true diff --git a/internal/config/rule.go b/internal/config/rule.go index 1bcedf2f..a8cb8f4d 100644 --- a/internal/config/rule.go +++ b/internal/config/rule.go @@ -110,6 +110,10 @@ func (rule Rule) validate() (err error) { } func isEnabled(enabledChecks, disabledChecks []string, rule parser.Rule, name string, check checks.RuleChecker, promTags []string) bool { + if check.Meta().AlwaysEnabled { + return true + } + matches := []string{ name, check.String(),