Skip to content
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

Error loading plugin when it is already being patched by require-in-the-middle #1461

Closed
maxmil7 opened this issue Aug 24, 2020 · 5 comments · Fixed by #1551
Closed

Error loading plugin when it is already being patched by require-in-the-middle #1461

maxmil7 opened this issue Aug 24, 2020 · 5 comments · Fixed by #1551
Assignees
Labels
bug Something isn't working

Comments

@maxmil7
Copy link

maxmil7 commented Aug 24, 2020

What version of OpenTelemetry are you using?

"@opentelemetry/node": "^0.10.0"
"@opentelemetry/tracing": "^0.10.1"

What version of Node are you using?

v12.17.0

What did you do?

I was trying to integrate open telemetry modules for NodeJS with my app, and saw the following error when starting the app:

PluginLoader#load: trying to load https@12.17.0
PluginLoader#load: trying to load http@12.17.0
PluginLoader#load: could not load plugin @opentelemetry/plugin-http of module http. Error: Cannot read property 'supportedVersions' of undefined

The issue is reproducible if you run the following piece of code:

const { SimpleSpanProcessor, ConsoleSpanExporter } = require('@opentelemetry/tracing');
const { NodeTracerProvider } = require('@opentelemetry/node');

const provider = new NodeTracerProvider();
provider.register();
provider.addSpanProcessor(new SimpleSpanProcessor(new ConsoleSpanExporter()));


const https = require('https');
const http = require('http');

process.env.NODE_TLS_REJECT_UNAUTHORIZED = 0;

function get(callback) {
    const req = https.request({ hostname: 'example.com' }, callback).end();
}

http.createServer((req, res) => {
    get((remote) => {
        remote.pipe(res);
    })
  }).listen(8000);

What did you expect to see?

plugin-http and plugin-https libraries should have registered without any error

What did you see instead?

There was an error registering the plugin-http library

Additional context

In the sample code above, https core module is required before http module.
This results in the following events:

  • When https module is required, require-in-the-middle loads plugin-https to patch it.
  • plugin-https imports @opentelemetry/plugin-http module
  • plugin-http imports core http library
  • require-in-the-middle loads the http library and executes the patch hook
  • The patch function tries to load the plugin-http module
  • Since, this module is already being patched by require-in-the-middle, it returns the current exports object, which doesn't have all the exported functions
  • Hence, the plugin property of the exported object is empty which results in the error here

Add any other context about the problem here.
There are other ways to reproduce this as well for eg:
After registering the provider, just require('@opentelemetry/plugin-http') will result in the same issue. Or any other plugin for that matter which imports the same core library.

I tried to think of a solve but couldn't come up with a clean solution. If anyone can point me in the right direction, I am happy to submit a PR for the issue

@maxmil7 maxmil7 added the bug Something isn't working label Aug 24, 2020
@dyladan
Copy link
Member

dyladan commented Aug 26, 2020

It looks like the issue is that request which is required here is not a type, but a concrete implementation. It is used here. The plugin should not require http. If the import declaration is changed to be import type { ... } from 'http';, then a require statement will not be included in the emitted JS and the circular dependency issue is resolved. The plugin should get the require function from this._moduleExports.request, not by importing it.

@dyladan dyladan self-assigned this Aug 26, 2020
@maxmil7
Copy link
Author

maxmil7 commented Aug 27, 2020

Thanks @dyladan for looking into it. I tried out your suggestion locally and it seems to resolve the issue. Would you accept a PR for it or will you be looking into it?

@zomble
Copy link

zomble commented Sep 21, 2020

For me, when following https://github.com/open-telemetry/opentelemetry-js/blob/master/getting-started/ts-example/README.md#getting-started-with-opentelemetry-js-typescript but then moving to set the specific plugins I want to load, specifying http & https gives me the same error as described above. Which led me to this issue.

@dyladan
Copy link
Member

dyladan commented Sep 21, 2020

@maxmil7 sorry this took so long but I opened a PR

@maxmil7
Copy link
Author

maxmil7 commented Sep 22, 2020

No problem. Thanks @dyladan!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants