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

Adjust process.command* to be opt-in #789

Closed
wants to merge 5 commits into from

Conversation

inssein
Copy link

@inssein inssein commented Mar 2, 2024

Fixes #626

Changes

I made it match the container resource.

Merge requirement checklist

@inssein inssein requested review from a team March 2, 2024 20:25
Copy link
Contributor

@lmolkova lmolkova left a comment

Choose a reason for hiding this comment

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

I believe command_args attributes should be defined as template attributes and have per-value opt-in mechanism similar to http headers. Created #790.
It can be done separately from this PR.

@trisch-me
Copy link
Contributor

@inssein we do have a new way of creating a changelog. Please update your PR

@trisch-me
Copy link
Contributor

Should we also add notes to the properties about sensitive content as well as we do for URL for example?
https://github.com/open-telemetry/semantic-conventions/blob/main/model/registry/url.yaml#L63

@joaopgrassi
Copy link
Member

I'd just want to hear from @breedx-nr and his comment on #626 (comment) before we merge this.

@joaopgrassi joaopgrassi requested review from lmolkova and a team March 20, 2024 13:52
@joaopgrassi
Copy link
Member

I took the liberty to fix the CI issues. Had to generate the markdown tables and add a change log. I think this is a breaking change.

CC @open-telemetry/semconv-system-approvers

@inssein
Copy link
Author

inssein commented Mar 20, 2024

Ah I totally missed the earlier notification, thank you @joaopgrassi for doing that.

@Oberon00
Copy link
Member

Oberon00 commented Mar 21, 2024

Note that this makes the conventions stricter and does not relax anything. Before, if you set one of the process.command* attributes, you didn't need to also set a process.executable* attribute. Now you do. My understanding of these "at least one of" groups is that if you set one of them then all the others are non-required (although it's undefined whether they are then opt-in or recommended).

So I'm not sure that the change does what you want it to do. process.command* was more or less opt-in already.

EDIT: Technically you could just change the requirement description from "conditionally_required: See alternative attributes below." to something like "conditionally_required: This should only be set as opt-in and another alternative (see below) should be preferred"

Copy link

github-actions bot commented Apr 6, 2024

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Apr 6, 2024
@@ -0,0 +1,5 @@
change_type: breaking
component: resource/process
Copy link
Contributor

Choose a reason for hiding this comment

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

it should be only process here, we don't have resource in registry

@github-actions github-actions bot removed the Stale label Apr 7, 2024
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Apr 22, 2024
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Apr 30, 2024
@trisch-me
Copy link
Contributor

@inssein please reopen PR again if you still want to implement this feature

@rogercoll
Copy link
Contributor

@inssein Would you be interested in reopening this PR? The PR was closed by the github-actions bot as being inactive, this PR is very helpful as would solve #626

@ChrsMark ChrsMark reopened this Sep 19, 2024
@ChrsMark ChrsMark requested review from a team as code owners September 19, 2024 14:21
@ChrsMark
Copy link
Member

This would require to be rebased on top of the recent project reorg.
@inssein would you like to revive this PR?

Copy link

github-actions bot commented Oct 5, 2024

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Oct 5, 2024
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Oct 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Adjust process.command_args and process.command_line to be opt-in
8 participants