-
Notifications
You must be signed in to change notification settings - Fork 39.3k
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
Change kubelet metrics to conform metrics guidelines #72470
Conversation
/assign @brancz |
Could you add the deprecation hint in the description here as well? Thanks! |
@brancz Added Deprecated in metrics description. PTAL |
/retest |
4 similar comments
/retest |
/retest |
/retest |
/retest |
/lgtm |
@derekwaynecarr can you help review this? Thanks |
@derekwaynecarr @vishh ping |
@thockin @derekwaynecarr @vishh can you help review this? Thanks |
rebased. |
/retest |
1 similar comment
/retest |
I believe this fixes #66791. |
PR rebased. @tallclair : could you help review it ? thanks |
pkg/kubelet/metrics/metrics.go
Outdated
KubeletSubsystem = "kubelet" | ||
NodeNameKey = "node_name" | ||
NodeLabelKey = "node" | ||
PodWorkerLatencyKey = "pod_worker_latency_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 just noticed this on another PR, these should all be duration
instead of latency
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.
Does this looks good ? 79a3eb8
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.
So we should do this change to all other metrics ?
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.
Yes we should.
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.
sure, I will change them.
Thanks for the quick update! /lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danielqsj, smarterclayton 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 |
/retest Review the full test history for this PR. Silence the bot with an |
@brancz this is probably worth adding to https://github.com/kubernetes/community/blob/81ec4af0ed02b4c5c0917a16563250b2f45250c2/contributors/devel/sig-instrumentation/instrumentation.md#naming |
@mikedanese You're totally right. There is this already, but we don't link to it yet. Let me fix that. |
What type of PR is this?
/kind feature
/sig instrumentation
What this PR does / why we need it:
As part of kubernetes metrics overhaul, change kubelet metrics to conform Kubernetes metrics instrumentation guidelines.
Change kubelet metrics to histogram for better aggregation.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
This patch does not remove the existing metrics but mark them as deprecated.
We need 2 releases for users to convert monitoring configuration.
Does this PR introduce a user-facing change?: