open-telemetry / opentelemetry-collector-contrib

Contrib repository for the OpenTelemetry Collector
https://opentelemetry.io
Apache License 2.0
3.13k stars 2.39k forks source link

Record pod Ready status, or all pod status conditions #32941

Open gdvalle opened 6 months ago

gdvalle commented 6 months ago

Component(s)

receiver/k8scluster

Is your feature request related to a problem? Please describe.

I would like to be able to see a pod's Ready status in metrics.

Describe the solution you'd like

A named metric with an analog to k8s.container.ready ,like k8s.pod.ready.

Or, a more generic solution where all pod status conditions are represented, i.e. k8s.pod.status.condition. I might prefer this as it captures more states.

I would like to include attributes for the detail info fields (message, reason), for either approach.

Describe alternatives you've considered

Using the existing k8s.container.ready metric. It's unfortunately not complete because of pod ReadinessGates.

Additional context

See https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/32933 for an impl of k8s.pod.ready, currently sans attributes.

If we can agree, I think the condition status metric makes more sense. I think we need agreement on how to represent it: other things in this receiver (e.g. k8s.pod.phase) use values to represent states, so 0=false, 1=true, 2=unknown could fit, but the ergonomics are a bit poor.

github-actions[bot] commented 6 months ago

Pinging code owners:

TylerHelmuth commented 6 months ago

This feels reasonable, especially if there is existing precedence in KSM.

Is PodStatus already watchable via the k8sobjectsreceiver?

gdvalle commented 6 months ago

Is PodStatus already watchable via the k8sobjectsreceiver?

Do you mean k8sclusterreceiver? It has existing watch support for Pod, which should cover the Status struct AFAIK.

k8sobjectsreceiver, I'm not certain, but glancing at the field selection language I'm not sure how you would get the ready condition out of the list.

povilasv commented 6 months ago

I think given we have k8s.container.ready we could add k8s.pod.ready.

Regarding pod.status.condition does Kube State Metrics have it? Would be interesting to see what they do around that and what kind of values are there.

sirianni commented 6 months ago

I think we need agreement on how to represent it: other things in this receiver (e.g. k8s.pod.phase) use values to represent states, so 0=false, 1=true, 2=unknown could fit, but the ergonomics are a bit poor.

Please no.

See https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/29157 for a generic k8s.pod.state metric modeled after kube-state-metrics.

sirianni commented 6 months ago

In general, I feel like the piecemeal and fragmented approach to Kubernetes metrics in OTel is going to make it impossible to have any vendors build meaningful user experiences on top of this dataset, ultimately delaying any possible standardization in this space.

povilasv commented 6 months ago

In this case k8s.pod.state sounds to me more like resource attribute than a metric? Then IMO we can add is a resource attribute.

Regarding:

think we need agreement on how to represent it: other things in this receiver (e.g. k8s.pod.phase) use values to represent states, so 0=false, 1=true, 2=unknown could fit, but the ergonomics are a bit poor.

I guess your against this enum approach? IMO then this should be added to the spec, how to correctly represent enums in otel, so we can solve it once and for all.

TylerHelmuth commented 6 months ago

In general, I feel like the piecemeal and fragmented approach to Kubernetes metrics in OTel is going to make it impossible to have any vendors build meaningful user experiences on top of this dataset, ultimately delaying any possible standardization in this space.

We have had an uptick in k8s components feature request, it would be be very nice to make progress on https://github.com/open-telemetry/semantic-conventions/issues/1032

sirianni commented 6 months ago

Thanks @TylerHelmuth . In general, my team has been happy with the metrics collected by kubeletstatsreceiver and how they are modeled. They are struggling significantly with the "state" metrics that come from k8sclusterreceiver. We are coming from a Datadog background.

gdvalle commented 6 months ago

I guess your against this enum approach? IMO then this should be added to the spec, how to correctly represent enums in otel, so we can solve it once and for all.

I made a bit of a side comment there. I personally feel the value-as-attribute approach to enums has the better user experience, and that's what we should favor, but the clear downside is extraneous metric volume when reporting zeroes. Machines use the numeric enum, but humans want the string.

I think some of this can be resolved in spec, and some ideas are outlined in https://github.com/open-telemetry/opentelemetry-specification/issues/1711, but given there's no consensus for some time, I'd suggest we move forward with existing patterns here. Whichever direction we go there's a need to review and refactor existing metrics in the collector.

More than we need something ideal, we need something usable.

In this case k8s.pod.state sounds to me more like resource attribute than a metric? Then IMO we can add is a resource attribute.

I think the problem with resource metrics for that (as I understand it) is they're effectively metadata, emitted to enrich an actual point. In the pod state example, the state change itself is the point.

Can we move forward, regardless of the enum end-game? I think we need to choose whether we represent pod ready status as e.g. k8s.pod.status.condition (generic) or k8s.pod.ready (cherry-picked/named). I think that means I am proposing k8s.pod.status.condition be part of the k8s semconv. WDYT?

povilasv commented 6 months ago

I agree on:

I'd suggest we move forward with existing patterns here. Whichever direction we go there's a need to review and refactor existing metrics in the collector.

I personally would be okay with k8s.pod.ready metric.

Regarding k8s.pod.status.condition, would be interesting to see some example data points to understand what kind of data would it actually produces and what did Kube State Metrics do in this regard.

gdvalle commented 6 months ago

Regarding k8s.pod.status.condition, would be interesting to see some example data points to understand what kind of data would it actually produces and what did Kube State Metrics do in this regard.

Whoops, thought I answered your question earlier, but apparently didn't hit send. KSM seems to use named conditions, kube_pod_status_ready and kube_pod_status_scheduled, so they only support whatever someone bothers to implement.

The generic approach would take any condition and record points for it. Here's a sample from a GKE cluster:

status:
  conditions:
  - lastProbeTime: null
    lastTransitionTime: null
    message: 'Pod is in NEG "Key{\"k8s1-7929355b-foo-80-6e399202\",
      zone: \"foo-b\"}". NEG is not attached to any BackendService with health
      checking. Marking condition "cloud.google.com/load-balancer-neg-ready" to True.'
    reason: LoadBalancerNegWithoutHealthCheck
    status: "True"
    type: cloud.google.com/load-balancer-neg-ready
  - lastProbeTime: null
    lastTransitionTime: "2024-05-28T19:14:30Z"
    status: "True"
    type: Initialized
  - lastProbeTime: null
    lastTransitionTime: "2024-05-28T19:14:36Z"
    status: "True"
    type: Ready
  - lastProbeTime: null
    lastTransitionTime: "2024-05-28T19:14:35Z"
    status: "True"
    type: ContainersReady
  - lastProbeTime: null
    lastTransitionTime: "2024-05-28T19:14:20Z"
    status: "True"
    type: PodScheduled

The enum is the status field, so Unknown, True, or False (docs). We can use reason ~and message~ fields as attributes. To illustrate:

k8s.pod.status.condition{type:"Ready", reason:"", message:""} 1
k8s.pod.status.condition{type:"cloud.google.com/load-balancer-neg-ready", reason:"LoadBalancerNegWithoutHealthCheck", message:'Pod is in NEG "Key{\"k8s1-7929355b-foo-80-6e399202\",
      zone: \"foo-b\"}". NEG is not attached to any BackendService with health
      checking. Marking condition "cloud.google.com/load-balancer-neg-ready" to True.'} 1

Edit: as pointed out below, message is probably too high cardinality for many systems, and has less value, so nix that.

povilasv commented 6 months ago

IMO message field is a big no, too much carnality and the value is too huge. Prometheus and other systems won't handle this.

Also, this looks like more like events to me rather than metric that tracks state changes?

Maybe instead if should be k8s.pod.status.last_condition or something like that and it would show the last condition from the list?

gdvalle commented 6 months ago

IMO message field is a big no, too much carnality and the value is too huge. Prometheus and other systems won't handle this.

I think it has potentially unbounded cardinality! Agree it's not a fit. I think reason is the field we want as it's machine-oriented.

Also, this looks like more like events to me rather than metric that tracks state changes?

k8s.pod.ready represents the Ready condition. You could also emit as events/logs, but I think they're far more useful as metrics, unless maybe you want to read the message field. The idea is making the case that Ready isn't special, and we can just represent all of these conditions so we're not inventing a new metric name for each.

Maybe instead if should be k8s.pod.status.last_condition or something like that and it would show the last condition from the list?

The k8scluster receiver works by watching over a pod resource, so we emit the current condition each time it changes. I don't think it makes sense to call it "last condition"?

dfsdevops commented 4 months ago

What about something like

Regarding k8s.pod.status.condition, would be interesting to see some example data points to understand what kind of data would it actually produces and what did Kube State Metrics do in this regard.

Whoops, thought I answered your question earlier, but apparently didn't hit send. KSM seems to use named conditions, kube_pod_status_ready and kube_pod_status_scheduled, so they only support whatever someone bothers to implement.

I like the idea of each status having it's own enableable metric, with k8s.pod.status_ready being enabled by default, and the configuring other status conditions to be included optionally via config. Right now some gymnastics have to be performed to tell if a running pod is actually fully ready or not because you have to cross reference the container metrics. As a user, having these is valuable in my mind as it could be useful in benchmarking or just generally getting a timeline of how long it takes pods to accumulate certain statuses over time.

github-actions[bot] commented 2 weeks ago

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.