Add k8s container ports attribute#2358
Conversation
| or | ||
| [K8s ContainerStateTerminated](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#containerstateterminated-v1-core) | ||
| examples: ['ContainerCreating', 'CrashLoopBackOff', 'CreateContainerConfigError', 'ErrImagePull', 'ImagePullBackOff', 'OOMKilled', 'Completed', 'Error', 'ContainerCannotRun'] | ||
| - id: k8s.container.ports |
There was a problem hiding this comment.
I think this should be under the container namespace.
There was a problem hiding this comment.
I think this should be under the container namespace.
I think this should be under the container namespace.
Did you mean this field belongs under the model/container section? I partially agree it fits there. However, given it's specific to Kubernetes, and I see other k8s.container.XXX fields already present, I'm wondering what rule guides the placement for such Kubernetes-specific attributes.
There was a problem hiding this comment.
I think the "container" namespace (without the k8s prefix) is reserved for generic, container runtime-agnostic attributes. In contrast, k8s.container.* attributes are reserved for information that’s specific to Kubernetes’ object model and APIs.
For container ports, if we only want to capture a simple list of exposed ports, it makes sense to propose a generic container.ports attribute. However, in k8s, ports can have additional metadata like name, and we may want to accommodate such enhancements in the future (see the discussion here). Given that, perhaps it makes sense to keep this under k8s.container.ports if we think future enhancements where we add more structured port information is a use case.
There was a problem hiding this comment.
I just looked at AWS ECS & gcp compute engine, both of which allow the protocol to be specified with ecs also allowing a name.
Thinking about it some more it might more sense to have a way to describe firewall/routing rules which is what is happening.
There was a problem hiding this comment.
It would make sense to use the container.* namespace if we can justify the decision based on an existing unified container spec, like https://opencontainers.org/. I'm not sure if OCI covers this (container ports) already, but with a quick search it seems that it doesn't.
If there is no unified container spec covering this right now, then we should go with the k8s.container.* option following the K8s API.
There was a problem hiding this comment.
I'm not sure OCI Image spec is what we are looking for here. Also if we are going to consume what K8s API provides then that might be a subset of the ports a runtime container actually exposes. In that case the k8s.container.* namespace need to be used.
@spurplewang could you verify what the context is here?
From open-telemetry/opentelemetry-collector-contrib#38019 I understand that the need is for providing the container ports that a K8s Pod spec defines as metadata, right? That might be different from what the runtime container actually exposes eventually and to know that we'd need to query the runtime API (docker, containerd, cri-o etc).
There was a problem hiding this comment.
Yes, the container ports defined in k8s pod spec will be used as metadata for metrics.
There was a problem hiding this comment.
In that case I think k8s.container.ports is the most accurate for this.
/cc @open-telemetry/semconv-k8s-approvers
There was a problem hiding this comment.
Hi @ChrsMark , I accidently closed this PR. I did reopen it but not sure how to bring it back to awaiting SIG approval in Semantic Conventions Triage. Could you help navigate how that could be done? Thanks!
github-project-automation
Bot
moved this from Untriaged
to Blocked
in Semantic Conventions Triage
lmolkova
moved this from Blocked
to Awaiting SIG approval
in Semantic Conventions Triage
spurplewang
force-pushed
the
addcontainerports
branch
from
2597bf6 to
463b7f5
Compare
There was a problem hiding this comment.
Thank's for reviving this!
It will need an entry to the .md file as well similar to https://github.com/open-telemetry/semantic-conventions/blob/v1.36.0/docs/system/k8s-metrics.md?plain=1#L283. Then run make fix to automatically update the md files.
ChrsMark
left a comment
ChrsMark
left a comment
There was a problem hiding this comment.
Hey @spurplewang, the change looks good.
Could you add a changelog entry for this and also run make fix to automatically update the md files?
@open-telemetry/semconv-k8s-approvers @jinja2 please take a look :)
| stability: development | ||
| brief: > | ||
| Number of port to expose on the pod's IP address. This must be a valid port number, 0 < x < 65536. | ||
| examples: [10053, 8080, 8081] |
There was a problem hiding this comment.
Should we include a full example for k8s.container.ports? would it be something like [{container_port: 10053}, {container_port: 8080}]?
There was a problem hiding this comment.
This type of example would need to be one level up though. @open-telemetry/specs-semconv-approvers is this type of examples allowed in general for map[] attributes?
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
This PR has been labeled as stale due to lack of activity. It will be automatically closed if there is no further activity over the next 14 days. |
5 participants
Fixes #2357
Changes
Add k8s container ports attribute
Merge requirement checklist
[chore]