Get rid of rulefmt.Parse

rulefmt.Parse checks if a rule file will pass Prometheus checks but it's very expensive.
On top of that it does a lot of what we do later on anyway, but without giving us same level of detail.

.github/spellcheck/wordlist.txt

@@ -39,6 +39,7 @@ promql
 PromQL
 PRs
 prymitive
+rulefmt
 samber
 SNI
 symlink

cmd/pint/scan.go

@@ -5,12 +5,9 @@ import (
 	"errors"
 	"fmt"
 	"log/slog"
-	"regexp"
-	"strconv"
 	"sync"
 	"time"
 
-	"github.com/prometheus/prometheus/model/rulefmt"
 	"go.uber.org/atomic"
 
 	"github.com/cloudflare/pint/internal/checks"
@@ -22,41 +19,14 @@ import (
 	"github.com/cloudflare/pint/internal/reporter"
 )
 
-var (
-	yamlErrRe          = regexp.MustCompile("^yaml: line (.+): (.+)")
-	yamlUnmarshalErrRe = regexp.MustCompile("^yaml: unmarshal errors:\n  line (.+): (.+)")
-	rulefmtGroupRe     = regexp.MustCompile("^([0-9]+):[0-9]+: group \".+\", rule [0-9]+, (.+)")
-	rulefmtGroupnameRe = regexp.MustCompile("^([0-9]+):[0-9]+: (groupname: .+)")
-)
-
 const (
 	yamlParseReporter   = "yaml/parse"
 	ignoreFileReporter  = "ignore/file"
 	pintCommentReporter = "pint/comment"
-)
-
-func tryDecodingYamlError(err error) (l int, s string) {
-	s = err.Error()
-
-	werr := &rulefmt.WrappedError{}
-	if errors.As(err, &werr) {
-		if uerr := werr.Unwrap(); uerr != nil {
-			s = uerr.Error()
-		}
-	}
 
-	for _, re := range []*regexp.Regexp{yamlErrRe, yamlUnmarshalErrRe, rulefmtGroupRe, rulefmtGroupnameRe} {
-		parts := re.FindStringSubmatch(err.Error())
-		if len(parts) > 2 {
-			line, err2 := strconv.Atoi(parts[1])
-			if err2 != nil || line <= 0 {
-				return 1, s
-			}
-			return line, parts[2]
-		}
-	}
-	return 1, s
-}
+	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 {
@@ -224,7 +194,6 @@ func scanWorker(ctx context.Context, jobs <-chan scanJob, results chan<- reporte
 					Owner: job.entry.Owner,
 				}
 			case job.entry.PathError != nil:
-				line, e := tryDecodingYamlError(job.entry.PathError)
 				results <- reporter.Report{
 					Path: discovery.Path{
 						Name:          job.entry.Path.Name,
@@ -233,11 +202,11 @@ func scanWorker(ctx context.Context, jobs <-chan scanJob, results chan<- reporte
 					ModifiedLines: job.entry.ModifiedLines,
 					Problem: checks.Problem{
 						Lines: parser.LineRange{
-							First: line,
-							Last:  line,
+							First: 1,
+							Last:  1,
 						},
 						Reporter: yamlParseReporter,
-						Text:     fmt.Sprintf("YAML parser returned an error when reading this file: `%s`.", e),
+						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).
@@ -247,6 +216,10 @@ If this file is a template that will be rendered into valid YAML then you can in
 					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
+				}
 				results <- reporter.Report{
 					Path: discovery.Path{
 						Name:          job.entry.Path.Name,
@@ -261,8 +234,7 @@ If this file is a template that will be rendered into valid YAML then you can in
 						},
 						Reporter: yamlParseReporter,
 						Text:     fmt.Sprintf("This rule is not a valid Prometheus rule: `%s`.", job.entry.Rule.Error.Err.Error()),
-						Details: `This Prometheus rule is not valid.
-This usually means that it's missing some required fields.`,
+						Details:  details,
 						Severity: checks.Fatal,
 					},
 					Owner: job.entry.Owner,

cmd/pint/tests/0004_fail_invalid_yaml.txt

@@ -5,9 +5,9 @@ cmp stderr stderr.txt
 -- stderr.txt --
 level=INFO msg="Loading configuration file" path=.pint.hcl
 level=INFO msg="Finding all rules to check" paths=["rules"]
-level=ERROR msg="Failed to parse file content" err="yaml: line 4: did not find expected key" path=rules/bad.yaml lines=1-7
-rules/bad.yaml:4 Fatal: YAML parser returned an error when reading this file: `did not find expected key`. (yaml/parse)
- 4 | 
+level=WARN msg="Failed to parse file content" err="error at line 4: did not find expected key" path=rules/bad.yaml lines=1-7
+rules/bad.yaml:1 Fatal: YAML parser returned an error when reading this file: `error at line 4: did not find expected key`. (yaml/parse)
+ 1 | xxx:
 
 rules/ok.yml:5 Fatal: Prometheus failed to parse the query with this PromQL error: unclosed left bracket. (promql/syntax)
  5 |     expr: sum(foo[5m)

cmd/pint/tests/0010_syntax_check.txt

@@ -5,9 +5,9 @@ cmp stderr stderr.txt
 -- stderr.txt --
 level=INFO msg="Loading configuration file" path=.pint.hcl
 level=INFO msg="Finding all rules to check" paths=["rules"]
-level=ERROR msg="Failed to parse file content" err="yaml: line 6: did not find expected '-' indicator" path=rules/1.yaml lines=1-12
-rules/1.yaml:6 Fatal: YAML parser returned an error when reading this file: `did not find expected '-' indicator`. (yaml/parse)
- 6 | 
+level=WARN msg="Failed to parse file content" err="error at line 6: did not find expected '-' indicator" path=rules/1.yaml lines=1-12
+rules/1.yaml:1 Fatal: YAML parser returned an error when reading this file: `error at line 6: did not find expected '-' indicator`. (yaml/parse)
+ 1 | - alert: Good
 
 level=INFO msg="Problems found" Fatal=1
 level=ERROR msg="Fatal error" err="found 1 problem(s) with severity Bug or higher"

cmd/pint/tests/0067_relaxed.txt

@@ -5,8 +5,9 @@ cmp stderr stderr.txt
 -- stderr.txt --
 level=INFO msg="Loading configuration file" path=.pint.hcl
 level=INFO msg="Finding all rules to check" paths=["rules"]
-rules/strict.yml:2 Fatal: YAML parser returned an error when reading this file: `cannot unmarshal !!seq into rulefmt.RuleGroups`. (yaml/parse)
- 2 | - alert: No Owner
+level=WARN msg="Failed to parse file content" err="error at line 2: top level field must be a groups key, got list" path=rules/strict.yml lines=1-4
+rules/strict.yml:1 Fatal: YAML parser returned an error when reading this file: `error at line 2: top level field must be a groups key, got list`. (yaml/parse)
+ 1 | {%- raw %} # pint ignore/line
 
 level=INFO msg="Problems found" Fatal=1
 level=ERROR msg="Fatal error" err="found 1 problem(s) with severity Bug or higher"

cmd/pint/tests/0071_ci_owner.txt

@@ -56,6 +56,7 @@ groups:
 
 -- src/.pint.hcl --
 ci {
+  include    = [".+.yml"]
   baseBranch = "main"
 }
 repository {

cmd/pint/tests/0074_strict_error.txt

@@ -4,8 +4,9 @@ cmp stderr stderr.txt
 
 -- stderr.txt --
 level=INFO msg="Finding all rules to check" paths=["rules"]
-rules/strict.yml:2 Fatal: YAML parser returned an error when reading this file: `field alert not found in type rulefmt.RuleGroup`. (yaml/parse)
- 2 | - alert: Conntrack_Table_Almost_Full
+level=WARN msg="Failed to parse file content" err="error at line 2: invalid group key alert" path=rules/strict.yml lines=1-9
+rules/strict.yml:1 Fatal: YAML parser returned an error when reading this file: `error at line 2: invalid group key alert`. (yaml/parse)
+ 1 | groups:
 
 level=INFO msg="Problems found" Fatal=1
 level=ERROR msg="Fatal error" err="found 1 problem(s) with severity Bug or higher"

cmd/pint/tests/0076_ci_group_errors.txt

@@ -75,6 +75,7 @@ groups:
 -- src/.pint.hcl --
 ci {
   baseBranch = "main"
+  include    = [".+.yml"]
 }
 repository {
   bitbucket {

cmd/pint/tests/0078_repeated_group.txt

@@ -4,8 +4,9 @@ cmp stderr stderr.txt
 
 -- stderr.txt --
 level=INFO msg="Finding all rules to check" paths=["rules"]
-rules/strict.yml:4 Fatal: YAML parser returned an error when reading this file: `groupname: "foo" is repeated in the same file`. (yaml/parse)
- 4 | - name: foo
+level=WARN msg="Failed to parse file content" err="error at line 4: duplicated group name" path=rules/strict.yml lines=1-5
+rules/strict.yml:1 Fatal: YAML parser returned an error when reading this file: `error at line 4: duplicated group name`. (yaml/parse)
+ 1 | groups:
 
 level=INFO msg="Problems found" Fatal=1
 level=ERROR msg="Fatal error" err="found 1 problem(s) with severity Bug or higher"

cmd/pint/tests/0123_ci_owner_allowed.txt

@@ -65,6 +65,7 @@ owners {
 }
 ci {
   baseBranch = "main"
+  include    = [".+.yml"]
 }
 repository {
   bitbucket {

cmd/pint/tests/0160_ci_comment_edit.txt

@@ -100,6 +100,7 @@ groups:
 -- src/.pint.hcl --
 ci {
   baseBranch = "main"
+  include    = [".+.yml"]
 }
 prometheus "prom" {
   uri     = "http://127.0.0.1:7160"

cmd/pint/tests/0161_ci_move_files.txt

@@ -78,6 +78,7 @@ groups:
 -- src/.pint.hcl --
 ci {
   baseBranch = "main"
+  include    = [".+.yml"]
 }
 prometheus "prom1" {
   uri      = "http://127.0.0.1:7161/1"

cmd/pint/tests/0162_ci_deleted_dependency.txt

@@ -47,6 +47,7 @@ groups:
 -- src/.pint.hcl --
 ci {
   baseBranch = "main"
+  include    = [".+.yml"]
 }
 prometheus "prom" {
   uri      = "http://127.0.0.1:7162"

cmd/pint/tests/0167_rule_duplicate_symlink.txt

@@ -28,6 +28,7 @@ cmp stderr ../stderrV1.txt
 -- stderrV1.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=WARN msg="Failed to parse file content" err="error at line 1: top level field must be a groups key, got string" path=.pint.hcl lines=1-20
 level=INFO msg="Configured new Prometheus server" name=prom1 uris=1 uptime=up tags=[] include=["^rules.yml$"] exclude=[]
 level=INFO msg="Configured new Prometheus server" name=prom2 uris=1 uptime=up tags=[] include=["^symlink.yml$"] exclude=[]
 -- stderrV2.txt --

cmd/pint/tests/0173_rule_duplicate_move.txt

@@ -25,6 +25,7 @@ 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=WARN msg="Failed to parse file content" err="error at line 1: top level field must be a groups key, got string" path=.pint.hcl lines=1-24
 level=INFO msg="Configured new Prometheus server" name=prom1 uris=1 uptime=up tags=[] include=["^rules/alert.*$"] exclude=[]
 level=INFO msg="Configured new Prometheus server" name=prom2a uris=1 uptime=up tags=[] include=["^rules/record.*$"] exclude=[]
 level=INFO msg="Configured new Prometheus server" name=prom2b uris=1 uptime=up tags=[] include=["^rules/record.*$"] exclude=[]

docs/changelog.md

@@ -7,6 +7,10 @@
 - Don't suggest using `humanize` when alert template is already using printf on format the `$value`.
 - Fixed git history parsing when running `pint ci` on a branch that include merge commits.
 
+### Changed
+
+- Refactored YAML syntax checks to avoid using [rulefmt.Parse](https://pkg.go.dev/github.com/prometheus/prometheus@v0.53.0/model/rulefmt#Parse) and effectively parsing rules twice. Some error messages will have different formatting.
+
 ## v0.60.0
 
 ### Fixed

internal/checks/base_test.go

@@ -173,7 +173,7 @@ func runTests(t *testing.T, testCases []checkTest) {
 }
 
 func parseContent(content string) (entries []discovery.Entry, err error) {
-	p := parser.NewParser()
+	p := parser.NewParser(false)
 	rules, err := p.Parse([]byte(content))
 	if err != nil {
 		return nil, err

internal/checks/template_test.go

@@ -105,7 +105,7 @@ func TestTemplatedRegexpExpand(t *testing.T) {
 }
 
 func newMustRule(content string) parser.Rule {
-	p := parser.NewParser()
+	p := parser.NewParser(false)
 	rules, err := p.Parse([]byte(content))
 	if err != nil {
 		panic(err)

internal/config/config_test.go

@@ -133,7 +133,7 @@ func TestSetDisabledChecks(t *testing.T) {
 }
 
 func newRule(t *testing.T, content string) parser.Rule {
-	p := parser.NewParser()
+	p := parser.NewParser(false)
 	rules, err := p.Parse([]byte(content))
 	if err != nil {
 		t.Error(err)

internal/discovery/discovery.go

@@ -6,10 +6,8 @@ import (
 	"fmt"
 	"io"
 	"log/slog"
-	"strings"
 	"time"
 
-	"github.com/prometheus/prometheus/model/rulefmt"
 	"golang.org/x/exp/slices"
 
 	"github.com/cloudflare/pint/internal/comments"
@@ -23,17 +21,6 @@ const (
 	RuleOwnerComment         = "rule/owner"
 )
 
-var ignoredErrors = []string{
-	"one of 'record' or 'alert' must be set",
-	"field 'expr' must be set in rule",
-	"could not parse expression: ",
-	"cannot unmarshal !!seq into rulefmt.ruleGroups",
-	": template: __",
-	"invalid label name: ",
-	"invalid annotation name: ",
-	"invalid recording rule name: ",
-}
-
 type FileIgnoreError struct {
 	Err  error
 	Line int
@@ -43,16 +30,6 @@ func (fe FileIgnoreError) Error() string {
 	return fe.Err.Error()
 }
 
-func isStrictIgnored(err error) bool {
-	s := err.Error()
-	for _, ign := range ignoredErrors {
-		if strings.Contains(s, ign) {
-			return true
-		}
-	}
-	return false
-}
-
 type ChangeType uint8
 
 func (c ChangeType) String() string {
@@ -113,8 +90,6 @@ type Entry struct {
 }
 
 func readRules(reportedPath, sourcePath string, r io.Reader, isStrict bool) (entries []Entry, err error) {
-	p := parser.NewParser()
-
 	content, fileComments, err := parser.ReadContent(r)
 	if err != nil {
 		return nil, err
@@ -182,36 +157,10 @@ func readRules(reportedPath, sourcePath string, r io.Reader, isStrict bool) (ent
 		return entries, nil
 	}
 
-	if isStrict {
-		if _, errs := rulefmt.Parse(content.Body); len(errs) > 0 {
-			seen := map[string]struct{}{}
-			for _, err := range errs {
-				if isStrictIgnored(err) {
-					continue
-				}
-				if _, ok := seen[err.Error()]; ok {
-					continue
-				}
-				seen[err.Error()] = struct{}{}
-				entries = append(entries, Entry{
-					Path: Path{
-						Name:          sourcePath,
-						SymlinkTarget: reportedPath,
-					},
-					PathError:     err,
-					Owner:         fileOwner,
-					ModifiedLines: contentLines.Expand(),
-				})
-			}
-			if len(entries) > 0 {
-				return entries, nil
-			}
-		}
-	}
-
+	p := parser.NewParser(isStrict)
 	rules, err := p.Parse(content.Body)
 	if err != nil {
-		slog.Error(
+		slog.Warn(
 			"Failed to parse file content",
 			slog.Any("err", err),
 			slog.String("path", sourcePath),

internal/discovery/discovery_test.go

@@ -24,7 +24,7 @@ func (r failingReader) Read(_ []byte) (int, error) {
 
 func TestReadRules(t *testing.T) {
 	mustParse := func(offset int, s string) parser.Rule {
-		p := parser.NewParser()
+		p := parser.NewParser(false)
 		r, err := p.Parse([]byte(strings.Repeat("\n", offset) + s))
 		if err != nil {
 			panic(fmt.Sprintf("failed to parse rule:\n---\n%s\n---\nerror: %s", s, err))