-
Notifications
You must be signed in to change notification settings - Fork 357
[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?
Changes from all commits
5bfc0cf
6316cca
ae97c1c
44d0dd8
4fede6b
36b62f4
6d77f69
415c7dc
c714d56
74eabc0
ae2e468
7fb1252
c8d5ee1
5bef668
247870d
f42b41e
bf8cbcf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| // Copyright The OpenTelemetry Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| using System.ComponentModel; | ||
|
|
||
| namespace OpenTelemetry.Resources.Process; | ||
|
|
||
| /// <summary> | ||
|
|
@@ -14,20 +16,39 @@ internal sealed class ProcessDetector : IResourceDetector | |
| /// <returns>Resource with key-value pairs of resource attributes.</returns> | ||
| public Resource Detect() | ||
| { | ||
| return new Resource(new List<KeyValuePair<string, object>>(2) | ||
| using var currentProcess = System.Diagnostics.Process.GetCurrentProcess(); | ||
|
|
||
| if (currentProcess.HasExited) | ||
| { | ||
| return Resource.Empty; | ||
| } | ||
|
|
||
| var attributes = new List<KeyValuePair<string, object>>(9) | ||
| { | ||
| new(ProcessSemanticConventions.AttributeProcessOwner, Environment.UserName), | ||
| new(ProcessSemanticConventions.AttributeProcessArgsCount, Environment.GetCommandLineArgs().Length), | ||
| new(ProcessSemanticConventions.AttributeProcessTitle, currentProcess.MainWindowTitle), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| new(ProcessSemanticConventions.AttributeProcessWorkingDir, Environment.CurrentDirectory), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. As it is conditionally required and we have AttributeProcessArgsCount, I would drop this attribute. |
||
|
|
||
| new(ProcessSemanticConventions.AttributeProcessExecName, currentProcess.ProcessName), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you sure that it met SHOULD criteria from OTel sem. conv? If not, it is conditionally required, and we have non-controversial in place AttributeProcessArgsCount. Potentially, it can be removed from this PR. |
||
| new(ProcessSemanticConventions.AttributeProcessInteractive, Environment.UserInteractive), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you sure that It can be omitted, as it is recommended attribute. |
||
| #if NET | ||
| new(ProcessSemanticConventions.AttributeProcessPid, Environment.ProcessId), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You already have What is more, there will be common scenario both for .NET/non .NET |
||
| }); | ||
| new(ProcessSemanticConventions.AttributeProcessExecPath, Environment.ProcessPath ?? string.Empty), | ||
| }; | ||
| #else | ||
| new(ProcessSemanticConventions.AttributeProcessPid, GetProcessPid()), | ||
| }); | ||
| static int GetProcessPid() | ||
| new(ProcessSemanticConventions.AttributeProcessPid, currentProcess.Id), | ||
| }; | ||
| #endif | ||
|
|
||
| try | ||
| { | ||
| using var process = System.Diagnostics.Process.GetCurrentProcess(); | ||
| return process.Id; | ||
| attributes.Add(new(ProcessSemanticConventions.AttributeProcessStartTime, currentProcess.StartTime.ToString("O"))); | ||
| } | ||
| #endif | ||
| catch (Win32Exception) | ||
| { | ||
| } | ||
|
|
||
| return new Resource(attributes); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,7 +53,9 @@ using var loggerFactory = LoggerFactory.Create(builder => | |
| The resource detectors will record the following metadata based on where | ||
| your application is running: | ||
|
|
||
| - **ProcessDetector**: `process.owner`, `process.pid`. | ||
| - **ProcessDetector**: `process.owner`, `process.pid`, `process.executable.path`, | ||
| `process.working_directory`, `process.args_count`,`process.creation.time`, | ||
| `process.executable.name`, `process.interactive`, `process.title`. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lack of info that process.executable.name is avaialbe only on .NET |
||
|
|
||
| ## References | ||
|
|
||
|
|
||
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.