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

fix(instrumentation): normalize paths for internal files in scoped packages #4467

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Feb 8, 2024

Which problem is this PR solving?

See #4436. On Windows, internal files were not instrumented as this check was failing.

This happens, as require-in-the-middle returns the package name concatenated with a path seperator (/ or \ depending on platform).

On scoped packages, this turns out to be something like @opentelemetry/test-package\src\internal.js (note the mixed \ and /). To find the files to patch, this check is performed in the node-specific instrumentation base class. As the string for name in the module's files is then @opentelemetry\test-package\src\internal.js (note: all path seperators are \), this check fails, and all files are filtered out.

While it would be possible to normalize the name passed to InstrumentationBase._onRequire() from RequireInTheMiddleSingleton, this would also normalize main module names like @opentelemetry/test-package to @opentelemetry\test-package, which means that internal files will be instrumented, but the main module will not.

Therefore this PR adds normalization of the name used during filtering instead of directly in RequireInTheMiddleSingleton.

Fixes #4436
Fixes #4402
Related open-telemetry/opentelemetry-js-contrib#1925

Short description of the changes

  • normalizes path returned by require-in-the-middle hook before checking the name against module file names

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Added tests
  • Manual testing on Windows

@pichlermarc pichlermarc changed the title fix(instrumentation): normalize paths for internal files in scoped pa… fix(instrumentation): normalize paths for internal files in scoped packages Feb 8, 2024
Copy link

codecov bot commented Feb 8, 2024

Codecov Report

Merging #4467 (e42f184) into main (f86251d) will not change coverage.
Report is 1 commits behind head on main.
The diff coverage is n/a.

❗ Current head e42f184 differs from pull request most recent head 19338a4. Consider uploading reports for the commit 19338a4 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4467   +/-   ##
=======================================
  Coverage   92.42%   92.42%           
=======================================
  Files         330      330           
  Lines        9520     9520           
  Branches     2031     2031           
=======================================
  Hits         8799     8799           
  Misses        721      721           

@pichlermarc pichlermarc marked this pull request as ready for review February 8, 2024 15:39
@pichlermarc pichlermarc requested a review from a team February 8, 2024 15:39
@pichlermarc pichlermarc added bug Something isn't working priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect pkg:instrumentation labels Feb 8, 2024
@trentm
Copy link
Contributor

trentm commented Feb 9, 2024

On scoped packages, this turns out to be something like @opentelemetry/test-package\src\internal.js (note the mixed \ and /).

Ew. RITM isn't being very nice here.

@pichlermarc pichlermarc merged commit 01348e6 into open-telemetry:main Feb 12, 2024
16 of 17 checks passed
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
…ckages (open-telemetry#4467)

* fix(instrumentation): normalize paths for internal files in scoped packages

* fix(instrumentation): normalize name passed to onRequire in RequireInTheMiddleSingleton

* fix(instrumentation): apply normalization during filtering internal files

* fix(changelog): add changelog entry

* fix: normalize before filtering

* fix: lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pkg:instrumentation priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect
Projects
None yet
3 participants