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]

@mhassan1 please refrain from modifying history or force pushing. it makes it very difficult to track changes

[mhassan1]

Unit tests for @opentelemetry/instrumentation-http are failing in the pipeline and on my machine. I am looking into it. It seems to be a timing issue between when the require-in-the-middle singleton is initialized and when require('http') is first called.

[mhassan1]

This PR seems to introduce a change in behavior that may not be acceptable without a fix (and this is the reason why the @opentelemetry/instrumentation-http tests are failing).

Assume this scenario:

require('http')
new HttpInstrumentation()
require('http')

In the main code, RITM gets set up for http after new HttpInstrumentation(), so it will get patched on the next call to require.

In this PR's code, the singleton RITM will get set up before the first require, and our onRequire function will not find any matches for http, since new HttpInstrumentation() hasn't been called, yet; RITM will cache the result of our onRequire, and the second require will not result in our onRequire being called, so http will never be patched.

Is this change in behavior acceptable? It seems like it would be too big of a change to tell users "you must instantiate instrumentation before you require a package, otherwise it will never get instrumented," since that seems to not be true, today.

[dyladan]

We already tell users instrumentations must be instantiated first. Your example with http only works because it is a system module. If you require a non-system module before initializing the instrumentation, then initialize, then require it again, you will have a cached module that is not instrumented. Might need to test to confirm that but i'm pretty sure

[dyladan]

we even warn if you instantiate an instrumentation for a module that is already in the require cache

[mhassan1]

Maybe a reasonable solution would be to instantiate RITM only when the first instance of InstrumentationBase is created.

With that change in place, I see another problem: @opentelemetry/instrumentation-http test suites run sequentially in the same process, and each one does new HttpInstrumentation(). If the first test suite ends with instrumentation.disable(), the module patch will be removed (as expected), then the second suite's instrumentation.enable() will have no effect because the onRequire function will never run again.

What do you think is the right way to resolve that? Is it expected behavior for the user to create multiple instances of new HttpInstrumentation(), some enabled and some disabled?

[dyladan]

Disabling instrumentations is mostly meant as a testing feature, but it needs to work. What is happening in the new version that breaks it? Shouldn't we be able to modify the disable method to remove it from the hook cache if needed?

[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!