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

vmagent doesn't follow redirects without stream_parse #1945

Closed
vbichov opened this issue Dec 14, 2021 · 21 comments
Closed

vmagent doesn't follow redirects without stream_parse #1945

vbichov opened this issue Dec 14, 2021 · 21 comments
Labels
bug Something isn't working vmagent

Comments

@vbichov
Copy link

vbichov commented Dec 14, 2021

Describe the bug
vmagent doesn't follow redirects without setting stream_parse: true
for details see slack thread:
https://victoriametrics.slack.com/archives/CGZF1H6L9/p1639404018163100

Expected behavior
it should follow redirects in a way that is similar to prometheus

VM agent config

global:
  scrape_interval: 1m0s
scrape_configs:
- job_name: emr
  follow_redirects: true
  relabel_configs:
  - source_labels: [__meta_ec2_private_ip]
    target_label: __metrics_path__
    replacement: /bi-emr-reverse-logs/$1:4040/metrics/executors/prometheus/
  - source_labels: [__meta_ec2_tag_Name]
    target_label: instance
  - target_label: __address__
    replacement: bo.XX.com
  ec2_sd_configs:
  - region: us-east-1
    access_key: XXX
    secret_key: <secret>
    filters:
    - name: tag:aws:elasticmapreduce:instance-group-role
      values:
      - MASTER
    - name: tag:Name
      values:
      - airflow-shared-persistent-emr-6.3.0

Version
tried v1.7.0 and v1.68.0 and latest docker version


**Logs**
`unexpected status code returned when scraping "http://bo.XXX.com:80/bi-emr-reverse-logs/10.207.109.205:4040/metrics/executors/prometheus/": 301; expecting 200; response body: "<html>\r\n<head><title>301 Moved Permanently</title></head>\r\n<body>\r\n<center><h1>301 Moved Permanently</h1></center>\r\n<hr><center>openresty/1.19.3.1</center>\r\n</body>\r\n</html>\r\n"`

**Workaround**
the issue was resolved by adding stream_parse: true
@valyala
Copy link
Collaborator

valyala commented Dec 15, 2021

vmagent was following only a single redirect up to v1.70.0 . It should follow up to 5 redirects when built from the commit a3adf24 . @vbichov , could you build vmagent from this commit according to these docs and verify whether this allows scraping targets without stream_parse: true mode?

Note that vmagent expects that redirects are performed to the original hostname when stream_parse: true isn't set in scrape config.

@valyala
Copy link
Collaborator

valyala commented Dec 20, 2021

vmagent should follow for up to 5 redirects starting from v1.71.0. @vbichov , could you check whether this helps resolving the issue?

@valyala
Copy link
Collaborator

valyala commented Feb 12, 2022

@vbichov , could you check whether the issue persists in vmagent v1.72.0?

@earendilfr
Copy link

Hello,

For information, I still have the issue with vmagent v1.79.5...

2022-12-14T09:46:18.261Z        warn    VictoriaMetrics/lib/promscrape/scrapework.go:385        cannot scrape "http://xxxxxxx:80/snmp?module=icx&target=xxxxxxx" (job "Switches-foundry", labels {datacenter="XX",env="XX",instance="XX",job="Switches-foundry",monitor="prometheus04",site="XX"}): error when scraping "http://xxxxxxx:80/snmp?module=icx&target=xxxxxxx": too many redirects
2022-12-14T09:46:18.280Z        warn    VictoriaMetrics/lib/promscrape/scrapework.go:385        cannot scrape "http://xxxxxxx:80/probe?module=icmp&target=xxxxxxx" (job "Switches-ping", labels {datacenter="XX",env="XX",instance="XX",job="Switches-ping",monitor="prometheus04",site="XX"}): error when scraping "http://xxxxxxx:80/probe?module=icmp&target=xxxxxxx": too many redirects
2022-12-14T09:46:18.379Z        warn    VictoriaMetrics/lib/promscrape/scrapework.go:385        cannot scrape "http://xxxxxxx:80/probe?module=icmp&target=xxxxxxx" (job "Routers-ping", labels {datacenter="XX",env="XX",instance="XX",job="Routers-ping",monitor="prometheus04",site="XX"}): error when scraping "http://xxxxxxx:80/probe?module=icmp&target=xxxxxxx": too many redirects

