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

serverset_sd_configs is only scraping the first configured path #1137

Closed
StephanErb opened this Issue Oct 2, 2015 · 7 comments

Comments

Projects
None yet
3 participants
@StephanErb
Copy link
Contributor

StephanErb commented Oct 2, 2015

I am running Prometheus 0.16.0rc2 and have the following config:

  - job_name: 'aurora'
    metrics_path: /probe
    serverset_sd_configs:
    - servers:
      - 'one.zk.server:2181'
      - 'two.zk.server:2181'
      - 'three.zk.server:2181'
      paths:
      - '/aurora/myrole/test/service-1'
      - '/aurora/myrole/test/service-2'
    relabel_configs:
      ... 
  • Current behaviour: Prometheus is only scraping one entry in the paths list. Whatever is listed first is scraped, all others are ignored. Multiple serversets are only scraped if one is specifying a root entry such as /aurora.
  • Expected behaviour: All listed paths are scraped.

I believe this code here might be the culprint

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented Oct 6, 2015

@brian-brazil serversets are your area of expertise I guess :)

@fabxc fabxc added the bug label Oct 6, 2015

@fabxc fabxc added this to the v0.16.0 milestone Oct 6, 2015

@juliusv

This comment has been minimized.

Copy link
Member

juliusv commented Oct 6, 2015

I have zero knowledge about Zookeeper or serversets, but this line stood out to me very obviously:

https://github.com/prometheus/prometheus/blob/master/retrieval/discovery/serverset.go#L90

sd.treeCache = newZookeeperTreeCache(conn, conf.Paths[0], updates)

I guess that's the bug and we just need one tree cache per watched path?

@juliusv

This comment has been minimized.

Copy link
Member

juliusv commented Oct 6, 2015

Without reading the code much further, does this branch work?

https://github.com/prometheus/prometheus/commits/fix-serverset-multiple-paths

See commit 62a6871

@StephanErb

This comment has been minimized.

Copy link
Contributor Author

StephanErb commented Oct 6, 2015

Looks valid. I will give it a test run and report back here.

@juliusv

This comment has been minimized.

Copy link
Member

juliusv commented Oct 6, 2015

Thanks! It's now out in a PR as well: #1148

@juliusv

This comment has been minimized.

Copy link
Member

juliusv commented Oct 8, 2015

@StephanErb Any news on this one?

@lock

This comment has been minimized.

Copy link

lock bot commented Mar 24, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Mar 24, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.