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

sidecar: Add args prometheus.get_config_interval and prometheus.timeout_get_config #5573

Merged
merged 2 commits into from
Sep 1, 2022
Merged

sidecar: Add args prometheus.get_config_interval and prometheus.timeout_get_config #5573

merged 2 commits into from
Sep 1, 2022

Conversation

zvlb
Copy link
Contributor

@zvlb zvlb commented Aug 5, 2022

Signed-off-by: Zemtsov Vladimir vl.zemtsov@gmail.com

Changes

Add 2 args for Thanos sidecar:
prometheus.get_config_interval - How often the Prometheus config. Default - 30s
prometheus.timeout_get_config -Timeout for get Prometheus config. Default - 5s

Add docs for this changes

This needed, bc:

  1. HardCode - it's bad
  2. If u have really big Prometheus installation, u don't want make many config-requests

@yeya24
Copy link
Contributor

yeya24 commented Aug 6, 2022

If u have really big Prometheus installation, u don't want make many config-requests

The config request is pretty lightweight to Prometheus I guess. Have you seen a performance issue on this? Or do you have any other use cases?

@zvlb
Copy link
Contributor Author

zvlb commented Aug 9, 2022

Simular issue - #4121

In My installation Prometheus I have more then 1000 ServiceMonitors, and very big Prometheus installation (many shards).

Sometimes prometheus answered on requests /api/v1/status/config more then 5s

cmd.Flag("prometheus.get_config_interval",
"How often the Prometheus config").
Default("30s").DurationVar(&pc.getConfigInterval)
cmd.Flag("prometheus.timeout_get_config",
Copy link
Contributor

Choose a reason for hiding this comment

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

rename it to prometheus.get_config_timeout

"How often the Prometheus config").
Default("30s").DurationVar(&pc.getConfigInterval)
cmd.Flag("prometheus.timeout_get_config",
"Timeout for get Prometheus config").
Copy link
Contributor

Choose a reason for hiding this comment

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

for getting

@@ -75,6 +77,12 @@ func (pc *prometheusConfig) registerFlag(cmd extkingpin.FlagClause) *prometheusC
cmd.Flag("prometheus.ready_timeout",
"Maximum time to wait for the Prometheus instance to start up").
Default("10m").DurationVar(&pc.readyTimeout)
cmd.Flag("prometheus.get_config_interval",
"How often the Prometheus config").
Copy link
Contributor

Choose a reason for hiding this comment

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

How often to get Prometheus config

Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Also please add a changelog as well.

@@ -136,6 +138,8 @@ Flags:
--prometheus.ready_timeout=10m
Maximum time to wait for the Prometheus
instance to start up
--prometheus.timeout_get_config=5s
Timeout for get Prometheus config
Copy link
Contributor

Choose a reason for hiding this comment

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

Please regenerate the docs by make docs.

@zvlb
Copy link
Contributor Author

zvlb commented Aug 26, 2022

@yeya24 Hi
Need add something else?

@yeya24
Copy link
Contributor

yeya24 commented Aug 30, 2022

Please rebase main and update the change log then we can merge. Thanks!

Signed-off-by: Zemtsov Vladimir <vl.zemtsov@gmail.com>
@zvlb
Copy link
Contributor Author

zvlb commented Aug 31, 2022

Done

@yeya24 yeya24 enabled auto-merge (squash) September 1, 2022 00:35
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

LGTM

@yeya24 yeya24 merged commit 06a64d6 into thanos-io:main Sep 1, 2022
@GiedriusS
Copy link
Member

(1) is not always bad, we are trying hard (and it's a really hard problem) to not have many different options because then it becomes really confusing (: but if this solves a practical problem then it looks good!

prajain12 pushed a commit to prajain12/thanos that referenced this pull request Sep 6, 2022
Signed-off-by: Zemtsov Vladimir <vl.zemtsov@gmail.com>

Signed-off-by: Zemtsov Vladimir <vl.zemtsov@gmail.com>
Signed-off-by: Prakul Jain <prakul.jain@udaan.com>
prajain12 pushed a commit to prajain12/thanos that referenced this pull request Sep 6, 2022
Signed-off-by: Zemtsov Vladimir <vl.zemtsov@gmail.com>

Signed-off-by: Zemtsov Vladimir <vl.zemtsov@gmail.com>
Signed-off-by: Prakul Jain <prakul.jain@udaan.com>
viennaa added a commit to viennaa/prometheus-operator that referenced this pull request Mar 8, 2023
Introducing missing arguments for thanos config interval and timeout.
thanos-io/thanos#5573
viennaa added a commit to viennaa/prometheus-operator that referenced this pull request Mar 8, 2023
Introducing missing arguments for thanos config interval and timeout.
thanos-io/thanos#5573
simonpasquier added a commit to prometheus-operator/prometheus-operator that referenced this pull request Mar 13, 2023
* add thanos config args

Introducing missing arguments for thanos config interval and timeout.
thanos-io/thanos#5573

Co-authored-by: Simon Pasquier <spasquie@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants