Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix (auto-reload): failing to auto-reload on changes to rule and scrape config files #15675

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix (auto-reload): failing to auto-reload on changes to rule and scrape config files #15675

wants to merge 1 commit into from

Conversation

cxdy
Copy link

@cxdy cxdy commented Dec 13, 2024 •
edited
Loading

Fixes #15673

An issue was reported in the CNCF Slack regarding the new auto-reload feature in Prometheus 3.0 failing to reload on changes to rule files and scrape config files if they aren't in the same directory as prometheus.yml.

This change verifies that dir does not exist in the specified rule/scrape_config_file string before setting it.

The previous behavior resulted in paths like this:

/etc/prometheus/etc/prometheus/scrape_config_files/scrape_config.yml

When we want this:

/etc/prometheus/scrape_config_files/scrape_config.yml

Configuration example:

rule_files:
  - rule_file.yml
  - /etc/prometheus/rules/rule_file.yml
  - /etc/prometheus/rules/glob/*.yml
scrape_config_files:
  - scrape_config.yml
  - /etc/prometheus/scrape_config_files/scrape_config.yml
  - /etc/prometheus/scrape_config_files/glob/*.yml

Logs of it working as intended:

# prometheus.yml (/etc/prometheus/prometheus.yml)
time=2024-12-13T19:14:22.819Z level=INFO source=main.go:1178 msg="Configuration file change detected, reloading the configuration."
time=2024-12-13T19:14:22.819Z level=INFO source=main.go:1442 msg="Loading configuration file" filename=/etc/prometheus/prometheus.yml
time=2024-12-13T19:14:23.258Z level=INFO source=main.go:1491 msg="Completed loading of configuration file" db_storage=1.71µs remote_storage=435.812µs web_handler=830ns query_engine=19.86µs scrape=18.849832ms scrape_sd=15.784323ms notify=141.87µs notify_sd=10.15µs rules=403.996265ms tracing=38.23µs filename=/etc/prometheus/prometheus.yml totalDuration=439.645443ms

# rules.yml (/etc/prometheus/rules/rule_file.yml)
time=2024-12-13T19:15:53.267Z level=INFO source=main.go:1178 msg="Configuration file change detected, reloading the configuration."
time=2024-12-13T19:15:53.267Z level=INFO source=main.go:1442 msg="Loading configuration file" filename=/etc/prometheus/prometheus.yml
time=2024-12-13T19:15:54.008Z level=INFO source=main.go:1491 msg="Completed loading of configuration file" db_storage=3.07µs remote_storage=576.493µs web_handler=890ns query_engine=22.82µs scrape=27.318201ms scrape_sd=23.386838ms notify=239.861µs notify_sd=17.11µs rules=688.309753ms tracing=15.61µs filename=/etc/prometheus/prometheus.yml totalDuration=740.420487ms


# scrape_config.yml (/etc/prometheus/scrape_config_files/scrape_config.yml)
time=2024-12-13T19:17:24.016Z level=INFO source=main.go:1178 msg="Configuration file change detected, reloading the configuration."
time=2024-12-13T19:17:24.016Z level=INFO source=main.go:1442 msg="Loading configuration file" filename=/etc/prometheus/prometheus.yml
time=2024-12-13T19:17:24.444Z level=INFO source=main.go:1491 msg="Completed loading of configuration file" db_storage=1.79µs remote_storage=367.732µs web_handler=600ns query_engine=18.59µs scrape=17.624679ms scrape_sd=12.13822ms notify=117.53µs notify_sd=9µs rules=397.346967ms tracing=10.78µs filename=/etc/prometheus/prometheus.yml totalDuration=427.959149ms

After collecting that, I do think it could be nice to specify which file was updated instead of only returning filename=/etc/prometheus/prometheus.yml, but that's in reloadConfig and maybe shouldn't be touched in this PR

@cxdy
fix: fails to auto-reload on changes to rule and scrape config files
42cb1dd 
Fixes prometheus#15673

An issue was reported in the CNCF Slack regarding the new auto-reload feature in Prometheus 3.0 failing to reload on changes to rule files and scrape config files.

Signed-off-by: Cody Kaczynski <[email protected]>
@cxdy cxdy force-pushed the fix/autoreload-scrapeconfigs-rules branch from 6ea1aef to 42cb1dd Compare December 13, 2024 18:42
@beorn7 beorn7 requested a review from roidelapluie December 17, 2024 18:50
bboreham
bboreham reviewed Dec 20, 2024
View reviewed changes
Copy link
Member

@bboreham bboreham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not very familiar with this code, but I am left wondering why it calls filepath.Join() rather than config.JoinDir() which is used when reading the config.

config/reload.go
@@ -49,10 50,15 @@ func GenerateChecksum(yamlFilePath string) (string, error) {
dir := filepath.Dir(yamlFilePath)

for i, file := range config.RuleFiles {
config.RuleFiles[i] = filepath.Join(dir, file)
if !strings.Contains(config.RuleFiles[i], dir) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why config.RuleFiles[i] and not file?

Copy link
Author

@cxdy cxdy Dec 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually am not sure what the exact value of file is but I figured it would get us close enough to detect changes if we used the value supplied in the config.

I suppose file might make more sense here though considering globs being used in the config.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You got me thinking, and I'll try it out to see how it works because now I'm concerned that it would reload for every file that had changes (which I didn't test). I really hope it doesn't do that heh

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for config.JoinDir(), I had no idea that was there, so thanks! My modifications were only what we had there. Curious to know what the difference/benefit would be of changing this code to use config.JoinDir() instead.

config/reload.go
for i, file := range config.ScrapeConfigFiles {
config.ScrapeConfigFiles[i] = filepath.Join(dir, file)
if !strings.Contains(config.ScrapeConfigFiles[i], dir) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar: why not file ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bboreham bboreham bboreham left review comments

@roidelapluie roidelapluie Awaiting requested review from roidelapluie

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature flag "auto-reload-config" does not support fileglob references in subdirectory or different directory
2 participants
@cxdy @bboreham