Skip to content

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!

@arminru arminru changed the title add process.name attribute add process.name attribute and adapt process.executable.name Jan 14, 2025
@braydonk
Copy link
Contributor Author

Seeing lots of good points in the discussions but having trouble figuring out where to take this. Here's my attempt to synthesize the current state:

  • It's unclear when each attribute should be used, and how consumers should handle the common case where process.executable.name and process.name will be the same
  • The notes are specific only to Linux and Windows right now
    • This is partially because I was adapting the original notes that only mentioned the two operating systems, but I am myself biased because they are the only two operating systems I know anything about
  • Descriptions of the attributes should provide examples of how process.executable.name and process.name could differ
    • There was a suggestion to make use of the examples field for this but I'm not sure how to codify it
  • process.title is another attribute that can be considered in this arena of the process's name
  • The idea of the user's interpretation of the best name for the process is a good candidate for what we consider the "General Class" of instrumentation within our group, however the names exist as they are today because the different ways to "name" the process (name, executable name, title) need to be instrumented in specific ways to avoid user confusion (as described in the notes, albeit not clear enough based on feedback)

For the most part I agree with every point of feedback I've seen, but unfortunately that means I agree with some that are that conflict with each other. process.name feels like "General Class" instrumentation and yet its definition is inextricably linked with the exact concept of the "process name" from the OS. The descriptions do only mention Linux and Windows and should find ways to be more general to operating systems, and yet if we aren't somewhat prescriptive on instrumentation in the descriptions then the relationship between these attributes could become muddy if different instrumentation makes different decisions about what values to use.

So I hate to say it, but I'm kinda stuck. I don't have a good idea where to take these attributes from here. I think the only thing I'm confident on is that process.title should be represented somehow (I am conflicted on exact naming because proctitle is Linux-exclusive iirc and the equivalent on Windows would I guess be MainWindowTitle but idk about other OS's).

I'm open to suggestions on what to do with this one.

@github-actions github-actions bot added the enhancement New feature or request label Jan 23, 2025
@christos68k
Copy link
Member

christos68k commented Jan 23, 2025

  • It's unclear when each attribute should be used, and how consumers should handle the common case where process.executable.name and process.name will be the same

Regarding avoiding duplication, we could specify that if process.name is not present, the consumer may retrieve it from process.executable.name. This should work for all cases presented in this issue, including memfd on Linux. The trade-off is some extra complexity and stateful processing on the consumer side. Alternatively, we could opt for simplicity, allow for the same value to be present in both attributes and rely on protocol-level deduplication mechanisms such as a string table which we use in OTLP profiling (outside of which I don't have any context as to the extent of the issue).

@lmolkova
Copy link
Member

lmolkova commented Jan 24, 2025

I don't have a strong opinion, a few things we could consider to get unstuck:

  • not adding process.name for now. Sticking with process.executable.name, just changing its description to fix process.executable.name shouldn't use /proc/[pid]/status #1736
  • going forward with process.name, but leaving its description open and soft so it can evolve after attribute is stabilized.
  • going forward with process.name in whatever form, but excluding it from initial stability scope.

For the most part I agree with every point of feedback I've seen, but unfortunately that means I agree with some that are that conflict with each other. process.name feels like "General Class" instrumentation and yet its definition is inextricably linked with the exact concept of the "process name" from the OS. The descriptions do only mention Linux and Windows and should find ways to be more general to operating systems, and yet if we aren't somewhat prescriptive on instrumentation in the descriptions then the relationship between these attributes could become muddy if different instrumentation makes different decisions about what values to use.

If we go down this road ("process.name" is the best known process name), we'd be:

  • prescriptive on how to populate it if instrumentation has no additional context
  • say that instrumentations that know better than that MAY populate it in different way. process.executable.name would be the precise one.
  • expect app developers to set whatever value they feel right.

Either way, prototyping and final stabilization push are usually good time to clean up descriptions and also if we don't believe that some of it is essential for stability, let's just not add it or let's keep it experimental.

@christos68k
Copy link
Member

I don't have a strong opinion, a few things we could consider to get unstuck:

  • not adding process.name for now. Sticking with process.executable.name, just changing its description to fix process.executable.name shouldn't use /proc/[pid]/status #1736
  • going forward with process.name, but leaving its description open and soft so it can evolve after attribute is stabilized.
  • going forward with process.name in whatever form, but excluding it from initial stability scope.

All of these options imply that we'd switch the semantics for process.executable.name to track the actual executable name. So if we keep being undecided on the semantics for process.name, I suggest we remove it from this PR and continue with process.executable.name . There is value in having improved semantics for that key and it would also unblock profiling work.

Either way, prototyping and final stabilization push are usually good time to clean up descriptions and also if we don't believe that some of it is essential for stability, let's just not add it or let's keep it experimental.

My preference for process.name would be to introduce it with simplicity in mind and not let the complexity arising out of deduplication derail us, at least initially.

@braydonk braydonk changed the title add process.name attribute and adapt process.executable.name adjust process.executable.name semantic expectation Feb 22, 2025
@braydonk
Copy link
Contributor Author

So if we keep being undecided on the semantics for process.name, I suggest we remove it from this PR and continue with process.executable.name.

That is now the purpose of this PR. process.name related stuff should all be removed, and only the change to the description of process.executable.name remains. I have opened #1928 to track adding process.name.

@lmolkova lmolkova moved this from In Review to Needs More Approval in Semantic Conventions Triage Feb 27, 2025
@lmolkova
Copy link
Member

@braydonk @christos68k @rockdaboot I think all the discussions can be resolved now and this PR should be ready to go.
Could you please check?

@lmolkova lmolkova moved this from Needs More Approval to Ready to be Merged in Semantic Conventions Triage Feb 27, 2025
@rockdaboot
Copy link

@braydonk @christos68k @rockdaboot I think all the discussions can be resolved now and this PR should be ready to go. Could you please check?

From my side yes, but don't have the permission to resolve my own comment 😃

@trask
Copy link
Member

trask commented Feb 27, 2025

but don't have the permission to resolve my own comment

TIL! You can resolve a conversation in a pull request if you opened the pull request or if you have write access to the repository where the pull request was opened

@christos68k
Copy link
Member

@braydonk @christos68k @rockdaboot I think all the discussions can be resolved now and this PR should be ready to go. Could you please check?

Thanks @lmolkova, I resolved the comment from @rockdaboot too. PR LGTM.

@braydonk
Copy link
Contributor Author

Should be good from my side since any open areas of concern have now been moved to #1928

@lmolkova lmolkova merged commit 10e3a0f into open-telemetry:main Feb 27, 2025
15 checks passed
@github-project-automation github-project-automation bot moved this from Ready to be Merged to Needs More Approval in Semantic Conventions Triage Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

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

6 participants