-
Notifications
You must be signed in to change notification settings - Fork 805
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
fix(instrumentation): normalize paths for internal files in scoped packages #4467
Conversation
Codecov Report
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 |
Ew. RITM isn't being very nice here. |
…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
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 forname
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()
fromRequireInTheMiddleSingleton
, 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
require-in-the-middle
hook before checking the name against module file namesType of change
How Has This Been Tested?