-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
ISSUE-1465: Added kube_pod_status_ready_time and kube_pod_status_containers_ready_time #1482
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Welcome @sgrzemski! |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sgrzemski The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@@ -9,6 +9,8 @@ | |||
| kube_pod_labels | Gauge | Kubernetes labels converted to Prometheus labels | | `pod`=<pod-name> <br> `namespace`=<pod-namespace> <br> `label_POD_LABEL`=<POD_LABEL> <br> `uid`=<pod-uid> | STABLE | | |||
| kube_pod_status_phase | Gauge | The pods current phase | | `pod`=<pod-name> <br> `namespace`=<pod-namespace> <br> `phase`=<Pending\|Running\|Succeeded\|Failed\|Unknown> <br> `uid`=<pod-uid> | STABLE | | |||
| kube_pod_status_ready | Gauge | Describes whether the pod is ready to serve requests | | `pod`=<pod-name> <br> `namespace`=<pod-namespace> <br> `condition`=<true\|false\|unknown> <br> `uid`=<pod-uid> | STABLE | | |||
| kube_pod_status_ready_time | Gauge | Time when pod passed readiness probes. | seconds | `pod`=<pod-name> <br> `namespace`=<pod-namespace> <br> `uid`=<pod-uid> | STABLE | | |||
| kube_pod_status_containers_ready_time | Gauge | Time when pod containers entered Ready state. | seconds | `pod`=<pod-name> <br> `namespace`=<pod-namespace> <br> `uid`=<pod-uid> | STABLE | |
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.
| kube_pod_status_containers_ready_time | Gauge | Time when pod containers entered Ready state. | seconds | `pod`=<pod-name> <br> `namespace`=<pod-namespace> <br> `uid`=<pod-uid> | STABLE | | |
| kube_pod_status_container_ready_time | Gauge | Time when the container of the pod entered Ready state. | seconds | `pod`=<pod-name> <br> `namespace`=<pod-namespace> <br> `uid`=<pod-uid> | STABLE | |
Would suggest to use singular here, since we already have kube_pod_container_info
and kube_pod_container_status_waiting
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.
Hello @mrueg,
The k8s documentation specifies this particular condition as
ContainersReady: all containers in the Pod are ready.
, hence the plural. Due to the fact there can be more than 1 containers in a pod and this condition turns into true only when all pods are ready, I think we should stick with plural. What do you think? If you still want to stick with singular that's fine for me as well, but I found it a little bit confusing.
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.
Thanks, good point! I'll defer to @lilic @brancz and @tariq1890 if we want to stick closer to k8s or not
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.
@sgrzemski can you also sign the linuxfoundation/cla? |
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.
Couple of comments. Thanks for the PR!
What is the cardinality of these new metrics?
@@ -1362,6 +1364,32 @@ func createPodStartTimeFamilyGenerator() generator.FamilyGenerator { | |||
) | |||
} | |||
|
|||
func createPodStatusContainersReadyTimeFamilyGenerator() generator.FamilyGenerator { |
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.
Could you add a test for both of these, thank you!
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.
Added, thank you!
@@ -9,6 +9,8 @@ | |||
| kube_pod_labels | Gauge | Kubernetes labels converted to Prometheus labels | | `pod`=<pod-name> <br> `namespace`=<pod-namespace> <br> `label_POD_LABEL`=<POD_LABEL> <br> `uid`=<pod-uid> | STABLE | | |||
| kube_pod_status_phase | Gauge | The pods current phase | | `pod`=<pod-name> <br> `namespace`=<pod-namespace> <br> `phase`=<Pending\|Running\|Succeeded\|Failed\|Unknown> <br> `uid`=<pod-uid> | STABLE | | |||
| kube_pod_status_ready | Gauge | Describes whether the pod is ready to serve requests | | `pod`=<pod-name> <br> `namespace`=<pod-namespace> <br> `condition`=<true\|false\|unknown> <br> `uid`=<pod-uid> | STABLE | | |||
| kube_pod_status_ready_time | Gauge | Time when pod passed readiness probes. | seconds | `pod`=<pod-name> <br> `namespace`=<pod-namespace> <br> `uid`=<pod-uid> | STABLE | | |||
| kube_pod_status_containers_ready_time | Gauge | Time when pod containers entered Ready state. | seconds | `pod`=<pod-name> <br> `namespace`=<pod-namespace> <br> `uid`=<pod-uid> | STABLE | |
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.
Done. Thank you for pointing that out, my initial try was unsuccessful. I've retried today and succeeded. |
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
@sgrzemski: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
@k8s-triage-robot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Hi @sgrzemski - this would be really useful for my organization. Is there any chance you can get this across the line? If not I'd be happy to take over the PR and get it merged. |
@mrueg could you maybe have a look on it and merge, as it's still needed. |
It seems like all that's needed is an opinion from @brancz about: |
@k8s-ci-robot / @mrueg and Team, |
What this PR does / why we need it:
Adds
kube_pod_status_ready_time
andkube_pod_status_containers_ready_time
metrics to get the information provided by Pod Lifecycle's PodConditions array.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 #1465