k8sattributes: automatically extract k8s.container.name

[gbbr]

Component(s)

processor/k8sattributes

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

The container name is already available in the spec and could easily be added when extracting pod container attributes as part of the already existing Container struct.

The information could be added as the k8s.container.name tag.

Describe the solution you'd like

I would like to add this automatically (or by means of config). Happy to work on it myself.

[github-actions]

Pinging code owners:

• processor/k8sattributes: @dmitryax @rmfitzpatrick

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

[dmitryax]

How do you know which container is sending data from a pod?

[gbbr]

Thanks for the response Dmitrii. I see what you are saying: currently the code is relying on the fact that the k8s.container.name resource attribute is set, and otherwise ignores adding some container tags (such as the image path and tag).

What I would recommend is to continue using the same logic, but instead relying on the container.id attribute. This comes more natural when you look at already existing SDK APIs, such as opentelemetry-go's WithContainer{ID} methods.

Actually, discovering this now, I understand why I was surprised to find out that container tags were missing in my traces, despite me adding the container ID in the SDK. That is quite unfortunate and unexepected.

I wouldn't ask to make a breaking change, but to at least temporarily add this functionality to allow picking up the container.id and use that instead to obtain all the other information. We may later on discuss on whether we'd like to deprecate the k8s.container.name requirement completely, or keep both (with tradeoffs).

[gbbr]

To shed more light onto the alternative I am proposing, can you please clarify how users are expected to set k8s.container.name in their setup? The current documentation assumes that the user knows this, but I couldn't figure out an easy automatic way.

[dmitryax]

@gbbr I see now. I didn't know that instrumentations can set container.id. Probably it's something new. We can add this functionality. It's not a breaking change. The only thing needed is to add is the support of container.id provided as part of pod_association (it technically becomes container association, but we can rethink the configuration interface in a separate effort). Do you want to work on adding this functionality?

[gbbr]

Yes, I'd be happy to work on this. Thanks for looking.

[gbbr]

P.S. I was thinking container.id would be picked up by default and supersed all other associations since it is very specific and clearly sent by the SDK/user specifically for this purpose (of association). Then, the pod_associations would be merely a fallback. But would this then be a breaking change? It could be for some users. 🤔

[dmitryax]

Looking for the specific container always comes after looking for a pod, so it doesn't matter which attribute is used to identify a container within a pod.

If we want to keep it this way, we don't even need any changes to the pod_association. We can just use container.id to identify a container within a pod if k8s.container.name isn't set. If we want to use container.id for pods lookup, we need to add support of container.id resource attributes in pod_association, which is not a breaking change as long as the default value isn't changed.

Another thing you need to do, is to add k8s.container.name support in metadata config section.

[gbbr]

Looking for the specific container always comes after looking for a pod, so it doesn't matter which attribute is used to identify a container within a pod.

Got it.

If we want to keep it this way, we don't even need any changes to the pod_association. We can just use container.id to identify a container within a pod if k8s.container.name isn't set. If we want to use container.id for pods lookup, we need to add support of container.id resource attributes in pod_association, which is not a breaking change as long as the default value isn't changed.

Thanks. I will do it as you initially suggested, as part of pod_association. I think we may be able to do even more with container.id later on.

Another thing you need to do, is to add k8s.container.name support in metadata config section.

Good point. This is something we need better support for (e.g. #19759). It is easy to omit this detail if nothing (or nobody) warns you.

[fatsheep9146]

Thanks. I will do it as you initially suggested, as part of pod_association. I think we may be able to do even more with container.id later on.

Just for curiosity, what benefits we can get from adding container.id as part of pod_association?
Could you share some examples?
@gbbr @dmitryax

[dmitryax]

Just for curiosity, what benefits we can get from adding container.id as part of pod_association?
Could you share some examples?

No need to provide any other pod-identifying attributes or use the connection source. I'm not sure how desirable this outcome is. @gbbr can probably provide some user perspective on this. My only concern is that this can increase the complexity (need to see a PR tho). But we definitely have to make sure that it doesn't affect CPU/memory utilization if container.id isn't set in pod_association.

[gbbr]

I'm going to turn back around on what I initially said re. my approach. I was not familiar enough with the code base at the time to express the best opinion. My suggestion is as follows:

1. Review and consider the initial PR I made: processor/k8sattributesprocessor: map containers by ID #20340, which simply takes the container.id as sent via resource attributes and uses it to obtain the container name, image name & tag.
2. Consider getting pods by container ID (as an alternative to the other pod associations) in a separate PR as that is indeed a bit more tricky and would overload the current PR in complexity.

This is basically what you (@dmitryax) suggested here. WDYT?

[gbbr]

@fatsheep9146 the main advantage I can think of is that you can then use the SDK API for pod association, without needing to mess around with your Kubernetes deployment's YAML to set environment variables etc. However, I am now proposing (as per my latest comment) that we address this aspect as part of a separate issue (which I am willing to open and work on). Otherwise, mixing both this change and that one might result in too much complexity both for reviewing and for writing in a qualitative way.