-
Notifications
You must be signed in to change notification settings - Fork 183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expanding k8s.pod resource to include pod labels #494
Conversation
|
closing and re-opening to try to retrigger CI checks which seem stuck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me. It seems more usable than a []string
for k8s.pod.labels
and follows the existing pattern set by container.labels
.
Hey @tokaplan since the It would need an update to the registry's markdown as well. |
@ChrsMark Done, thank you |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
The change looks good to me from the semantic conventions perspective. This pattern is already being used in the collector's k8sattributes processor. I'm not sure about utilizing this definition. AFAIK, this is the first example of a "partial" attributes definition. So, I'm not sure that the tooling that generates the semantic conventions code in the instrumentation libraries and collector will not fail. Even if doesn't fail, the generated code likely will be useless. Should we prepare for that before merging this PR by properly supporting this new type of resource attributes? Or at least provide a way to mark them in some way to skip the code generation? |
@dmitryax Please see this comment: #494 (comment) |
Co-authored-by: Armin Ruech <7052238+arminru@users.noreply.github.com> Co-authored-by: Alexander Wert <AlexanderWert@users.noreply.github.com>
Changes
This adds support for pod's labels to k8s.pod resource.
Merge requirement checklist