@hagen1778
Copy link
Collaborator

@earendilfr do you know how many redirects are there? It could be checked via browser...

@earendilfr
Copy link

@earendilfr do you know how many redirects are there? It could be checked via browser...

@hagen1778 : sorry, I have changed of jobs so hard to me to have this information but, if I remember correctly, I have only one redirect because, in this case, it's was just a redirect from http to https on the same server.

MatthewJohn added a commit to MatthewJohn/vault-nomad-consul-terraform that referenced this issue Jun 10, 2023
@yrka5180
Copy link
Contributor

@earendilfr do you know how many redirects are there? It could be checked via browser...

@hagen1778 : sorry, I have changed of jobs so hard to me to have this information but, if I remember correctly, I have only one redirect because, in this case, it's was just a redirect from http to https on the same server.

hi, have you solved this?
I am facing the same problem that it shows too many redirect while directing from http to https.

@hagen1778
Copy link
Collaborator

@yrka5180 have you tried enabling/disabling stream parse?

@jnadler
Copy link

jnadler commented Aug 21, 2023

we just ran into this same issue: a single hop redirect http -> https fails with too many redirects

we enabled streamParse for all of vmagent via arguments, but i don't feel we should have to do this

and after enabling streamParse we see some new skipping duplicate scrape target with identical labels errors for some of our targets; does streamParse change how relabelling works?

@hagen1778
Copy link
Collaborator

we just ran into this same issue: a single hop redirect http -> https fails with too many redirects

Is it possible to compile docker-compose env where this issue can be reproduced? This would simplify debug from our side.

and after enabling streamParse we see some new skipping duplicate scrape target with identical labels errors for some of our targets; does streamParse change how relabelling works?

Not that I'm aware of. Have you checked https://docs.victoriametrics.com/vmagent.html#troubleshooting ?

@jnadler
Copy link

jnadler commented Aug 25, 2023

I just turned off streamParse and confirmed we're still facing this problem. Here's what the redirect looks like:

Screenshot 2023-08-25 at 9 40 09 AM

The metrics target is cockroachDB. I'll see if I can reproduce in docker-compose or even better, narrow down the problem in the code.

@jnadler
Copy link

jnadler commented Aug 25, 2023

After reading some code, perhaps this behavior is unfortunate but expected.

// It is expected that the redirect is made on the same host.

I expect that http->https qualifies as changing the host, but fasthttp is reusing the existing connection to port 80, so of course this will be a redirect loop as the redirect was not followed at all.

@jnadler
Copy link

jnadler commented Aug 25, 2023

I believe it will be possible to fix this case, but @valyala may not love it

If we detect this case on redirect in promscrape client.go and create new c.hc = &fasthttp.HostClient{... with IsTLS now set appropriately, it will work thereafter.

@jnadler
Copy link

jnadler commented Aug 25, 2023

Here's a PR (the code is awful! it's just to communicate the idea) that shows it is possible to fix this behavior #4897

@hagen1778
Copy link
Collaborator

I believe, the issue would be fixed if we completely migrate vmagent to native HTTP lib. @zekker6 I recall you was working on that some time ago... wdyt?

@zekker6
Copy link
Contributor

zekker6 commented Aug 29, 2023

@hagen1778 When I've been testing this native HTTP lib was consuming more memory than fasthttp, and we decided to keep it as is in order to avoid increasing resource usage during scrapes.
It seems to me like it will be better to add a logic to re-create fasthttp.HostClient in case of redirects as it will create much smaller memory footprint than switching to native HTTP lib.

@jnadler
Copy link

