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.

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

fix inconsistent behaviors with prometheus when scraping #5153

Merged
merged 4 commits into from Oct 17, 2023

Conversation

Haleygo
Copy link
Collaborator

@Haleygo Haleygo commented Oct 10, 2023

  1. address vmagent: do not crash on bad scrape config, as promethues does #4959. skip job with wrong syntax in scrape_configs with error logs instead of exiting;
  2. show error messages on vmagent /targets ui if there are wrong auth configs in scrape_configs, previously will print error logs and do scrape without auth header;
  3. don't send requests if there are wrong auth configs in:
    1. vmagent remoteWrite;
    2. vmalert datasource/remoteRead/remoteWrite/notifier.

@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Attention: 126 lines in your changes are missing coverage. Please review.

Comparison is base (c2d252c) 60.19% compared to head (179a3d1) 60.20%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5153   +/-   ##
=======================================
  Coverage   60.19%   60.20%           
=======================================
  Files         398      398           
  Lines       74120    74200   +80     
=======================================
+ Hits        44618    44673   +55     
- Misses      26991    27019   +28     
+ Partials     2511     2508    -3     
Files Coverage Δ
lib/promscrape/config.go 35.16% <100.00%> (+0.26%) ⬆️
lib/promscrape/scrapework.go 57.34% <ø> (ø)
app/vmalert/notifier/alertmanager.go 71.00% <25.00%> (-2.47%) ⬇️
lib/promscrape/discovery/kubernetes/api_watcher.go 63.59% <40.00%> (-0.27%) ⬇️
app/vmalert/datasource/init.go 0.00% <0.00%> (ø)
app/vmalert/remotewrite/client.go 67.01% <0.00%> (-0.71%) ⬇️
lib/promauth/config.go 56.40% <84.44%> (+6.29%) ⬆️
app/vmagent/remotewrite/client.go 0.00% <0.00%> (ø)
lib/promscrape/scraper.go 0.00% <0.00%> (ø)
lib/promscrape/discoveryutils/client.go 25.60% <0.00%> (-0.77%) ⬇️
... and 3 more

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@hagen1778 hagen1778 left a comment

Choose a reason for hiding this comment

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

LGTM

lib/promscrape/discoveryutils/client.go Outdated Show resolved Hide resolved
lib/promscrape/discoveryutils/client.go Outdated Show resolved Hide resolved
app/vmalert/datasource/vm.go Outdated Show resolved Hide resolved
app/vmalert/datasource/vm.go Show resolved Hide resolved
@Haleygo Haleygo force-pushed the fix-get-auth-header-err branch 4 times, most recently from 451bb24 to b1cd497 Compare October 16, 2023 07:55
1. address #4959. skip job with wrong syntax in `scrape_configs` with error logs instead of exiting;
2. show error messages on vmagent /targets ui if there are wrong auth configs in `scrape_configs`, previously will print error logs and do scrape without auth header;
3. don't send requests if there are wrong auth configs in:
    1. vmagent remoteWrite;
    2. vmalert datasource/remoteRead/remoteWrite/notifier.
@Haleygo Haleygo merged commit e16d3f5 into master Oct 17, 2023
9 of 10 checks passed
@Haleygo Haleygo deleted the fix-get-auth-header-err branch October 17, 2023 09:58
@valyala
Copy link
Collaborator

valyala commented Oct 25, 2023

@Haleygo , thanks for the pull request! It looks great overall. I noticed a few issues while reviewing the pull request - some of these issues existed before this pull request. I'd recommend taking a look at the follow-up commit, which addresses these issues - d5a599b . If you notice some issues in this commit, then feel free sending follow-up pull requests on top of this commit!

valyala pushed a commit that referenced this pull request Oct 26, 2023
* fix inconsistent behaviors with prometheus when scraping

1. address #4959. skip job with wrong syntax in `scrape_configs` with error logs instead of exiting;
2. show error messages on vmagent /targets ui if there are wrong auth configs in `scrape_configs`, previously will print error logs and do scrape without auth header;
3. don't send requests if there are wrong auth configs in:
    1. vmagent remoteWrite;
    2. vmalert datasource/remoteRead/remoteWrite/notifier.

* add changelogs

* address review comments

* fix ut
AndrewChubatiuk pushed a commit to AndrewChubatiuk/VictoriaMetrics that referenced this pull request Nov 15, 2023
…rics#5153)

* fix inconsistent behaviors with prometheus when scraping

1. address VictoriaMetrics#4959. skip job with wrong syntax in `scrape_configs` with error logs instead of exiting;
2. show error messages on vmagent /targets ui if there are wrong auth configs in `scrape_configs`, previously will print error logs and do scrape without auth header;
3. don't send requests if there are wrong auth configs in:
    1. vmagent remoteWrite;
    2. vmalert datasource/remoteRead/remoteWrite/notifier.

* add changelogs

* address review comments

* fix ut
@valyala
Copy link
Collaborator

valyala commented Nov 15, 2023

FYI, this pull request has been included in v1.95.0 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants