Add required and recommended 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.
| #include <fstream> | ||
| #include <string> | ||
| #include <vector> | ||
|
|
||
| #ifdef _MSC_VER | ||
| // clang-format off | ||
| # include <windows.h> | ||
| # include <psapi.h> | ||
| # include <shellapi.h> | ||
| # pragma comment(lib, "shell32.lib") | ||
| // clang-format on | ||
| #else | ||
| # include <sys/types.h> | ||
| # include <unistd.h> | ||
| # include <cstdio> | ||
| #endif |
| * Retrieves the absolute file system path and the base name of the process executable. | ||
| * Platform-specific behavior: | ||
| * - Windows: Uses OpenProcess() + GetProcessImageFileNameW(). | ||
| * - Linux/Unix: Reads the /proc/<pid>/exe symbolic link. |
| 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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4437 +/- ##
==========================================
+ Coverage 83.09% 83.17% +0.08%
==========================================
Files 519 519
Lines 20253 20390 +137
==========================================
+ Hits 16827 16957 +130
- Misses 3426 3433 +7
🚀 New features to boost your workflow:
|
dbarker
left a comment
dbarker
left a comment
There was a problem hiding this comment.
Thanks for fixing the CI issues. Please see some feedback below on the process.executable.build_id.htlhash attribute value calculation.
|
Thanks for the review and feedback! Learnt a lot working through this. |
| // Minimal self-contained SHA-256 implementation. | ||
| // Reference: FIPS 180-4 | ||
| struct Sha256Context |
There was a problem hiding this comment.
This is a duplicated implementation of SHA256, that already exists elsewhere.
This code will never be maintained, and now opentelemetry-cpp contains crytographic software, which has huge legal implications.
Please remove entirely, and add a dependency to openssl instead.
opentelemetry-cpp already depends on curl, which already depends on openssl, so this is not a big change overall for the final application, only a change in makefiles.
There was a problem hiding this comment.
Thanks for raising this. We could take a dependency on openssl, that is a bit heavy though and may need to be installed by users just for this use case. CURL is optional and many don't install it. There may be other lighter weight dependencies we can consider too.
My feeling is that managing the dependency will be more maintenance for otelcpp and users of this simple detector.
My understanding is that the SHA256 algorithm is fixed/frozen as published by NIST (linked below) and won't require changes to maintain. It's use in otelcpp is very limited to only produce the executable ID required by the semantic conventions and no user can call the methods directly as the headers are not installed.
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. |
Fixes #4436
Changes
process.creation.time,process.owner,process.executable.build_id.htlhashandprocess.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