Incompatibility with npm modules which need a fresh copy of submodules

[kirrg001]

What happened?

Steps to Reproduce

See elastic/require-in-the-middle#61.
There is also a zip inside.

Expected Result

I expected no error.
Opentelemetry is not compatible with npm modules, which rely on a fresh copy of submodules.

See https://github.com/request/request-promise-native/blob/master/lib/rp.js#L7
See https://github.com/request/request-promise/blob/master/lib/rp.js#L10

Actual Result

Error

Error: Unable to expose method "then"
    at plumbing.exposePromiseMethod (/Users/kirrg001/Downloads/req-pro/node_modules/request-promise-core/lib/plumbing.js:140:19)
    at module.exports (/Users/kirrg001/Downloads/req-pro/node_modules/request-promise-core/configure/request2.js:57:83)
    at Object.<anonymous> (/Users/kirrg001/Downloads/req-pro/node_modules/request-promise-native/lib/rp.js:15:1)
    at Module._compile (node:internal/modules/cjs/loader:1254:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1308:10)
    at Module.load (node:internal/modules/cjs/loader:1117:32)
    at Module._load (node:internal/modules/cjs/loader:958:12)
    at Module.require (node:internal/modules/cjs/loader:1141:19)
    at Hook._require.Module.require (/Users/kirrg001/Downloads/req-pro/node_modules/@opentelemetry/instrumentation/node_modules/require-in-the-middle/index.js:101:39)

Additional Details

OpenTelemetry Setup Code

// minimal setup to reproduce

const { FsInstrumentation } = require('@opentelemetry/instrumentation-fs');
const instrumentation = new FsInstrumentation({})

const request = require('request')
console.log('request', request.Request.prototype.then)

const rp = require('request-promise');
console.log('rp', rp)

const request1 = require('request')
console.log('request', request1.Request.prototype.then)

const rpn = require('request-promise-native');
console.log('rpn', rpn)

package.json

{
  "name": "req-pro",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "ISC",
  "dependencies": {
    "@opentelemetry/instrumentation-fs": "^0.7.0",
    "request": "*",
    "request-promise": "*",
    "request-promise-native": "*",
    "require-in-the-middle": "*"
  }
}

Relevant log output

No response

[dyladan]

This seems to be caused by the fact that request-promise and request-promise-native are bypassing the require cache using stealthy-require. We are using require-in-the-middle which has its own cache, so the stealthy-require is removing the module from require.cache but require-in-the-middle is returning its own cached version.

If this is a bug, then the root cause is with require-in-the-middle, but I think it won't be very easy to fix this in a general way. The only way for us to fix this would be to replace require-in-the-middle with something else which doesn't cache, but that would have massive performance implications.

[Flarna]

I think it's not needed to use something which doesn't cache.
It seems require-in-the-middle is called before the node built in cache is accessed. This results in calling it on every require.
Maybe this could be changed in a way that the hook is called after node module cache. Then no more caching in this new thing would be needed.

If someone modifies/bypasses node cache like stealthy-require the hook would be called again and this second instance would be also instrumented.

Another option is to patch stealthy-require to remove the module in question not only in node cache but also in require-in-the-middle. But that seems quite a hacky aproach. But well, that fits maybe well into the world of monkey patching and module cache modifcation :o).

[kirrg001]

I have added a code change suggestion to the require-in-the-middle bug report. The maintainer hasn't reacted to the report at all yet.

---

To workaround this problem for now: Would you accept a pull request to pass in an option to only hook into certain instrumentations via require-in-the-middle?

See https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts#L64

requireInTheMiddleList = ['fs', 'socket.io'];
RequireInTheMiddle(
      // Intercept all `require` calls; we will filter the matching ones below
      // Exception: user passes in list of instrumentations, which should get patched only
      options.requireInTheMiddleList || null
      ...
)

That would be great. Thanks

[kirrg001]

@dyladan I would like to create a PR

[Flarna]

@kirrg001 There is no need to ask for permissions to create a PR :o)
Once it's there we have a CI run and a place to discuss about it further.

[kirrg001]

@Flarna I've never asked for permission. I have asked for a pre-approval of the suggested change.

[dyladan]

I'm labeling this as an enhancement since it is a workaround for a bug in one of our dependencies. I also linked the PR so that people finding this issue will be directed there.

[trentm]

I've created a PR to fix this: elastic/require-in-the-middle#63
I'd appreciate a review from anyone here if they are so inclined. Even perhaps just running OTel JS tests with this changed RITM patch?

(My change removes the hook.cache field as part of the change. AFAICT OTel code isn't using that undocumented field, so shouldn't be affected.)