-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add metrics for config file changes #1916
Conversation
471fc16
to
7a56756
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These metrics will be very useful, thanks Manuel!
7a56756
to
e3a1593
Compare
pkg/app/server.go
Outdated
}) | ||
configSuccess := promauto.With(ksmMetricsRegistry).NewGauge( | ||
prometheus.GaugeOpts{ | ||
Name: "kube_state_metrics_config_last_reload_successful", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer kube_state_metrics_last_config_reload_successful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to agree, in future we might have other config files as well though, that's why I added the infix.
Another config we might want to create metrics for once the hot reload for it is in place as well:
kube_state_metrics_customresourcestateconfig_last_reload_successful
Alternatively we could drop it from the name and use:
kube_state_metrics_last_reload_successful{type="config"}
kube_state_metrics_last_reload_successful{type="customresourcestateconfig"}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd vote for using labels, seems to reduce verbosity and users can also monitor all config updates through a single metric.
pkg/app/server.go
Outdated
}) | ||
configSuccessTime := promauto.With(ksmMetricsRegistry).NewGauge( | ||
prometheus.GaugeOpts{ | ||
Name: "kube_state_metrics_config_last_reload_success_timestamp_seconds", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about kube_state_metrics_last_config_uptime
?
So that we can easily know whether there are some changes to config recently.
It equals to current_timestamp - kube_state_metrics_config_last_reload_success_timestamp_seconds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to keep it as it, as a timestamp over a counter has a couple of advantages:
- few static values inside the TSDB for this metric
- Resets can also be detected if the timestamp changes
- You never have the same counter twice.
See also: https://prometheus.io/docs/practices/instrumentation/#timestamps-not-time-since
e3a1593
to
b2682b8
Compare
So this is how metrics currently look like. Any thoughts on the config hash? We could make the value static (=1) and add it as a label instead. |
Seems fine to me to keep it as the value. |
/triage accepted |
/lgtm |
b2682b8
to
792c9a3
Compare
This uses code pieces from prometheus/alertmanager in https://github.com/prometheus/alertmanager/blob/main/config/coordinator.go#LL56C26-L56C26 licensed under Apache-2.0. kube_state_metrics_config_hash{type="config", filename="config.yml"} 4.0061079457904e+13 kube_state_metrics_config_last_reload_success_timestamp_seconds{type="config", filename="config.yml"} 1.6697483049487052e+09 kube_state_metrics_config_last_reload_successful{type="config", filename="config.yml"} 1 Signed-off-by: Manuel Rüger <manuel@rueg.eu>
792c9a3
to
ee89176
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fpetkovski, mrueg, rexagod The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold cancel Let's proceed here |
What this PR does / why we need it:
Provides metrics for config file updates when viper picks up a new config.
This uses code pieces from prometheus/alertmanager in https://github.com/prometheus/alertmanager/blob/main/config/coordinator.go#LL56C26-L56C26 licensed under Apache-2.0.
kube_state_metrics_config_hash 4.0061079457904e+13 kube_state_metrics_config_last_reload_success_timestamp_seconds 1.6697483049487052e+09 kube_state_metrics_config_last_reload_successful 1
How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)
Adds three metric series to the ksm exporter, not the kubernetes exporter.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #1893