[processor/k8sattributes] Update the list of attributes added by default

[dmitryax]

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

Currently, the following the processor adds the following attributes by default:

• k8s.namespace.name
• k8s.pod.name
• k8s.pod.uid
• k8s.pod.start_time
• k8s.deployment.name
• k8s.node.name
• container.image.name (if container identity provided)
• container.image.tag (if container identity provided)

I believe we need to revisit the list or make it required. I don't think we need to add the following attributes by default:

• k8s.pod.start_time because it's not defined in the spec and is arguably less useful than others.
• k8s.deployment.name because it's unclear why only deployment gets into the list but not daemonset or statefulset. Also, it requires keeping the replicasets in the processor's state, which appeared to be pretty expensive: [processor/k8sattributes] Excessive heap usage from k8sattributes processor after 0.78.0 upgrade #23067

Describe the solution you'd like

Update the list of the attributes added by default to:

- k8s.namespace.name
- k8s.pod.name
- k8s.pod.uid
- k8s.node.name
- container.image.name (if container identity provided)
- container.image.tag (if container identity provided)

Maybe we don't even add k8s.pod.uid for consistency with other fields

[dmitryax]

cc @povilasv @fatsheep9146 @gbbr please let me know WDYT

[fatsheep9146]

I wonder why container.image.name and container.image.tag are needed to default?
@dmitryax

[dmitryax]

I'm not sure. Maybe k8s.namespace.name, k8s.pod.name and k8s.node.name are enough. Happy to hear other suggestions for the default list.

[dmitryax]

What about?

- k8s.namespace.name
- k8s.pod.name
- k8s.node.name
- container.name (if container.id provided)

[fatsheep9146]

What about?

- k8s.namespace.name
- k8s.pod.name
- k8s.node.name
- container.name (if container.id provided)

I agree more with this version, I think the basic goal of using k8s attribute is to find out which container exports this span or metric. And in the next step, user would consider how to aggregate them with other labels, for example, deployment name, job name and so on.

@dmitryax
maybe the discussion can be shared with @open-telemetry/collector-contrib-approvers to hear more suggestions.

[gbbr]

I'm not an expert on how valuable these attributes are but I'll just drop my 2c:

• Re. start time: making it optional doesn't seem like a huge loss, but I do see it possibly being disappointing to people that are using it to measure things like uptime. Rather than removing it as a default, would it be a good compromise to simply document the concern?
• Re. deployment name: this seems like something useful, but I see your concerns:
  • Would adding daemonset and stateful make [processor/k8sattributes] Excessive heap usage from k8sattributes processor after 0.78.0 upgrade #23067 worse?
  • Isn't it possible to significantly improve [processor/k8sattributes] Excessive heap usage from k8sattributes processor after 0.78.0 upgrade #23067 by means of a cache?

If the answer is no to the latter, then I think that we should definitely make it disabled by default.

[sirianni]

I personally think these attributes are useful

- k8s.pod.name
- k8s.pod.uid
- k8s.node.name
- k8s.deployment.name
- k8s.statefulset.name
- k8s.daemonset.name
- container.image.name (if container identity provided)
- container.image.tag (if container identity provided)

If it's significantly more expensive to fetch k8s.[deployment|statefulset|daemonset].name then I suppose these could be omitted from the defaults, but I think those are probably the most useful tags for charting/alerting. I think filtering/faceting by k8s.namespace.name and k8s.deployment.name is the primary use case I see.

[dmitryax]

Ok, at this point, I don't think we have a strong reason to change it with a consensus on how the default list should look like. If #23067 can be resolved without performance degradation, I think we can just close this issue

[povilasv]

I personally think these attributes are useful

- k8s.pod.name
- k8s.pod.uid
- k8s.node.name
- k8s.deployment.name
- k8s.statefulset.name
- k8s.daemonset.name
- container.image.name (if container identity provided)
- container.image.tag (if container identity provided)

If it's significantly more expensive to fetch k8s.[deployment|statefulset|daemonset].name then I suppose these could be omitted from the defaults, but I think those are probably the most useful tags for charting/alerting. I think filtering/faceting by k8s.namespace.name and k8s.deployment.name is the primary use case I see.

I also tend to think that the default should include deployment.name and other high level resources if there is no performance drawbacks. These are very useful when digging in the data

[swiatekm]

I personally think these attributes are useful

- k8s.pod.name
- k8s.pod.uid
- k8s.node.name
- k8s.deployment.name
- k8s.statefulset.name
- k8s.daemonset.name
- container.image.name (if container identity provided)
- container.image.tag (if container identity provided)

If it's significantly more expensive to fetch k8s.[deployment|statefulset|daemonset].name then I suppose these could be omitted from the defaults, but I think those are probably the most useful tags for charting/alerting. I think filtering/faceting by k8s.namespace.name and k8s.deployment.name is the primary use case I see.

I agree with this list, minus k8s.pod.uid, which I don't think I've ever used. Does it really give us anything the Pod name doesn't?

[sirianni]

I agree with this list, minus k8s.pod.uid, which I don't think I've ever used.

Agreed. I included it because it seems inexpensive, but I've never used it either.

[jinja2]

I think k8s.pod.uid should be kept in the default set. It is the only attribute in the suggested list which is guaranteed to change on a pod recreate (pod name will be same for statefulset pod). Grouping by uid provides an easy way to visualize if a metric anomaly is due to pod spec change.

[github-actions]

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:

• processor/k8sattributes: @dmitryax @rmfitzpatrick @fatsheep9146

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

[github-actions]

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:

• processor/k8sattributes: @dmitryax @rmfitzpatrick @fatsheep9146 @TylerHelmuth

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

[github-actions]

This issue has been closed as inactive because it has been stale for 120 days with no activity.