Skip to content
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

Merged
merged 21 commits into from
Nov 27, 2023

Conversation

tokaplan
Copy link
Contributor

@tokaplan tokaplan commented Nov 7, 2023

Changes

This adds support for pod's labels to k8s.pod resource.

Merge requirement checklist

Copy link

linux-foundation-easycla bot commented Nov 7, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@tokaplan tokaplan marked this pull request as ready for review November 9, 2023 21:45
@tokaplan tokaplan requested review from a team November 9, 2023 21:45
@trask
Copy link
Member

trask commented Nov 9, 2023

@trask
Copy link
Member

trask commented Nov 9, 2023

closing and re-opening to try to retrigger CI checks which seem stuck

@trask trask closed this Nov 9, 2023
@trask trask reopened this Nov 9, 2023
Copy link
Contributor

@dashpole dashpole left a 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.

docs/resource/k8s.md Outdated Show resolved Hide resolved
docs/resource/k8s.md Outdated Show resolved Hide resolved
model/resource/k8s.yaml Outdated Show resolved Hide resolved
@arminru arminru requested a review from a team November 14, 2023 12:25
CHANGELOG.md Outdated Show resolved Hide resolved
@ChrsMark
Copy link
Member

Hey @tokaplan since the k8s attributes are already in the registry shall we add the new one to the registry as well?

It would need an update to the registry's markdown as well.

@tokaplan
Copy link
Contributor Author

tokaplan commented Nov 20, 2023

Hey @tokaplan since the k8s attributes are already in the registry shall we add the new one to the registry as well?

It would need an update to the registry's markdown as well.

@ChrsMark Done, thank you

Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@dmitryax
Copy link
Member

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?

@AlexanderWert
Copy link
Member

AlexanderWert commented Nov 22, 2023

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)

@AlexanderWert AlexanderWert merged commit fc73453 into open-telemetry:main Nov 27, 2023
9 checks passed
@tokaplan tokaplan deleted the pod-annotations branch November 27, 2023 19:38
pyohannes pushed a commit to pyohannes/semantic-conventions that referenced this pull request Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.