fix(instrumentation): add back support for absolute paths via require-in-the-middle

[mhassan1]

Which problem is this PR solving?

In #3161, we replaced require-in-the-middle (RITM) with RequireInTheMiddleSingleton; one of the differences between the two is that RequireInTheMiddleSingleton does not support module names with absolute paths (i.e. what @opentelemetry/instrumentation-aws-lambda uses). This PR adds back support for module names with absolute paths.

Fixes open-telemetry/opentelemetry-js-contrib#1285

Short description of the changes

This PR adds fallback behavior: when an instrumentation plugin wants to instrument a module that is an absolute path, we will fall back to using RITM directly. The downside is that this will create additional instances of RITM, which we want to avoid, but @opentelemetry/instrumentation-aws-lambda is the only plugin that uses an absolute path, and we don't anticipate others.

In open-telemetry/opentelemetry-js-contrib#1285, we discussed a few options for resolving the need to instrument absolute paths. This PR represents option 2:

In @opentelemetry/instrumentation, handle this scenario by creating a new instance of require-in-the-middle, passing the absolute path

Type of change

Please delete options that are not relevant.

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

How Has This Been Tested?

I have tested this against @opentelemetry/instrumentation-aws-lambda and other instrumentation plugins.

Checklist:

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

[codecov]

Codecov Report

Merging #3457 (f11f703) into main (91f94a8) will increase coverage by 0.37%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3457      +/-   ##
==========================================
+ Coverage   93.37%   93.75%   +0.37%     
==========================================
  Files         248      248              
  Lines        7547     7552       +5     
  Branches     1574     1576       +2     
==========================================
+ Hits         7047     7080      +33     
+ Misses        500      472      -28     

Impacted Files	Coverage Δ
...strumentation/src/platform/node/instrumentation.ts	89.58% <100.00%> (+25.84%)	⬆️
...-trace-base/src/platform/node/RandomIdGenerator.ts	87.50% <0.00%> (-6.25%)	⬇️
...atform/node/instrumentationNodeModuleDefinition.ts	100.00% <0.00%> (+85.71%)	⬆️

[Flarna]

Is it possible to add a test for absolute path support?

[mhassan1]

@Flarna I've added a couple tests of enable/disable, which includes a test for absolute path support.

[vmarchaud]

i'm not sure to see why we can't handle absolute path with the singleton ? wouldn't it be as simple as checking for exact match here ?

[mhassan1]

i'm not sure to see why we can't handle absolute path with the singleton ? wouldn't it be as simple as checking for exact match here ?

Unfortunately, not. I explained it here. It is most likely a bug in RITM.

[weyert]

Does that mean we are dropping the singleton implementation? If so, can’t we put this change behind a flag that people using AWS Lambda can enable? The singleton really sped up startup times for me by minutes don’t want to loose it

Looks like it’s only happening in this specific case, sweet :):):)

[dyladan]

This should make it so that the AWS lambda instrumentation can keep using this module right?

[mhassan1]

This should make it so that the AWS lambda instrumentation can keep using this module right?

Exactly.