[receiver/k8scluster] add attributes to node and pod entities #36862
Conversation
github-actions
bot
requested review from
ChrsMark,
dmitryax,
povilasv and
TylerHelmuth
jinja2
force-pushed
the
add-k8scluster-attr
branch
from
b9be8c2 to
226670f
Compare
| "foo1": "", | ||
| "k8s.node.name": "test-node-1", | ||
| "node.creation_timestamp": "0001-01-01T00:00:00Z", | ||
| "k8s.node.condition_disk_pressure": "false", |
There was a problem hiding this comment.
Shouldn't those first go through Semantic Conventions?
We have been blocking PRs adding new metrics or attributes in k8s components asking for SemConv definition first. Not sure if this has changed.
There was a problem hiding this comment.
These are experimental entity attributes. They are aligned with the existing metrics. It can be easily changed later since it's experimental
There was a problem hiding this comment.
Do we want to define such attributes in SemConv first even if we don't have entity signal yet?
There was a problem hiding this comment.
These won't be eligible as resource attributes due to values being mutable. And I tried to align with our existing stuff. But if we are ready to start with enitities in semantic conventions then I'd be happy to add these.
There was a problem hiding this comment.
Maybe those would make sense to be defined in the attributes' registry first? (It's a chicken-egg problem)
Having a specific guideline about what we "allow" to be added directly and what not would help here. I'm not sure if we have sth like this right now. I guess we haven't.
Since entity signal is not yet defined maybe it would be just fine if we merge them and file an issue against SemConv to keep track of them making clear the reason why those were added to the receiver now. This issue could be used for reference in the future if there are more requests for additional attributes?
I don't have strong preference here, just want to ensure consistency.
There was a problem hiding this comment.
Since entity signal is not yet defined maybe it would be just fine if we merge them and file an issue against SemConv to keep track of them making clear the reason why those were added to the receiver now. This issue could be used for reference in the future if there are more requests for additional attributes?
Your idea sounds good! This will help us make progress on the experimental entities (and hopeful encourage its usage) while still tracking the new additions. I'll start an issue on SemConv.
There was a problem hiding this comment.
Sounds good to me. Let's have a tracking issue for k8s entity attributes that we have in the collector. The issue will be essentially blocked until we have the signal ready.
There was a problem hiding this comment.
There was a problem hiding this comment.
Thank's! Please consider adding this in the PR description as well for easier future reference.
| "foo1": "", | ||
| "k8s.node.name": "test-node-1", | ||
| "node.creation_timestamp": "0001-01-01T00:00:00Z", | ||
| "k8s.node.condition_disk_pressure": "false", |
There was a problem hiding this comment.
Thank's! Please consider adding this in the PR description as well for easier future reference.
jinja2
changed the title
jinja2
changed the title
jinja2
force-pushed
the
add-k8scluster-attr
branch
from
e8d0b6c to
5582d26
Compare
There was a problem hiding this comment.
LGTM
@TylerHelmuth @povilasv please take a look, specially on the plan discussed at https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/36862/files#r1888186023
Description
Adds below additional metadata attributes to the node and pod entity -
k8s.pod.phase - This is similar to the existing metric k8s.pod.phase. The values can be Pending, Running, Succeeded, Failed, Unknown.
k8s.pod.status_reason - Similar to k8s.pod.status_reason metric. A brief CamelCase message indicating details about why the pod is in this state. Example values - Evicted, NodeLost, UnexpectedAdmissionError
k8s.node.condition_{type} - similar to existing metrics enabled by the config node_conditions_to_report, e.g. k8s.node.condition_ready. Add k8s default kubelet conditions only.
We'll be tracking changes to entitiy attributes for k8s in this SemConv issue.
Link to tracking issue
Fixes - #36859
Testing
Added unit tests and verified in cluster
Documentation