feat(instrumentation): implement require-in-the-middle singleton

[mhassan1]

Which problem is this PR solving?

This PR changes the behavior of instrumentation plugins to create a single require patch, instead of one per plugin. This should be a significant performance improvement for projects using many instrumentation plugins.

Fixes #3029.

Short description of the changes

This PR creates a require-in-the-middle singleton that looks for registered onRequire functions for a given module and executes them.

Type of change

• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

This PR is mostly covered by existing tests in InstrumentationBase.test.ts. It adds unit tests for the logic in the _shouldHook function.

Checklist:

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

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: mhassan1 / name: Marc Hassan (d5e4bf2)

[weyert]

Awesome, this sounds promising, thank you for the PR :))

[codecov]

Codecov Report

Merging #3161 (ed6e3ef) into main (0d4c71f) will increase coverage by 0.02%.
The diff coverage is 96.82%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3161      +/-   ##
==========================================
+ Coverage   93.44%   93.47%   +0.02%     
==========================================
  Files         241      243       +2     
  Lines        7260     7321      +61     
  Branches     1507     1517      +10     
==========================================
+ Hits         6784     6843      +59     
- Misses        476      478       +2     

Impacted Files	Coverage Δ
...n/src/platform/node/RequireInTheMiddleSingleton.ts	92.85% <92.85%> (ø)
...nstrumentation/src/platform/node/ModuleNameTrie.ts	100.00% <100.00%> (ø)
...strumentation/src/platform/node/instrumentation.ts	64.13% <100.00%> (+0.39%)	⬆️

[dyladan]

Love this idea. Very excited for this as its something we've been considering for a while

[weyert]

How can we progress this PR? I am really interested to speed up the start up of my projects now feels like it takes a few minutes

[dyladan]

I am also interested in getting this PR across the finish line. We are receiving more complaints now that we have more and more instrumentations in auto-instrumentations packages (example: #3229 (reply in thread))

[dyladan]

@mhassan1 curious what is required for you to bring this PR into a state ready for review? I have been getting increasing questions about startup cost and this would be a huge win for us.

[mhassan1]

I've moved it out of Draft. I see some failing checks that I will address now.

[weyert]

Awesome @mhassan1 looking forward trying it out :)

[mhassan1]

I've cleaned up the PR and added more tests. This is ready for review.

[mhassan1]

Unit tests are failing. I will look into it.

[dyladan]

With this change we are making a hard promise never to change the interface of this because of the global singleton. IMO we should upgrade this to 1.0 in order to make this promise explicit. @open-telemetry/javascript-maintainers wdyt

[dyladan]

I was indeed involved in that PR. It was required in order to instrument aws lambda using a layer.

Thanks for the research. Sounds like a huge performance improvement. Did you happen to step through to see if it is possible to work around the issue in the aws lambda or did you just run the test? If we're intercepting all require calls, shouldn't we be able to match on an exact path the same way ritm does?

[mhassan1]

I don't think there's anything that @opentelemetry/instrumentation-aws-lambda can do as a workaround, since RITM will never call its onRequire function for an absolute require when null is passed to RITM. We will need to pass something other than null to RITM, or we will need a patch to RITM to support absolute requires when null is passed.

I also noticed that RITM passes name and basedir a little differently, in this scenario, so we will need to update our logic in RequireInTheMiddleSingleton to handle it (in the current branch's logic, we don't use basedir).

[weyert]

Personally, I think it’s worth it to revisit lambda in the future and merge this PR as it will benefit everyone that’s not using AWS Lambda’s. I can imagine AWS/Amazon can have a closer look at it :)

Yes, I am bias as I can’t use AWS in the cases I am using Opentelemetry but do have slow starts (feels to take minutes) when enabling instrumentation ☺️

[dyladan]

I dismissed my review until I have time to review the updated logic. Don't want this to get merged accidentally before that based on my outdated ✅

[mhassan1]

@dyladan Is there anything outstanding that is blocking this PR?

[dyladan]

@dyladan Is there anything outstanding that is blocking this PR?

Nothing particular just trying to make sure it's sufficiently reviewed because it's quite fundamental to the instrumentation. I asked more people to review it at the SIG meeting today.

[dyladan]

Requesting changes here so this isn't accidentally merged before @rauno56 comment is addressed

[rauno56]

Fantastic job, @mhassan1. Thanks for the work and your patience!