[resource_detectors] implementation of remaining process attributes #3603
Conversation
✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3603 +/- ##
==========================================
- Coverage 90.08% 90.05% -0.02%
==========================================
Files 220 220
Lines 7081 7081
==========================================
- Hits 6378 6376 -2
- Misses 703 705 +2 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements the remaining process attributes process.command_args and process.command_line for the OpenTelemetry resource detector. It refactors the existing command extraction logic to return a vector of command arguments instead of just the command string, enabling proper separation of the command from its arguments.
- Refactored command extraction to return command with arguments as a vector
- Added utility function to convert command arguments vector to command line string
- Updated Windows implementation to use
CommandLineToArgvW()for proper argument parsing
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| resource_detectors/include/opentelemetry/resource_detectors/detail/process_detector_utils.h | Updated function signatures and documentation for command argument extraction |
| resource_detectors/process_detector_utils.cc | Refactored command extraction functions to handle command arguments and added string conversion utility |
| resource_detectors/process_detector.cc | Updated to use new command argument extraction and populate all three process attributes |
| resource_detectors/test/process_detector_test.cc | Updated tests to verify command argument extraction and added new test cases |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Do the specs say anything about scrubbing or masking args that may contain secrets/passwords? Right now we’d be storing and exporting in plain text. Probably worth checking what other language SDKs are doing here too. |
|
@lalitb otel-js and otel-go sdk have process resource detectors and they do not have a filter, but yeah we could implement one. |
|
@nikhilbhatia08 see if java is doing something - https://github.com/search?q=org%3Aopen-telemetry+process.command+scrub&type=code . Can we add a similar pass here? At least cover the basic cases like |
|
Yeah we can a filter. Actually when thinking of this we can give the user the choice of what needs to be scrubbed and what not. For example |
Yes, configuring resource keys to be scrubbed as part of this detector ctor would be right fix, as separate PR. |
|
Ready To merge :) |
@dbarker what do you think of this idea as a process for sanitization ? |
I like the idea of implementing it in a new PR and moving the design discussion there. How about starting with a proposal in a new issue? We can review the spec and other language SDKs to see if there is any guidance. |
|
Thanks for the PR @nikhilbhatia08. I am requesting two changes:
|
Fixes # (issue)
Changes
implements
process.command_argsprocess.command_lineprocess.command_argsattribute should return a list with command and full list of arguments so changed the implementation ofprocess.commandas it should only get the first item that is command from that list.Please provide a brief description of 8000 the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes