-
Notifications
You must be signed in to change notification settings - Fork 599
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
Updated rules using deprecated metrics with new versions. #556
base: master
Are you sure you want to change the base?
Updated rules using deprecated metrics with new versions. #556
Conversation
Signed-off-by: GitHub <[email protected]>
rules/apps.libsonnet
Outdated
@@ -64,7 +68,7 @@ | |||
sum by (namespace) ( | |||
sum by (namespace, pod) ( | |||
max by (namespace, pod, container) ( | |||
kube_pod_container_resource_requests_memory_bytes{%(kubeStateMetricsSelector)s} | |||
kube_pod_container_resource_requests{%(kubeStateMetricsSelector)s, %(memorySelector)s, %(byteSelector)s} |
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.
Any reason why we don't just include the memorySelector
and byteSelector
directly in this query? We try to keep configuration minimal if there aren't really any other ways to configure something.
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.
We can. I thought this was the preferred way of doing it. I'll change that.
Signed-off-by: GitHub <[email protected]>
CI is failing, looks like our tests need some changes to the inputs since we're using different metrics with these changes. I think you should be fine just adapting the inputs of this test: Lines 113 to 156 in 3152600
|
Signed-off-by: GitHub <[email protected]>
Since these metrics have existed pretty much forever in kube-state-metrics, I'm happy with moving ahead right away with this. Wdyt @paulfantom ? |
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.
We have similar PR in #534 and the result of discussion from it was to wait for a stable KSM release. Without such release, this PR is breaking compatibility for all users, and merging it will result in requiring mixed metrics coming from KSM v1 and v2.
I wouldn't have much objections if there would be any guarantee that stable KSM v2 would be released soon. However, as it is right now, the first alpha release of kube-state-metrics was on 2020-09-16 and this doesn't reassure me that stable release is going to be released in the next days.
I'm not sure I understand as the metrics as they are used in this PR have been around for years in kube-state-metric 1.x, only the ones currently used in the mixin are being dropped in 2.0. Am I missing something else? |
Refer this changelog: https://github.com/kubernetes/kube-state-metrics/blob/33db2356bf1f0a1f51ddaaeb165bce04ab5aa0df/CHANGELOG.md#v200-alpha--2020-09-16
kube_pod_container_resource_requests_cpu_cores
andkube_pod_container_resource_requests_memory_bytes
are deprecated.