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

Overhaul process command-line resource conventions. #1137

Merged

Conversation

Oberon00
Copy link
Member

@Oberon00 Oberon00 commented Oct 23, 2020

Fixes #831, fixes #997

Changes

@Oberon00 Oberon00 added area:semantic-conventions Related to semantic conventions spec:resource Related to the specification/resource directory labels Oct 23, 2020
@Oberon00 Oberon00 changed the title Clean up process resource conventions. Overhaul process resource conventions. Oct 23, 2020
@Oberon00 Oberon00 marked this pull request as ready for review October 23, 2020 14:30
@Oberon00 Oberon00 requested review from a team October 23, 2020 14:30
@Oberon00

This comment has been minimized.

On Windows and other systems where the native format of process commands is a single string,
`process.command_args` can additionally (or instead) be used.

At least one of `process.executable.name`, `process.executable.path`, `process.command`, `process.command_line` or `process.command_args` is required.
Copy link
Member

Choose a reason for hiding this comment

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

What if someone only wants to report the PID for whatever reason? What about:

Suggested change
At least one of `process.executable.name`, `process.executable.path`, `process.command`, `process.command_line` or `process.command_args` is required.
At least one of `process.executable.name`, `process.executable.path`, `process.command`, `process.command_line` or `process.command_args` is required to allow back ends to identify the executable.

Copy link
Member Author

Choose a reason for hiding this comment

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

This paragraph was already there previously, I just added the command_args alternative.

@arminru

This comment has been minimized.

@arminru arminru self-assigned this Oct 23, 2020
@arminru arminru requested review from a team October 23, 2020 14:47
@Oberon00

This comment has been minimized.

@arminru

This comment has been minimized.

Comment on lines +20 to +23

For backwards compatibility with older versions of this semantic convention,
it is possible but deprecated to use an array as type for `process.command_line`.
In that case it MUST be interpreted as if it was `process.command_args`.
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this paragraph to keep the change non-breaking but I would be happy to remove it if deemed unnecessary.

Copy link
Member

@arminru arminru left a comment

Choose a reason for hiding this comment

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

LGTM. I'd prefer to remove the backwards-compatibility part as mentioned in the comment, however.

Comment on lines +20 to +23

For backwards compatibility with older versions of this semantic convention,
it is possible but deprecated to use an array as type for `process.command_line`.
In that case it MUST be interpreted as if it was `process.command_args`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
For backwards compatibility with older versions of this semantic convention,
it is possible but deprecated to use an array as type for `process.command_line`.
In that case it MUST be interpreted as if it was `process.command_args`.

I'm not sure if we want to keep backwards compatibility here since the previous solution was a bit unclean IMHO. We don't have multiple types (string and array of strings in this case) for the same attribute anywhere else, which is also why we weren't able to pour these into YAML definitions for the markdown and constants generator tool yet. While not making interpretation impossible, I think it would still be easier for consumers and overall cleaner if we can stick to one well-defined type per attribute.

I would be +1 for a breaking change here if we can settle on this before GA (which I hope we can). The strict trace spec freeze does not apply to semantic conventions as far as I know.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we would still declare command_line as non-array string in the YAML and have this text just there in the markdown as prose. We should clarify that the MUST only applies for those that choose to support this at at all (which should be a MAY).

Copy link
Contributor

Choose a reason for hiding this comment

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

I will register that I don't understand why it matters if a value can take on more than one type. I didn't see a problem with command_line being a string or a list. I really wish I knew how people plan on using this information such that the this distinction matters. 😁

Copy link
Member Author

Choose a reason for hiding this comment

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

For one, this is not supported by the current semantic convention tool, which is why this file has no corresponding YAML yet.

Comment on lines +20 to +23

For backwards compatibility with older versions of this semantic convention,
it is possible but deprecated to use an array as type for `process.command_line`.
In that case it MUST be interpreted as if it was `process.command_args`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I will register that I don't understand why it matters if a value can take on more than one type. I didn't see a problem with command_line being a string or a list. I really wish I knew how people plan on using this information such that the this distinction matters. 😁

@jmacd jmacd changed the title Overhaul process resource conventions. Overhaul process command-line resource conventions. Oct 27, 2020
@Oberon00
Copy link
Member Author

@anuraaga Could you please review again? I fixed the issue you mentioned.

@Oberon00
Copy link
Member Author

Oberon00 commented Nov 3, 2020

This PR formally fulfills the requirements for being merged. So I guess if you want to still review (@anuraaga?) please do so quickly.

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

So sorry for the late review - having trouble differentiating mentions from review requests in GitHub notifications and need to figure out a way to better handle them.

@arminru
Copy link
Member

arminru commented Nov 3, 2020

@anuraaga On https://github.com/pulls ("Pull requests" in Github's title bar) there is a filter called "Mentioned". That one shows all PRs on which you were mentioned at any point, thus also PRs where you were only mentioned in the description or an old comment, but it's better than nothing 🙂

@arminru arminru merged commit b1b5b76 into open-telemetry:master Nov 3, 2020
@arminru arminru deleted the issue-997-runtime-process branch November 3, 2020 10:50
@anuraaga
Copy link
Contributor

anuraaga commented Nov 3, 2020

@arminru Yeah I generally use github.com/notifications and it has a link for mentions. I need to get used to clicking it at least once a day :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions spec:resource Related to the specification/resource directory
Projects
None yet
4 participants