implement latest semconv spec for process resource #2892 #3347
Conversation
Co-authored-by: Martin Costello <martin@martincostello.com>
github-actions
bot
added
the
comp:resources.process
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3347 +/- ##
==========================================
- Coverage 70.79% 69.50% -1.29%
==========================================
Files 440 386 -54
Lines 17265 15581 -1684
==========================================
- Hits 12222 10829 -1393
+ Misses 5043 4752 -291
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
thompson-tomo
changed the title
| new(ProcessSemanticConventions.AttributeProcessStartTime, currentProcess.StartTime.ToString("O") ?? string.Empty), | ||
| new(ProcessSemanticConventions.AttributeProcessTitle, currentProcess.MainWindowTitle), | ||
| new(ProcessSemanticConventions.AttributeProcessWorkingDir, Environment.CurrentDirectory), | ||
|
|
||
| new(ProcessSemanticConventions.AttributeProcessExecName, currentProcess.ProcessName), |
There was a problem hiding this comment.
Some process related properties may throw exceptions. For example, StartTime throws 3 exceptions, and there is likely no way to detect errors like Win32Exception ahead of time.
There was a problem hiding this comment.
Looking at the exceptions the only one we are interested in is win32 so have added a try catch and also exited earlier if process has exited.
The others are for scenarios we don't support ie windows 98.
The current directory does through a number of exceptions but they are for the set method which is not used.
| 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.
Fallbacks here are not true IMO. If you are not detect attributes, just omit it.
There was a problem hiding this comment.
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.
Lack of the data is better than the wrong data.
There was a problem hiding this comment.
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.
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.
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. |
4 participants
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