-
Notifications
You must be signed in to change notification settings - Fork 896
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 spec resource container attributes #3468
Add spec resource container attributes #3468
Conversation
23c6eb2
to
d2a1134
Compare
@marcsanmi heads up - most likely this PR will be closed, and we'll ask you to resubmit the PR in a new repo, please refer to #3474 (comment). |
@@ -11,7 +11,9 @@ | |||
|---|---|---|---|---| | |||
| `container.name` | string | Container name used by container runtime. | `opentelemetry-autoconf` | Recommended | | |||
| `container.id` | string | Container ID. Usually a UUID, as for example used to [identify Docker containers](https://docs.docker.com/engine/reference/run/#container-identification). The UUID might be abbreviated. | `a3bf90e006b2` | Recommended | | |||
| `container.command_line` | string | The full command executed by the container. | `nginx -g daemon off;` | Recommended | |
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.
Could this contain any credentials that would need to be redacted?
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.
Good point. Yes, this would contain any command (+args) the container runs. By need to be redacted
do you mean writing some advice in the Description
column?
It's worth mentioning that this resource attribute will be disabled by default. Let me know your thoughts.
Note: considering the first comment, I'll make the changes in the new repository.
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.
Anything added regarding that here should probably added also to the process.* semantic conventions. And I wonder if these could somehow be shared. At least they should be aligned, so consider also adding a container.command_args string array. https://github.com/open-telemetry/opentelemetry-specification/blob/b0a22680de561c2a7d45cf78aa65c65583f42ed2/specification/resource/semantic_conventions/process.md#process
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.
You'll need to add these changes in the YAML definition from which these MD files are then generated.
See https://github.com/open-telemetry/opentelemetry-specification/tree/main/semantic_conventions for the YAMLs and the README there for instructions.
@marcsanmi the move of semconv to a new repo happened. Could you please rebase and submit the PR there? https://github.com/open-telemetry/semantic-conventions |
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.
Please reopen this against: https://github.com/open-telemetry/semantic-conventions
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Changes
Update the container semantic conventions to add a couple of non-default resource attributes introduced in open-telemetry/opentelemetry-collector-contrib#21185.