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

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

yeya24
yeya24 reviewed Aug 12, 2022
View reviewed changes
cmd/thanos/config.go Outdated
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

cmd/thanos/config.go Outdated
"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

cmd/thanos/config.go Outdated
@@ -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

yeya24
yeya24 reviewed Aug 13, 2022
View reviewed changes
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.

docs/components/sidecar.md Outdated
@@ -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!

@zvlb
Add args for Thanos sidecar
90f44c4 
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
yeya24
yeya24 approved these changes Sep 1, 2022
View reviewed changes
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
@zvlb @prajain12
Add args for Thanos sidecar (thanos-io#5573)
1153765 
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
@zvlb @prajain12
Add args for Thanos sidecar (thanos-io#5573)
115ed03 
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
@viennaa
add thanos config args
b258cbb 
Introducing missing arguments for thanos config interval and timeout.
thanos-io/thanos#5573
@viennaa viennaa mentioned this pull request Mar 8, 2023
Merged
5 tasks
viennaa added a commit to viennaa/prometheus-operator that referenced this pull request Mar 8, 2023
@viennaa
add thanos config args
018eadd 
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
@viennaa @simonpasquier
add thanos config_timeout and config_interval args (#5399)
e2ce57a 
* 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
Reviewers

@yeya24 yeya24 yeya24 approved these changes

Assignees
No one assigned
Labels
size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zvlb @yeya24 @GiedriusS