Add semconv for k8s node condition as metric #2321
Conversation
Signed-off-by: ChrsMark <[email protected]>
lmolkova
moved this from Untriaged
to Awaiting SIG approval
in Semantic Conventions Triage
model/k8s/registry.yaml
Outdated
| brief: | | ||
| The node is healthy and ready to accept pods. and Unknown if the node controller has not heard from the node in the last node-monitor-grace-period" | ||
| stability: development | ||
| - id: ready_unknown |
There was a problem hiding this comment.
This doesn't seem to be listed in node conditions. Is this just because it is the only condition that can be unknown in addition to t/f?
There was a problem hiding this comment.
Right, but we can avoid this by using the k8s.condition.status with values T/F/unknown. Should be fixed by
34f1e08.
Also, note that beyond these listed conditions there might other custom ones defined. So it's safer to have the option to track unknown status properly.
model/k8s/registry.yaml
Outdated
| @@ -434,6 +434,43 @@ groups: | |||
| value: 'terminating' | |||
| brief: "Terminating namespace phase as described by [K8s API](https://pkg.go.dev/k8s.io/[email protected]/core/v1#NamespacePhase)" | |||
| stability: development | |||
| - id: k8s.node.condition | |||
There was a problem hiding this comment.
Did we consider using k8s.condition.type and k8s.condition.status (without node/pod namespace)? I know technically all of the XXXCondition types are separate, but they all follow the same type/status/reason pattern.
There was a problem hiding this comment.
What values would k8s.condition.status and k8s.condition.type accept?
There was a problem hiding this comment.
Status would have T/F/unknown. Type would be open-ended, but we could provide examples from common objects
There was a problem hiding this comment.
I see!
It seems kube-state-metrics follows the same pattern here: https://github.com/kubernetes/kube-state-metrics/blob/v2.15.0/docs/metrics/cluster/node-metrics.md?plain=1#L14
I will change it like this 👍
There was a problem hiding this comment.
Out of curiosity, does KSM have reason as a label? That seems like it would be useful, although it is somewhat higher cardinality
There was a problem hiding this comment.
There are some reason labels for containers' and Pods' statuses but not for Node's conditions: https://github.com/kubernetes/kube-state-metrics/blob/v2.15.0/docs/metrics/workload/pod-metrics.md
For containers' status we have it already in SemConv: https://github.com/open-telemetry/semantic-conventions/blob/main/model/k8s/metrics.yaml#L80
There was a problem hiding this comment.
I would lean towards adding it, but if we are unsure, we can always do that later.
There was a problem hiding this comment.
Good idea! I have added it.
The modeling of this metric is becoming a bit tricky because we have to ensure that metric's name does not collide with a namespace. k8s.node.condition cannot be the metric name and a namespace for the attributes at the same time. Let me know what you think about the current suggestion.
Signed-off-by: ChrsMark <[email protected]>
Signed-off-by: ChrsMark <[email protected]>
Fixes #2077
Changes
This PR adds the
k8s.node.conditionmetricMerge requirement checklist
[chore]