Add in process executable entity #2655 #2657
Conversation
| name: process.executable | ||
| brief: The executable which is being run by the process. | ||
| attributes: | ||
| - ref: process.executable.name |
There was a problem hiding this comment.
Can you select which ones of these will be identifying or descriptive?
There was a problem hiding this comment.
Hmm, should the identity attribute values change if the executable is updated to a new version? If so we would probs need an additional attribute for version or everything becomes identity.
There was a problem hiding this comment.
That's a question for how this entity will be used and tracked in the system.
As I think this entity is MOST relevant to @open-telemetry/profiling-maintainers - I'd like to get there take on what "identity" for an executable should be.
Do you want to group profiles by an executable by "concept" or "by its version"? I suspect it's the later...
There was a problem hiding this comment.
It could be used as either but what I will do is define it without a version (this PR) and create a seperate pr to introduce a new attribute and add it to the entity as identifying as I see a couple of options to address it.
There was a problem hiding this comment.
Seperate PR for adding an attribute for executable version #2673.
There was a problem hiding this comment.
In profiling, process.executable.build_id.htlhash is used to uniquely identify executables when 1-1 accuracy is needed (e.g. symbolization) but for other cases (e.g. visualization of flamegraphs) one may use process.executable.name, which introduces fuzziness (e.g. multiple different executables, which could be entirely unrelated functionality-wise, with the same executable name collapsed under one entity).
There was a problem hiding this comment.
It sounds to me like we want the executable to be identified by the build-id, and the name is a descriptive attirbute which can be used for grouping.
@braydonk / @christos68k - I think this is worth further discussion
There was a problem hiding this comment.
It's worth noting that it's not uncommon but possible for an executable to be renamed while a process is running. Based on the instrumentation instructions for this attribute, someone mving an executable during a process's runtime will result in a new name being retrieved. I haven't tried on Windows, but I'm pretty sure that renaming an executable while a process running will result in a new value from GetProcessImageFileNameW, so the same would apply if I'm remembering correctly (needs validation).
@braydonk Actually if the path to the executable is all that is changed and the file name renames the same the value of the name attribute wouldn't change.
I am more than happy to introduce a new attribute to use as the identifying attribute.
Howabout I set the name to descriptive in this PR or even leave it unset & we use #2673 for defining a new identifying attribute.
There was a problem hiding this comment.
I think it's important that the profiling SIG make the decision for what the identifying attribute(s) should be. Based on my discussion with them we at least agreed on the name not being identifying so we'll start with that, and it seems like the attribute @christos68k mentioned above is likely the best candidate, but it sounded like the Profiling SIG had other things to work through before making the final decision if I understood correctly.
There was a problem hiding this comment.
Sure I have made the name attribute to be descriptive and the htlhash to not have a role so that we can discuss seperately if htlhash is the identity and the description is updated. Let me know if this PR nneeds any additional changes.
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Sig have approved and requested extension has been done. Awaiting feedback. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
thompson-tomo
force-pushed
the
feature/#2655_process_executable
branch
from
ee4a4a7 to
60c08f2
Compare
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
thompson-tomo
force-pushed
the
feature/#2655_process_executable
branch
3 times, most recently
from
bf789e3 to
b6c341c
Compare
thompson-tomo
force-pushed
the
feature/#2655_process_executable
branch
from
5d55abf to
d1ebd89
Compare
5 participants
Progresses #2655
Changes
This introduces a new process executable entity which describes the executable being run by the process.
This entity is designed to be consistent across all instances regardless of machine or where files are placed.
Note: if the PR is touching an area that is not listed in the existing areas, or the area does not have sufficient domain experts coverage, the PR might be tagged as experts needed and move slowly until experts are identified.
Merge requirement checklist
[chore]