Skip to content

Conversation

nikhilbhatia08
Copy link
Contributor

@nikhilbhatia08 nikhilbhatia08 commented Aug 13, 2025

Fixes # (issue)

Changes

Implements process detector currently for attributes - [process.pid, process.executable.path, process.command], the next pull request will implement more of the process attributes. Added tests.

reference - https://github.com/open-telemetry/semantic-conventions/blob/main/docs/resource/process.md#selecting-process-attributes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@nikhilbhatia08 nikhilbhatia08 requested a review from a team as a code owner August 13, 2025 16:31
Copy link

netlify bot commented Aug 13, 2025

Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name Link
🔨 Latest commit 66c4b49
🔍 Latest deploy log https://app.netlify.com/projects/opentelemetry-cpp-api-docs/deploys/68a55e550e24a50008814e86

Copy link

codecov bot commented Aug 13, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.03%. Comparing base (42050d0) to head (66c4b49).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3591   +/-   ##
=======================================
  Coverage   90.03%   90.03%           
=======================================
  Files         220      220           
  Lines        7069     7069           
=======================================
  Hits         6364     6364           
  Misses        705      705           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@lalitb lalitb requested a review from Copilot August 14, 2025 07:29
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements a process resource detector for OpenTelemetry that extracts process-related attributes according to semantic conventions. It adds functionality to detect process ID, executable path, and command line information for the current process.

  • Adds ProcessResourceDetector class that implements the ResourceDetector interface
  • Implements utility functions for cross-platform process information extraction (Windows/Unix)
  • Provides comprehensive test coverage for the new functionality

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
resource_detectors/include/opentelemetry/resource_detectors/process_detector.h Defines the ProcessResourceDetector class interface
resource_detectors/include/opentelemetry/resource_detectors/process_detector_utils.h Declares utility functions for process information extraction
resource_detectors/process_detector.cc Implements the main ProcessResourceDetector functionality
resource_detectors/process_detector_utils.cc Implements cross-platform process information extraction utilities
resource_detectors/test/process_detector_test.cc Adds unit tests for the process detector utilities
resource_detectors/test/CMakeLists.txt Updates test build configuration to include new test file
resource_detectors/test/BUILD Updates Bazel build configuration for tests
resource_detectors/CMakeLists.txt Updates library build configuration to include new source files
resource_detectors/BUILD Updates Bazel build configuration for the library

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@nikhilbhatia08
Copy link
Contributor Author

nikhilbhatia08 commented Aug 14, 2025

The tests on the latest commit fail because psapi.h is included first and then windows.h, but if we try to include windows.h first and then include psapi.h then it would fail the format test. What can be done for this any suggestions ?

@lalitb
Copy link
Member

lalitb commented Aug 14, 2025

The tests on the latest commit fail because psapi.h is included first and then windows.h, but if we try to include windows.h first and then include psapi.h then it would fail the format test. What can be done for this any suggestions ?

What is the format error? You switch off the clang/iwyu check for that portion of the include, depending on the error you are getting.

@nikhilbhatia08
Copy link
Contributor Author

If I run format of ci then I'm getting this error

The following files have changes:
resource_detectors/process_detector_utils.cc
diff --git a/resource_detectors/process_detector_utils.cc b/resource_detectors/process_detector_utils.cc
index cb6e064..d20f367 100644
--- a/resource_detectors/process_detector_utils.cc
+++ b/resource_detectors/process_detector_utils.cc
@@ -7,8 +7,8 @@
 #include <string>
 
 #ifdef _MSC_VER
-#  include <windows.h>
 #  include <psapi.h>
+#  include <windows.h>
 #else
 #  include <sys/types.h>
 #  include <unistd.h>

So if I include psapi.h first and then windows.h, the windows.h header redefines some macros present in psapi.h, that is why windows.h should be included first, but doing so gives the above format error.

But now I got how to disable the format error for this part of code. Thanks 👍🏻 !

@dbarker
Copy link
Member

dbarker commented Aug 18, 2025

Thanks for the PR! Please see comments above.

@nikhilbhatia08
Copy link
Contributor Author

nikhilbhatia08 commented Aug 18, 2025

For the tests added now and resource detectors tests in general we need to enable the tests -DWITH_RESOURCE_DETECTORS_PREVIEW=ON or else they will go untested as the actions ran in the previous commit.

Copy link
Member

@dbarker dbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks!

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looks good, just requesting change for this - as it can lead to crash.

@lalitb lalitb merged commit 601b4ed into open-telemetry:main Aug 20, 2025
70 checks passed
@nikhilbhatia08 nikhilbhatia08 deleted the implement_process_detector branch August 21, 2025 05:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants