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

Add container labels field from ECS #125

Merged
merged 3 commits into from
Sep 26, 2023

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Jun 20, 2023

This PR is part of #72 and adds the container.lables.<key> similarly to http-request-and-response-headers.

Note that these labels refer to the container labels themselves and not the container's image labels. For image labels we would need to attach them under container.image.labels.* instead but for now there is no specific need for populating these too.

Note also that I had to manually update the markdown table cause the make table-generation command was failing with the following:

Invalid id labels.<key>. Semantic Convention ids MUST match ([a-z](\.?[a-z0-9_-]+)+) - @35:9 make: *** [Makefile:77: table-generation] Error 1

It seems that the http-headers table was created manually too.

cc: @AlexanderWert @kaiyan-sheng @mlunadia

@ChrsMark ChrsMark force-pushed the add_container_labels branch from 865c663 to 52ceb1c Compare September 11, 2023 10:29
@ChrsMark ChrsMark marked this pull request as ready for review September 11, 2023 10:44
@ChrsMark ChrsMark requested review from a team September 11, 2023 10:45
@ChrsMark ChrsMark force-pushed the add_container_labels branch 2 times, most recently from 80144e7 to 918491f Compare September 13, 2023 06:53
Copy link
Member

@AlexanderWert AlexanderWert left a comment

Choose a reason for hiding this comment

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

LGTM

@ChrsMark
Copy link
Member Author

@open-telemetry/specs-semconv-approvers @open-telemetry/specs-semconv-maintainers could we have some more reviews on this? Otherwise we could merge if no objection?

@ChrsMark
Copy link
Member Author

Thanks for the reviews folks! Anything else missing here?

Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark force-pushed the add_container_labels branch from dcc04c5 to 957bbc6 Compare September 22, 2023 13:26
@ChrsMark ChrsMark requested a review from arminru September 22, 2023 13:26
@joaopgrassi
Copy link
Member

@ChrsMark I just looked into https://www.elastic.co/guide/en/ecs/current/ecs-container.html#field-container-labels and there it says: "Image labels.". But in the PR here you say these are container labels, and not image. Is this intended? Wouldn't that contradict ECS? 🤔

@kaiyan-sheng
Copy link
Contributor

@ChrsMark I just looked into https://www.elastic.co/guide/en/ecs/current/ecs-container.html#field-container-labels and there it says: "Image labels.". But in the PR here you say these are container labels, and not image. Is this intended? Wouldn't that contradict ECS? 🤔

@joaopgrassi Sorry Christos is out on vacation this week. I think in ECS Image labels is specifically talking about docker container labels. These labels are added to the image through dockerfile and then to the container. So here for the container labels, it doesn't contradict ECS.

@joaopgrassi joaopgrassi merged commit 2bcee3c into open-telemetry:main Sep 26, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants