-
Notifications
You must be signed in to change notification settings - Fork 358
[Resources.Process] implement latest semantic convention spec #3347
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
base: main
Are you sure you want to change the base?
[Resources.Process] implement latest semantic convention spec #3347
Conversation
Co-authored-by: Martin Costello <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3347 +/- ##
==========================================
+ Coverage 71.16% 71.30% +0.14%
==========================================
Files 452 442 -10
Lines 17567 17530 -37
==========================================
- Hits 12501 12500 -1
+ Misses 5066 5030 -36
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| attributes.Add(new(ProcessSemanticConventions.AttributeProcessStartTime, currentProcess.StartTime.ToString("O") ?? DateTime.Now.ToString("O"))); | ||
| } | ||
| #endif | ||
| catch (Win32Exception) | ||
| { | ||
| attributes.Add(new(ProcessSemanticConventions.AttributeProcessStartTime, DateTime.Now.ToString("O"))); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fallbacks here are not true IMO. If you are not detect attributes, just omit it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that but in the case here creation time is an identifying attribute and all identifying should actually be required hence open-telemetry/weaver#986 to address it.
Also Using The current time enables the functionality of distinguishing between telemetry from process starts, leaving it out you lose that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lack of the data is better than the wrong data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you saying that all the process entity attributes should be omitted if the creation time can not be set given it is an identifying attribute?
For me the exact value of the atrribute is not as meaningful as having a value there which can then be used to achieve the use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not against keeping part of the attributes. Part of data seems to be fine. I am against putting obviously wrong data in the attributes/any other signal.
In this case, add this attribute conditionally if it is available. You can document it properly in the README. Small deviations from the semantics should be allowed if we have technical blockers for implementation.
| attributes.Add(new(ProcessSemanticConventions.AttributeProcessStartTime, currentProcess.StartTime.ToString("O") ?? DateTime.Now.ToString("O"))); | ||
| } | ||
| #endif | ||
| catch (Win32Exception) | ||
| { | ||
| attributes.Add(new(ProcessSemanticConventions.AttributeProcessStartTime, DateTime.Now.ToString("O"))); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lack of the data is better than the wrong data.
|
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. |
| new(ProcessSemanticConventions.AttributeProcessExecName, currentProcess.ProcessName), | ||
| new(ProcessSemanticConventions.AttributeProcessInteractive, Environment.UserInteractive), | ||
| #if NET | ||
| new(ProcessSemanticConventions.AttributeProcessPid, Environment.ProcessId), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You already have currentProcess.Id here. Is there any reason to go through Environment and potentially fetching process data from the scratch?
What is more, there will be common scenario both for .NET/non .NET
| { | ||
| new(ProcessSemanticConventions.AttributeProcessOwner, Environment.UserName), | ||
| new(ProcessSemanticConventions.AttributeProcessArgsCount, Environment.GetCommandLineArgs().Length), | ||
| new(ProcessSemanticConventions.AttributeProcessTitle, currentProcess.MainWindowTitle), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if it should be used here. Main windows title is not the process title. Check https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.process.mainwindowtitle?view=net-10.0
It is recommended attribute, so it can be omitted in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot have corrected how this is sourced.
| new(ProcessSemanticConventions.AttributeProcessOwner, Environment.UserName), | ||
| new(ProcessSemanticConventions.AttributeProcessArgsCount, Environment.GetCommandLineArgs().Length), | ||
| new(ProcessSemanticConventions.AttributeProcessTitle, currentProcess.MainWindowTitle), | ||
| new(ProcessSemanticConventions.AttributeProcessWorkingDir, Environment.CurrentDirectory), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based what on I see is not reliable and might change in time.
alternative approach currentProcess.StartInfo.WorkingDirectory is not working while I was testing.
As it is conditionally required and we have AttributeProcessArgsCount, I would drop this attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works for me in my testing. Resource attributes are allowed to change, they just need to be a descriptive attribute which this is. The description of the property mentions that this is the working directory.
| new(ProcessSemanticConventions.AttributeProcessTitle, currentProcess.MainWindowTitle), | ||
| new(ProcessSemanticConventions.AttributeProcessWorkingDir, Environment.CurrentDirectory), | ||
|
|
||
| new(ProcessSemanticConventions.AttributeProcessExecName, currentProcess.ProcessName), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure that it met SHOULD criteria from OTel sem. conv?
The name of the process executable. On Linux based systems, this SHOULD be set to the base name of the target of /proc/[pid]/exe. On Windows, this SHOULD be set to the base name of GetProcessImageFileNameW.
If not, it is conditionally required, and we have non-controversial in place AttributeProcessArgsCount. Potentially, it can be removed from this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have updated it to use the main module name.
| new(ProcessSemanticConventions.AttributeProcessWorkingDir, Environment.CurrentDirectory), | ||
|
|
||
| new(ProcessSemanticConventions.AttributeProcessExecName, currentProcess.ProcessName), | ||
| new(ProcessSemanticConventions.AttributeProcessInteractive, Environment.UserInteractive), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure that
https://learn.microsoft.com/en-us/dotnet/api/system.environment.userinteractive?view=net-10.0
is compliant with sem conf description: Whether the process is connected to an interactive shell.?
It can be omitted, as it is recommended attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but to further increase the checks, I now check if stdio is connected.
…//github.com/thompson-tomo/opentelemetry-dotnet-contrib into feature/#2892_ImplementRecomendedProcessAttr
Fixes #2892
Design discussion issue #
Changes
This adds in additional recommend attributes to describe process. Implementation has been based on latest spec https://github.com/open-telemetry/semantic-conventions/blob/main/docs/resource/process.md
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes