[RESOURCE] Process resource detector reports more attributes - #4437
Conversation
dbarker
left a comment
dbarker
left a comment
There was a problem hiding this comment.
Thanks for the PR! Please see feedback below.
…o with other minor changes
|
@dbarker Thanks for the review and feedback! I've made the changes
|
There was a problem hiding this comment.
Pull request overview
Adds missing required/recommended Process Entity and Process Executable Entity semantic convention attributes to the Process Resource Detector, with supporting utilities, tests, and documentation updates.
Changes:
- Extends process detection to populate
process.creation.time,process.owner,process.executable.build_id.htlhash, andprocess.executable.name(when available). - Refactors executable discovery utility to return both path + basename, and adds utilities for creation time / owner / deterministic build ID.
- Adds unit + integration-style tests and updates the resource detector README and header docstrings accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| resource_detectors/test/process_detector_test.cc | Adds new utility tests and an integration test asserting expected resource attributes are present. |
| resource_detectors/src/process_detector.cc | Populates the new process attributes in Detect() using the new utility APIs. |
| resource_detectors/src/process_detector_utils.cc | Implements executable info, creation time, owner lookup, and htlhash build ID computation. |
| resource_detectors/README.md | Documents added attributes and platform limitations (notably macOS executable info). |
| resource_detectors/include/opentelemetry/resource_detectors/process_detector.h | Updates public detector documentation to describe newly populated attributes. |
| resource_detectors/include/opentelemetry/resource_detectors/detail/process_detector_utils.h | Declares new utility APIs (ExecutableInfo, creation time, owner, htlhash, SHA-256 helper). |
Suppressed comments (1)
resource_detectors/include/opentelemetry/resource_detectors/detail/process_detector_utils.h:81
- GetProcessOwner() has no pid parameter, but the doc comment includes an
@parampid entry. This is confusing for users of this header (even under detail/).
*
* @param pid Process ID.
*/
std::string GetProcessOwner();
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| std::ifstream f(exe_path, std::ios::binary | std::ios::ate); | ||
| if (!f.is_open()) | ||
| { | ||
| return std::string(); | ||
| } | ||
|
|
||
| auto file_size = static_cast<std::uint64_t>(f.tellg()); | ||
|
|
| std::string expected_path = opentelemetry::resource_detector::detail::GetExecutableInfo(pid).path; | ||
| EXPECT_EQ(path, expected_path); |
|
Thanks for the review! I'm back to this now. |
marcalff
left a comment
marcalff
left a comment
There was a problem hiding this comment.
Thanks for the patch.
See a concern about the embedded SHA256 implementation.
|
Updated this to use openssl and removed the embedded SHA256 implementation. I've pushed the changes for another look. |
dbarker
left a comment
dbarker
left a comment
There was a problem hiding this comment.
@LevelVoid Thanks for the changes. Adding the openssl dependency is a major change to the scope and impact of this PR (#4437 (comment)). Based on this I'm requested a change to break out the SHA256 utils and calculation of the process.executable.build_id.htlhash to a separate PR.
This PR without the process.executable.build_id.htlhash is very valuable because it still adds theprocess.creation.time (which completes the process entity identity) and the recommend attributes for process and process.executable.
Yes, I agree it should be done separately. The issue with SHA256 is not technical, it is legal: opentelemetry-cpp can not have crytographic code, even if it is stable and public. |
marcalff
left a comment
marcalff
left a comment
There was a problem hiding this comment.
Code depending on SHA256 should be moved in a different PR.
|
Got it. I'll remove the |
dbarker
left a comment
dbarker
left a comment
There was a problem hiding this comment.
Thanks for the updates! This is a nice addition.
|
Thanks for the review! |
|
@marcalff I've addressed the requested changes, and it's ready for another look. |
marcalff
changed the title
marcalff
changed the title
marcalff
left a comment
marcalff
left a comment
There was a problem hiding this comment.
LGTM, thanks for the feature.
There is a lot of platform specific code, but this is hard to avoid.
The process resource detector is valuable for this very reason, nice work.
|
Thanks for the thorough reviews and guidance throughout this. I learned a lot working through the platform-specific edge cases. |
Fixes #4436
Changes
process.creation.time,process.owner, (deferred:process.executable.build_id.htlhash) andprocess.executable.namewith declaration inresource_detectors/include/opentelemetry/resource_detectors/detail/process_detector_utils.hand implementation inresource_detectors/src/process_detector_utils.cc.resource_detectors/src/process_detector.cc.resource_detectors/test/process_detector_test.cc.Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes