Send process name as process.executable.name (/proc/PID/comm) #298
Conversation
| @@ -641,6 +646,19 @@ func (pm *ProcessManager) CleanupPIDs() { | |||
| } | |||
| } | |||
|
|
|||
| // NameForPID returns the process name for given PID. | |||
There was a problem hiding this comment.
Depending on how we go about solving #278, we may move these (process name, executable path) somewhere else. For now, I left them as processInfo attributes.
| @@ -48,7 +48,8 @@ type Frame struct { | |||
|
|
|||
| type Trace struct { | |||
| Comm string | |||
| Executable string | |||
| ProcessName string | |||
There was a problem hiding this comment.
With this change there are two fields with roughly the same information. Comm originates from the eBPF space and is fetched with the helper bpf_get_current_comm() and ProcessName is fetched from procfs via /prod/%d/comm.
To some degree this is duplicate information - ignoring for the moment all the events that can change these values and cause differences in them.
Going forward, I think there should be either Comm or ProcessName but not both. If both fields should be kept, what is the point they bring in that makes them valuable?
There was a problem hiding this comment.
It's not the same information: Comm retrieved in the kernel is the thread name and /proc/PID/comm is the process name, they're sent to the backend under different keys.
The value they bring is more flexible aggregations: executable, process, thread. Why pick one or the other?
There was a problem hiding this comment.
Both values are kind of metadata to me and I'm wondering if there is something in OTel that enriches metadata based on a PID. The value from bpf_get_current_comm() might not be available to a OTel metadata enricher, but the rest should be. So overall, I'm thinking more about what components are the key functionality of profiling vs what enrichment can be added by other components.
There was a problem hiding this comment.
The reason we want to do this particular enrichment here that depends on process lifetime is because we're in full control of the metadata (process name, executable name) that is thrown away on process exit and can guarantee their availability (after we fix #278).
Co-authored-by: Tim Rühsen <[email protected]>
Summary
This PR adds support for sending process name as
process.executable.name, abiding by current OTel semantics for that key. Instead ofNamein/prod/PID/statusas semconv dictates, I'm using /proc/PID/comm which contains the same value and is simpler to parse.Regarding these semantics, I also started a discussion in semconv WG to improve clarity as
process.executable.nameis misleading/confusing/wrong depending on interpretation so we may end up changing the key in the future.