Skip to content

Commit

Permalink
Move error handing out of scan workers
Browse files Browse the repository at this point in the history
  • Loading branch information
prymitive committed Aug 23, 2024
1 parent dd52e56 commit 8c3a9ab
Show file tree
Hide file tree
Showing 9 changed files with 176 additions and 140 deletions.
150 changes: 24 additions & 126 deletions cmd/pint/scan.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,32 +2,19 @@ package main

import (
"context"
"errors"
"fmt"
"log/slog"
"sync"
"time"

"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")
Expand Down Expand Up @@ -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",
Expand All @@ -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 {
Expand All @@ -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)
Expand Down Expand Up @@ -154,115 +139,28 @@ 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,
SymlinkTarget: job.entry.Path.SymlinkTarget,
},
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,
}
}
}
Expand Down
1 change: 0 additions & 1 deletion cmd/pint/tests/0184_ci_file_ignore.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 3 additions & 2 deletions docs/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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.
Expand Down
5 changes: 3 additions & 2 deletions internal/checks/base.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
119 changes: 119 additions & 0 deletions internal/checks/error.go
Original file line number Diff line number Diff line change
@@ -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
}
Loading

0 comments on commit 8c3a9ab

Please sign in to comment.