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

Set --metrics-max-age default value equal to relist interval #230

Merged
merged 2 commits into from
Sep 4, 2020

Conversation

weibeld
Copy link
Contributor

@weibeld weibeld commented Aug 14, 2019

The--metrics-max-age flag has been introduced in #136 with a fixed default value of 20 minutes.

The current default value leads to the situation that when metrics drop out of Prometheus, they are still reported in the discovery information of the Custom Metrics API (/apis/custom.metrics.k8s.io/v1beta1) for 20 minutes.

This can be quite confusing if you don't know about the --metrics-max-age flag and expect the behaviour that is described in the docs. Namely that the Prometheus Adapter discovers all metrics that have been updated since the last discovery query (defined by the relist interval), which is also the reason the relist interval must be at least as large as the Prometheus scraping interval (see #229).

By setting the default value of --metrics-max-age equal to the relist interval, the expected behaviour as described in the docs is preserved, but you can still set --metrics-max-age to some other value if you know about it.

The other way to go would be to update the docs and expect all users to set the --metrics-max-age flag (and the Helm chart would need to be updated to use it too). But this would make the configuration of the Prometheus Adapter even more complicated than it already is. It is probably easier to just remember to set the relist interval at least as large as the Prometheus scraping interval, than to additionally have to figure out what the metrics-max-age is and what to set it to.

@s-urbaniak
Copy link
Contributor

Despite the age of the PR, I think this is still reasonable, hence merging.

@s-urbaniak s-urbaniak merged commit 2678f90 into kubernetes-sigs:master Sep 4, 2020
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.

2 participants