Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: collect additional process attributes #3605

Merged
merged 14 commits into from
Mar 17, 2023

Conversation

mwear
Copy link
Member

@mwear mwear commented Feb 11, 2023

Which problem is this PR solving?

This PR updates the ProcessDetector to collect three additional attributes: process.command_args, process.owner, and process.executable_path. We currently collect process.command_line, which we could technically drop. The spec has this to say about process.command_args vs process.command_line:

Between process.command_args and process.command_line, usually process.command_args should be preferred. On Windows and other systems where the native format of process commands is a single string, process.command_line can additionally (or instead) be used.

We can keep both, or if we want only one, it should be process.command_args.

This will likely have to be updated when #3460 merges.

Fixes #3604

Short description of the changes

Updates the ProcessDetector to collect:

  • process.command_args
  • process.owner
  • process.executable_path

Two Additional Changes

  • Browser and Node used to share a ProcessorDetector implementation. We now export a noopDetector for browser and have moved the operational implementation to be node specific.
  • The detector used to return an empty resource if it did not collect all attributes. Based on the type definitions and the implementation of the detector, there will always be a meaningful process resource that is spec compliant to return. I removed the validation logic that would potential discard a valid process resource.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • unit tests
  • manual testing

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@mwear mwear requested a review from a team as a code owner February 11, 2023 00:28
@codecov
Copy link

codecov bot commented Feb 11, 2023

Codecov Report

Merging #3605 (1d346ea) into main (18ad56f) will decrease coverage by 0.41%.
The diff coverage is 92.85%.

❗ Current head 1d346ea differs from pull request most recent head 00f7641. Consider uploading reports for the commit 00f7641 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3605      +/-   ##
==========================================
- Coverage   94.09%   93.68%   -0.41%     
==========================================
  Files         263      277      +14     
  Lines        7625     8445     +820     
  Branches     1608     1753     +145     
==========================================
+ Hits         7175     7912     +737     
- Misses        450      533      +83     
Impacted Files Coverage Δ
...ges/opentelemetry-resources/src/detectors/index.ts 100.00% <ø> (ø)
...try-resources/src/platform/node/ProcessDetector.ts 100.00% <ø> (ø)
...resources/src/platform/node/ProcessDetectorSync.ts 92.85% <92.85%> (ø)

... and 15 files with indirect coverage changes

This commit introduces a slight change in behavior. Previously the
ProcessDectector would validate that it received attributes that it
expected, and returned an empty Resource if it didn't. Based on the
types, and the implementation of the ProcessDetector, there will
always be a valid ProcessResource. Collection of many of the attributes
can be done as a best effort without violating the specification.
@mwear mwear force-pushed the process-detector-improvements branch from 42f74b3 to eb0a60f Compare February 15, 2023 03:53
@mwear
Copy link
Member Author

mwear commented Feb 15, 2023

Rebased to pick up the changes in #3460.

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.

Collect Additional Process Attributes
6 participants