Prometheus : parse alerting before loading WAL #16462

marcodebba · 2025-04-21T21:14:17Z

Signed-off-by: pc-casa-ubuntu <marcodebonis74@gmail.com>

tjhop · 2025-04-24T06:22:13Z

rules/manager.go

+func ParseFile(fn string) (interface{}, error) {
+	content, err := os.ReadFile(fn)
+	if err != nil {
+		return nil, err
+	}
+
+	var rules interface{}
+	err = yaml.Unmarshal(content, &rules)
+	if err != nil {
+		return nil, err
+	}
+
+	return rules, nil
+}


There's an existing function in the rulefmt package in prometheus called ParseFile() that seems to do both yaml validation as well as validation of the literal rule groups. Is this new function necessary?

prometheus/model/rulefmt/rulefmt.go

Lines 364 to 375 in 9659e30

// ParseFile reads and parses rules from a file.

func ParseFile(file string, ignoreUnknownFields bool) (*RuleGroups, []error) {

b, err := os.ReadFile(file)

if err != nil {

return nil, []error{fmt.Errorf("%s: %w", file, err)}

}

rgs, errs := Parse(b, ignoreUnknownFields)

for i := range errs {

errs[i] = fmt.Errorf("%s: %w", file, errs[i])

}

return rgs, errs

}

tjhop · 2025-04-24T06:23:19Z

.gitignore

+.idea/modules.xml
+.idea/prometheus.iml
+.idea/vcs.xml
+.idea/workspace.xml


These should go in your global git ignore file:
https://sebastiandedeyne.com/setting-up-a-global-gitignore-file/

The spurious lines are still in here. Please remove them.

Sorry, let me fix this evening . Thanks for suggestions

Copilot

Pull Request Overview

This PR fixes #11486 by introducing early parsing and validation of alerting rules before loading the WAL.

Added a new ParseFile function in rules/manager.go to parse YAML rule files.
Introduced a TestParseFile unit test in rules/manager_test.go to verify YAML parsing.
Updated cmd/prometheus/main.go to integrate rule file parsing and exit early on errors.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
rules/manager_test.go	Added TestParseFile to verify YAML parsing of rule files.
rules/manager.go	Introduced ParseFile to load and unmarshal YAML rule definitions.
cmd/prometheus/main.go	Integrated rule file parsing and error handling during startup.

Comments suppressed due to low confidence (1)

rules/manager_test.go:2587

[nitpick] To improve test coverage and robustness, add concrete assertions to validate the structure of the parsed rules rather than checking only for a non-nil result.

// Optionally, you can add more assertions to validate the structure of the parsed rules.

aknuds1

I agree with @tjhop that rulefmt.ParseFile should be used.

aknuds1 · 2025-04-25T10:49:04Z

cmd/prometheus/main.go

+
+	// This section of the code is responsible for validating and parsing rule files
+	// specified in the Prometheus configuration. It ensures that all rule files
+	// match the provided patterns and are syntactically correct before Prometheus
+	// starts processing them.
+	// 1. Iterate through the list of rule file patterns specified in the configuration (`cfgFile.RuleFiles`).
+	// 2. Use `filepath.Glob` to expand each pattern into a list of matching files.
+	// 3. If an error occurs during pattern expansion, log the error, resolve the absolute path of the pattern, and exit with an error code.
+	// 4. For each matching file, attempt to parse it using `rules.ParseFile`.
+	// 5. If parsing fails, log the error, resolve the absolute path of the file, and exit with an error code.
+	//
+	// Errors are logged with details such as the configuration file path, the rule file pattern, and the specific error encountered.
+
+	if len(cfgFile.RuleFiles) > 0 {
+		for _, pat := range cfgFile.RuleFiles {
+			files, err := filepath.Glob(pat)
+			if err != nil {
+				absPath, pathErr := filepath.Abs(pat)
+				if pathErr != nil {
+					absPath = pat
+				}
+				logger.Error(fmt.Sprintf("Error loading rules pattern (--config.file=%q)", cfg.configFile), "file", absPath, "pattern", pat, "err", err)
+				os.Exit(2)
+			}
+			for _, fn := range files {
+				if _, err := rules.ParseFile(fn); err != nil {
+					absPath, pathErr := filepath.Abs(fn)
+					if pathErr != nil {
+						absPath = fn
+					}
+					logger.Error(fmt.Sprintf("Error loading rules file (--config.file=%q)", cfg.configFile), "file", absPath, "err", err)
+					os.Exit(2)
+				}
+			}
+		}
+	}


It's better to fully embed the logic in a function in the rules package:

Suggested change

// This section of the code is responsible for validating and parsing rule files

// specified in the Prometheus configuration. It ensures that all rule files

// match the provided patterns and are syntactically correct before Prometheus

// starts processing them.

// 1. Iterate through the list of rule file patterns specified in the configuration (`cfgFile.RuleFiles`).

// 2. Use `filepath.Glob` to expand each pattern into a list of matching files.

// 3. If an error occurs during pattern expansion, log the error, resolve the absolute path of the pattern, and exit with an error code.

// 4. For each matching file, attempt to parse it using `rules.ParseFile`.

// 5. If parsing fails, log the error, resolve the absolute path of the file, and exit with an error code.

//

// Errors are logged with details such as the configuration file path, the rule file pattern, and the specific error encountered.

if len(cfgFile.RuleFiles) > 0 {

for _, pat := range cfgFile.RuleFiles {

files, err := filepath.Glob(pat)

if err != nil {

absPath, pathErr := filepath.Abs(pat)

if pathErr != nil {

absPath = pat

}

logger.Error(fmt.Sprintf("Error loading rules pattern (--config.file=%q)", cfg.configFile), "file", absPath, "pattern", pat, "err", err)

os.Exit(2)

}

for _, fn := range files {

if _, err := rules.ParseFile(fn); err != nil {

absPath, pathErr := filepath.Abs(fn)

if pathErr != nil {

absPath = fn

}

logger.Error(fmt.Sprintf("Error loading rules file (--config.file=%q)", cfg.configFile), "file", absPath, "err", err)

os.Exit(2)

}

}

}

}

if err := rules.ParseFiles(cfgFile.RuleFiles); err != nil {

absPath, pathErr := filepath.Abs(cfg.configFile)

if pathErr != nil {

absPath = cfg.configFile

}

logger.Error(fmt.Sprintf("Error loading rule file patterns from config (--config.file=%q)", cfg.configFile), "file", absPath, "err", err)

os.Exit(2)

}

The comment hasn't been addressed yet.

I've used the existing function and deleted my not useful method.

aknuds1 · 2025-04-25T10:51:35Z

rules/manager.go

+func ParseFile(fn string) (interface{}, error) {
+	content, err := os.ReadFile(fn)
+	if err != nil {
+		return nil, err
+	}
+
+	var rules interface{}
+	err = yaml.Unmarshal(content, &rules)
+	if err != nil {
+		return nil, err
+	}
+
+	return rules, nil
+}


The following makes more sense to me:

Suggested change

func ParseFile(fn string) (interface{}, error) {

content, err := os.ReadFile(fn)

if err != nil {

return nil, err

}

var rules interface{}

err = yaml.Unmarshal(content, &rules)

if err != nil {

return nil, err

}

return rules, nil

}

// ParseFiles parses the rule files corresponding to glob patterns.

func ParseFiles(patterns []string) error {

files := map[string]string{}

for _, pat := range patterns {

fns, err := filepath.Glob(pat)

if err != nil {

return fmt.Errorf("failed retrieving rule files for %q: %w", pat, err)

}

for _, fn := range fns {

absPath, err := filepath.Abs(fn)

if err != nil {

absPath = fn

}

cleanPath, err := filepath.EvalSymlinks(absPath)

if err != nil {

return fmt.Errorf("failed evaluating rule file path %q (pattern: %q): %w", absPath, pat, err)

}

files[cleanPath] = pat

}

}

for fn, pat := range files {

_, errs := rulefmt.ParseFile(fn, false)

if len(errs) > 0 {

return fmt.Errorf("parse rules from file %q (pattern: %q): %w", fn, pat, errors.Join(errs...))

}

}

return nil

}

aknuds1 · 2025-04-25T11:01:37Z

rules/manager_test.go

+func TestParseFile(t *testing.T) {
+	// Create a temporary directory for the test.
+	tmpDir := t.TempDir()
+
+	// Define the test file path.
+	testFile := filepath.Join(tmpDir, "test_rules.yml")
+
+	// Define the content of the test YAML file.
+	testContent := `
+groups:
+  - name: example
+    rules:
+      - alert: HighRequestLatency
+        expr: job:request_latency_seconds:mean5m{job="myjob"} > 0.5
+        for: 10m
+        labels:
+          severity: warning
+        annotations:
+          summary: "High request latency"
+`
+
+	// Write the test content to the file.
+	err := os.WriteFile(testFile, []byte(testContent), 0o644)
+	require.NoError(t, err)
+
+	// Call the ParseFile function.
+	result, err := ParseFile(testFile)
+	require.NoError(t, err)
+
+	// Assert that the result is not nil.
+	require.NotNil(t, result, "Expected parsed rules to be non-nil")
+
+	// Optionally, you can add more assertions to validate the structure of the parsed rules.
+	// For example, if you know the expected structure, you can cast `result` to the appropriate type
+	// and check its fields.
+}


This can be simplified as follows, even with an error case added. Note that the error case needs an extra file invalid-rules.yaml in rules/fixtures/.

Suggested change

func TestParseFile(t *testing.T) {

// Create a temporary directory for the test.

tmpDir := t.TempDir()

// Define the test file path.

testFile := filepath.Join(tmpDir, "test_rules.yml")

// Define the content of the test YAML file.

testContent := `

groups:

- name: example

rules:

- alert: HighRequestLatency

expr: job:request_latency_seconds:mean5m{job="myjob"} > 0.5

for: 10m

labels:

severity: warning

annotations:

summary: "High request latency"

`

// Write the test content to the file.

err := os.WriteFile(testFile, []byte(testContent), 0o644)

require.NoError(t, err)

// Call the ParseFile function.

result, err := ParseFile(testFile)

require.NoError(t, err)

// Assert that the result is not nil.

require.NotNil(t, result, "Expected parsed rules to be non-nil")

// Optionally, you can add more assertions to validate the structure of the parsed rules.

// For example, if you know the expected structure, you can cast `result` to the appropriate type

// and check its fields.

}

func TestParseFiles(t *testing.T) {

t.Run("good files", func(t *testing.T) {

err := ParseFiles([]string{filepath.Join("fixtures", "rules.y*ml")})

require.NoError(t, err)

})

t.Run("bad files", func(t *testing.T) {

err := ParseFiles([]string{filepath.Join("fixtures", "invalid-rules.y*ml")})

require.ErrorContains(t, err, "field unexpected_field not found in type rulefmt.Rule")

})

}

beorn7

Thanks @tjhop and @aknuds1 for your reviews.

@marcodebba you resolved all the comments, but you haven't really addressed them.
Have you maybe forgotten to push your latest changes?

I agree with everything the reviewers said, and I think you should address their comments.

beorn7 · 2025-04-29T15:24:12Z

.gitignore

+.idea/modules.xml
+.idea/prometheus.iml
+.idea/vcs.xml
+.idea/workspace.xml


The spurious lines are still in here. Please remove them.

beorn7 · 2025-04-29T15:28:17Z

cmd/prometheus/main.go

+
+	// This section of the code is responsible for validating and parsing rule files
+	// specified in the Prometheus configuration. It ensures that all rule files
+	// match the provided patterns and are syntactically correct before Prometheus
+	// starts processing them.
+	// 1. Iterate through the list of rule file patterns specified in the configuration (`cfgFile.RuleFiles`).
+	// 2. Use `filepath.Glob` to expand each pattern into a list of matching files.
+	// 3. If an error occurs during pattern expansion, log the error, resolve the absolute path of the pattern, and exit with an error code.
+	// 4. For each matching file, attempt to parse it using `rules.ParseFile`.
+	// 5. If parsing fails, log the error, resolve the absolute path of the file, and exit with an error code.
+	//
+	// Errors are logged with details such as the configuration file path, the rule file pattern, and the specific error encountered.
+
+	if len(cfgFile.RuleFiles) > 0 {
+		for _, pat := range cfgFile.RuleFiles {
+			files, err := filepath.Glob(pat)
+			if err != nil {
+				absPath, pathErr := filepath.Abs(pat)
+				if pathErr != nil {
+					absPath = pat
+				}
+				logger.Error(fmt.Sprintf("Error loading rules pattern (--config.file=%q)", cfg.configFile), "file", absPath, "pattern", pat, "err", err)
+				os.Exit(2)
+			}
+			for _, fn := range files {
+				if _, err := rules.ParseFile(fn); err != nil {
+					absPath, pathErr := filepath.Abs(fn)
+					if pathErr != nil {
+						absPath = fn
+					}
+					logger.Error(fmt.Sprintf("Error loading rules file (--config.file=%q)", cfg.configFile), "file", absPath, "err", err)
+					os.Exit(2)
+				}
+			}
+		}
+	}


The comment hasn't been addressed yet.

beorn7 · 2025-04-29T15:33:32Z

cmd/prometheus/main.go

+	// This section of the code is responsible for validating and parsing rule files
+	// specified in the Prometheus configuration. It ensures that all rule files
+	// match the provided patterns and are syntactically correct before Prometheus
+	// starts processing them.
+	// 1. Iterate through the list of rule file patterns specified in the configuration (`cfgFile.RuleFiles`).
+	// 2. Use `filepath.Glob` to expand each pattern into a list of matching files.
+	// 3. If an error occurs during pattern expansion, log the error, resolve the absolute path of the pattern, and exit with an error code.
+	// 4. For each matching file, attempt to parse it using `rules.ParseFile`.
+	// 5. If parsing fails, log the error, resolve the absolute path of the file, and exit with an error code.
+	//
+	// Errors are logged with details such as the configuration file path, the rule file pattern, and the specific error encountered.


While I like readable comments in proper English, this one is a bit too verbose.

A general good advice is that code should not be an English version of the Go code. You can assume that the reader of your code knows Go well (maybe even better than English ;).

If you move the code below into a function rules.ParseFiles, your comment here can be its doc comment, but it should still focus more on what the function does rather than how.

Thanks for suggestions @beorn7 . I'll try to fix everything today

Co-authored-by: marcodebba <marcodebonis74@gmail.com> Co-authored-by: subhramit <subhramit.bb@live.in> Signed-off-by: subhramit <subhramit.bb@live.in>

Builds over prometheus#16462 Co-authored-by: marcodebba <marcodebonis74@gmail.com> Co-authored-by: subhramit <subhramit.bb@live.in> Signed-off-by: Subhramit Basu <subhramit.bb@live.in>

Builds over prometheus#16462 Addresses reviews, adds invalid rule test file Co-authored-by: marcodebba <marcodebon1s74@gmail.com> Co-authored-by: subhramit <subhramit.bb@live.in> Signed-off-by: Subhramit Basu <subhramit.bb@live.in>

Builds over prometheus#16462 Addresses comments, adds invalid rules file Co-authored-by: marcodebba <marcodebonis74@gmail.com> Co-authored-by: subhramit <subhramit.bb@live.in> Signed-off-by: subhramit <subhramit.bb@live.in>

beorn7 · 2025-05-16T20:07:05Z

Closing in lieu of #16601 .

Builds over #16462 Addresses comments, adds invalid rules file Signed-off-by: subhramit <subhramit.bb@live.in> Co-authored-by: marcodebba <marcodebonis74@gmail.com>

marcodebba force-pushed the main branch from c2f157f to 4501196 Compare April 22, 2025 13:11

marcodebba requested a review from roidelapluie as a code owner April 22, 2025 13:11

marcodebba force-pushed the main branch 2 times, most recently from 2ca87e7 to b76fec7 Compare April 22, 2025 13:26

marcodebba added 4 commits April 22, 2025 15:31

changed some header

7f297a4

Signed-off-by: pc-casa-ubuntu <marcodebonis74@gmail.com>

Made new commit to check rules

6dc92cd

Signed-off-by: pc-casa-ubuntu <marcodebonis74@gmail.com>

Add: Added test for ParseRule

34996e9

Signed-off-by: pc-casa-ubuntu <marcodebonis74@gmail.com>

Fix: fixed create file for test

9bc3e5b

Signed-off-by: pc-casa-ubuntu <marcodebonis74@gmail.com>

marcodebba force-pushed the main branch from b76fec7 to 9bc3e5b Compare April 22, 2025 13:33

beorn7 added the contribfest 🎉 label Apr 22, 2025

beorn7 requested review from beorn7 and removed request for roidelapluie April 22, 2025 22:32

tjhop reviewed Apr 24, 2025

View reviewed changes

aknuds1 requested a review from Copilot April 25, 2025 07:29

Copilot AI reviewed Apr 25, 2025

View reviewed changes

aknuds1 requested changes Apr 25, 2025

View reviewed changes

aknuds1 added kind/bug kind/enhancement and removed kind/bug labels Apr 25, 2025

marcodebba requested a review from aknuds1 April 25, 2025 15:17

Merge branch 'prometheus:main' into main

ac964f0

beorn7 requested changes Apr 29, 2025

View reviewed changes

This was referenced May 15, 2025

Add parse alerting for rules files before loading WAL #16601

Merged

parse alerting before loading WAL #11486

Closed

beorn7 closed this May 16, 2025

aknuds1 pushed a commit that referenced this pull request May 26, 2025

Add parse alerting for rules files (#16601)

44e27a8

Builds over #16462 Addresses comments, adds invalid rules file Signed-off-by: subhramit <subhramit.bb@live.in> Co-authored-by: marcodebba <marcodebonis74@gmail.com>

	// ParseFile reads and parses rules from a file.
	func ParseFile(file string, ignoreUnknownFields bool) (*RuleGroups, []error) {
	b, err := os.ReadFile(file)
	if err != nil {
	return nil, []error{fmt.Errorf("%s: %w", file, err)}
	}
	rgs, errs := Parse(b, ignoreUnknownFields)
	for i := range errs {
	errs[i] = fmt.Errorf("%s: %w", file, errs[i])
	}
	return rgs, errs
	}

-func ParseFile(fn string) (interface{}, error) {
-	content, err := os.ReadFile(fn)
-	if err != nil {
-		return nil, err
-	}
-	var rules interface{}
-	err = yaml.Unmarshal(content, &rules)
-	if err != nil {
-		return nil, err
-	}
-	return rules, nil
-}
+// ParseFiles parses the rule files corresponding to glob patterns.
+func ParseFiles(patterns []string) error {
+	files := map[string]string{}
+	for _, pat := range patterns {
+		fns, err := filepath.Glob(pat)
+		if err != nil {
+			return fmt.Errorf("failed retrieving rule files for %q: %w", pat, err)
+		}
+		for _, fn := range fns {
+			absPath, err := filepath.Abs(fn)
+			if err != nil {
+				absPath = fn
+			}
+			cleanPath, err := filepath.EvalSymlinks(absPath)
+			if err != nil {
+				return fmt.Errorf("failed evaluating rule file path %q (pattern: %q): %w", absPath, pat, err)
+			}
+			files[cleanPath] = pat
+		}
+	}
+	for fn, pat := range files {
+		_, errs := rulefmt.ParseFile(fn, false)
+		if len(errs) > 0 {
+			return fmt.Errorf("parse rules from file %q (pattern: %q): %w", fn, pat, errors.Join(errs...))
+		}
+	}
+	return nil
+}

Prometheus : parse alerting before loading WAL #16462

Prometheus : parse alerting before loading WAL #16462

Uh oh!

Conversation

marcodebba commented Apr 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

aknuds1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beorn7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beorn7 commented May 16, 2025

Uh oh!

Uh oh!