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 process.name attribute #1737

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

braydonk
Copy link
Contributor

Fixes #1736

Changes

This PR adds a new attribute process.name that uses the description that used to apply to process.executable.name. The process.executable.name attribute's description is adjusted such that the value of the attribute will reliably contain the executable name.

Merge requirement checklist

@braydonk braydonk requested review from a team as code owners January 10, 2025 14:22
@braydonk braydonk requested a review from a team as a code owner January 10, 2025 14:26
Copy link
Member

@christos68k christos68k left a comment

Choose a reason for hiding this comment

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

Thanks!

stability: experimental
brief: >
The name of the process. On Linux based systems, can be set
to the `Name` in `proc/[pid]/status`. On Windows, can be set to the
Copy link
Member

@christos68k christos68k Jan 10, 2025

Choose a reason for hiding this comment

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

Suggested change
to the `Name` in `proc/[pid]/status`. On Windows, can be set to the
to the value in `/proc/[pid]/comm` or to the (equivalent)
`Name` in `/proc/[pid]/status`. On Windows, can be set to the

We can also use /proc/[pid]/comm which requires no parsing, unlike extracting Name out of /proc/[pid]/status. The values should be equivalent.

Also see:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think about suggesting both since they're the same anyway? For example, in the hostmetricsreceiver we'll already have parsed /proc/[pid]/status for other information, so just getting the name from that is fine in our case, but in other cases people might prefer to just get /proc/[pid]/comm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 5a679fd

# Use pipe (|) for multiline entries.
subtext: |
The new `process.name` attribute uses the original guidance for `process.executable.name`,
suggesting use of the `Name` field from `/proc/[pid]/status` on Linux. `process.executable.name`
Copy link
Member

@christos68k christos68k Jan 10, 2025

Choose a reason for hiding this comment

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

Suggested change
suggesting use of the `Name` field from `/proc/[pid]/status` on Linux. `process.executable.name`
suggesting use of `/proc/[pid]/comm` or the equivalent `Name` field
from `/proc/[pid]/status` on Linux. `process.executable.name`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 5a679fd

type: string
stability: experimental
brief: >
The name of the process. On Linux based systems, can be set to
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can or SHOULD ?

to the `Name` in `proc/[pid]/status`. On Windows, can be set to the
base name of `GetProcessImageFileNameW`.
to the base name of the target of `/proc/[pid]/exe`. On Windows,
can be set to the base name of `GetProcessImageFileNameW`.
examples: ['otelcol']
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be possible to have an example that's different between process.name and process.executable.name ?

to the `Name` in `proc/[pid]/status`. On Windows, can be set to the
base name of `GetProcessImageFileNameW`.
to the base name of the target of `/proc/[pid]/exe`. On Windows,
can be set to the base name of `GetProcessImageFileNameW`.
Copy link
Contributor

@lmolkova lmolkova Jan 10, 2025

Choose a reason for hiding this comment

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

It seems process.name and process.executable.name are always the same on windows - is it the case?

In the spirit of T-shaped API, do you think it could be one of these linux-specific things? I.e. process.linux.exe|exectuable.name ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

process.executable.name shouldn't use /proc/[pid]/status
3 participants