jnadler commented Aug 29, 2023

Agreed, moving away from fasthttp would probably hurt performance in a way that might surprise users. This change gets it a little closer to the normal behavior expected from the native client.

Note that in my example here I'm only handling http->https redirects but "different hostname" redirects could also be handled with the same approach.

If you'd like me to do any more work down this road, or clean up this code a bit, just let me know! I factored the creation of the fasthttp client out of the new method, and could do the same for the streamparse client for consistency (not that there's any reason to recreate that one at runtime).

zekker6 added a commit that referenced this issue Aug 31, 2023
This will allow to handle redirect easily as fasthttp.Client creates new fasthttp.HostClient on demand and caches created instances per URL.

See: #1945
Signed-off-by: Zakhar Bessarab <z.bessarab@victoriametrics.com>
@ndevenish
Copy link

I ran into this with federation targets. This specifically seems to be a problem because it looks like prometheus doesn't let you write https:// in the target, but VM defaults to http and fails on the redirect.

I think this means that you can't write a completely compatible configuration that works on both (this is probably only an issue while transitioning)

@ptimofee
Copy link

ptimofee commented Dec 4, 2023

I can confirm that at least vmagent v1.93.4 does follow redirects only when stream_parse is true, no matter if follow_redirects is set to true or false. I. e. follow_redirects has no effect at all.

valyala added a commit that referenced this issue Jan 30, 2024
…Client for scraping targets in non-streaming mode

While fasthttp.Client uses less CPU and RAM when scraping targets with small responses (up to 10K metrics),
it doesn't work well when scraping targets with big responses such as kube-state-metrics.
In this case it could use big amounts of additional memory comparing to net/http.Client,
since fasthttp.Client reads the full response in memory and then tries re-using the large buffer
for further scrapes.

Additionally, fasthttp.Client-based scraping had various issues with proxying, redirects
and scrape timeouts like the following ones:

- #1945
- #5425
- #2794
- #1017

This should help reducing memory usage for the case when target returns big response
and this response is scraped by fasthttp.Client at first before switching to stream parsing mode
for subsequent scrapes. Now the switch to stream parsing mode is performed on the first scrape
after reading the response body in memory and noticing that its size exceeds the value passed
to -promscrape.minResponseSizeForStreamParse command-line flag.
Updates #5567

Overrides #4931
valyala added a commit that referenced this issue Jan 30, 2024
…Client for scraping targets in non-streaming mode

While fasthttp.Client uses less CPU and RAM when scraping targets with small responses (up to 10K metrics),
it doesn't work well when scraping targets with big responses such as kube-state-metrics.
In this case it could use big amounts of additional memory comparing to net/http.Client,
since fasthttp.Client reads the full response in memory and then tries re-using the large buffer
for further scrapes.

Additionally, fasthttp.Client-based scraping had various issues with proxying, redirects
and scrape timeouts like the following ones:

- #1945
- #5425
- #2794
- #1017

This should help reducing memory usage for the case when target returns big response
and this response is scraped by fasthttp.Client at first before switching to stream parsing mode
for subsequent scrapes. Now the switch to stream parsing mode is performed on the first scrape
after reading the response body in memory and noticing that its size exceeds the value passed
to -promscrape.minResponseSizeForStreamParse command-line flag.
Updates #5567

Overrides #4931
@valyala
Copy link
Collaborator

valyala commented Jan 30, 2024

FYI, the commit bc7cf49 should fix following redirects in stream parsing mode when the redirect is performed to different hostname. This commit will be included in the next release. In the mean time it is possible to build vmagent from this commit according to these docs and verify whether follow_redirects: true option in scrape_configs works as intended.

@valyala
Copy link
Collaborator

valyala commented Jan 30, 2024

vmagent should properly follow redirects at scrape targets when stream parsing mode is disabled starting from v1.97.0. Closing the issue as fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working vmagent
Projects
None yet
Development

No branches or pull requests

10 participants