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

[receiver/k8sclusterreceiver]: add k8s.pod.ready metric #32933

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gdvalle
Copy link

@gdvalle gdvalle commented May 7, 2024

Description:
Introduce a new metric, k8s.pod.ready, to go alongside k8s.container.ready. #32941

This is helpful to represent, as k8s.container.ready alone doesn't tell the full picture of whether a pod's ready for service, due to pod ReadinessGates.

With this, we don't need to rely on kube-state-metrics for the info.

This does the simple thing and adds an analog for the pod ready condition, but I could also see a more generalized version, i.e. k8s.pod.status.condition, where we could capture all conditions (API docs), e.g. 0=False, 1=True, 2=Unknown, along with reason in attributes. Maybe message too, if cardinality potential isn't a concern here.

Testing: A couple unit tests were added to assert ready pod condition is correctly recorded for both true and false states.

Documentation: Only auto-generated docs.

@povilasv
Copy link
Contributor

povilasv commented May 8, 2024

Hey, please create an issue, attach the issue to description and changelog entry.

@gdvalle
Copy link
Author

gdvalle commented May 8, 2024

Hey, please create an issue, attach the issue to description and changelog entry.

Done, see #32941

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label May 23, 2024
@gdvalle
Copy link
Author

gdvalle commented Jun 5, 2024

not stale

@github-actions github-actions bot removed the Stale label Jun 6, 2024
@@ -287,6 +287,13 @@ metrics:
gauge:
value_type: int

k8s.pod.ready:
enabled: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think maybe this should be disabled by default?

I'm a bit worried It would add quite a bit of new metrics 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd argue that pod ready state is pretty fundamental to k8s, and it should be part of the default set. If k8s.container.ready is, then this should be too. I'd think for most workloads pod ready state is more important at a high level than container, as it affects whether a Service directs traffic to it. It's also part of the default set in KSM.

That said, I'm happy to flip it if that's what it takes to get this over the line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants