Skip to content

Conversation

nikhilbhatia08
Copy link
Contributor

Fixes # (issue)

Changes

implements

  • process.command_args
  • process.command_line
    process.command_args attribute should return a list with command and full list of arguments so changed the implementation of process.command as it should only get the first item that is command from that list.

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 22, 2025 12:48
Copy link

netlify bot commented Aug 22, 2025

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

Name Link
🔨 Latest commit a915a3d
🔍 Latest deploy log https://app.netlify.com/projects/opentelemetry-cpp-api-docs/deploys/68a8a0b088891f00082eca19
Copy link

codecov bot commented Aug 22, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.05%. Comparing base (6bc8349) to head (7b7ba30).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@ 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 

see 2 files with indirect coverage changes

🚀 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 29, 2025 01:10
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 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.

@nikhilbhatia08 nikhilbhatia08 requested a review from owent September 2, 2025 00:34
@marcalff marcalff self-assigned this Sep 3, 2025
@lalitb
Copy link
Member

lalitb commented Sep 5, 2025

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.

@nikhilbhatia08
Copy link
Contributor Author

@lalitb otel-js and otel-go sdk have process resource detectors and they do not have a filter, but yeah we could implement one.

@lalitb
Copy link
Member

lalitb commented Sep 5, 2025

@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 --password/secret=<value> or --password/secret <value>. I don’t think there’s a fool-proof way to mask everything, but handling the obvious scenarios would be good.

@nikhilbhatia08
Copy link
Contributor Author

nikhilbhatia08 commented Sep 5, 2025

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 --PASS=some_pass_key --SOME_ARG=some_value then when the user is using the process detector then the user can pass a list of args they want to scrub, so the user for example provides {"PASS"} then we can give the args list as {"--PASS=*****", "--SOME_ARG=some_value"} so that the user has full control over which args they want to scrub and which they want to show. Does this idea look good @lalitb ? And Can this be in another PR?

@lalitb
Copy link
Member

lalitb commented Sep 5, 2025

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 --PASS=some_pass_key --SOME_ARG=some_value then when the user is using the process detector then the user can pass a list of args they want to scrub, so the user for example provides {"PASS"} then we can give the args list as {"--PASS=*****", "--SOME_ARG=some_value"} so that the user has full control over which args they want to scrub and which they want to show. Does this idea look good @lalitb ? And Can this be in another PR?

Yes, configuring resource keys to be scrubbed as part of this detector ctor would be right fix, as separate PR.

@nikhilbhatia08
Copy link
Contributor Author

Ready To merge :)

@nikhilbhatia08
Copy link
Contributor Author

nikhilbhatia08 commented Sep 6, 2025

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 --PASS=some_pass_key --SOME_ARG=some_value then when the user is using the process detector then the user can pass a list of args they want to scrub, so the user for example provides {"PASS"} then we can give the args list as {"--PASS=*****", "--SOME_ARG=some_value"} so that the user has full control over which args they want to scrub and which they want to show. Does this idea look good @lalitb ? And Can this be in another PR?

@dbarker what do you think of this idea as a process for sanitization ?

@dbarker
Copy link
Member

dbarker commented Sep 6, 2025

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 --PASS=some_pass_key --SOME_ARG=some_value then when the user is using the process detector then the user can pass a list of args they want to scrub, so the user for example provides {"PASS"} then we can give the args list as {"--PASS=*****", "--SOME_ARG=some_value"} so that the user has full control over which args they want to scrub and which they want to show. Does this idea look good @lalitb ? And Can this be in another PR?

@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.

@dbarker
Copy link
Member

dbarker commented Sep 6, 2025

Thanks for the PR @nikhilbhatia08. I am requesting two changes:

  1. comment out setting the process-command-args attribute for now until there is a santization feature,
  2. remove setting the process-command-line attribute (and utility function) since the spec implies we should not assemble it just for monitoring and should use the process-command-args attribute instead.
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.

LGTM. Thanks!

@marcalff marcalff merged commit d660b3b into open-telemetry:main Sep 7, 2025
66 checks passed
@nikhilbhatia08 nikhilbhatia08 deleted the process_detector_attr branch September 7, 2025 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

6 participants