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

bug(node-trace-sdk): could not load plugin errors are polutting stdout #1124

Closed
vmarchaud opened this issue May 29, 2020 · 9 comments
Closed
Assignees
Labels
bug Something isn't working Discussion Issue or PR that needs/is extended discussion.

Comments

@vmarchaud
Copy link
Member

Since the express module is in the list of default plugins, the plugin loader try to load it when started:

PluginLoader#load: could not load plugin @opentelemetry/plugin-express of module express. Error: Cannot find module '@opentelemetry/plugin-express'
Require stack:
- /home/node/node_modules/@opentelemetry/node/build/src/instrumentation/PluginLoader.js
- /home/node/node_modules/@opentelemetry/node/build/src/NodeTracerProvider.js
- /home/node/node_modules/@opentelemetry/node/build/src/index.js
- /home/node/node_modules/@rlvt/telemetry/build/index.js
- /home/node/src/index.js

However in my case i don't want the express plugin, and i also don't want to those errors because it's expected.

I would don't think we should log if the module isn't found in this case, WDYT ?

@vmarchaud vmarchaud added bug Something isn't working Discussion Issue or PR that needs/is extended discussion. labels May 29, 2020
@dyladan
Copy link
Member

dyladan commented May 29, 2020

Can you just add { express: { enabled: false } } to the plugin config?

@vmarchaud
Copy link
Member Author

Yeah i can do it as a workaround, however i don't think we should require users to disable plugins they don't want (since they didn't install them in the first place).

@vmarchaud
Copy link
Member Author

I think we should really fix this, its really common for people to think the sdk is not working when seeing those logs. Could we just log those as debug ? cc @open-telemetry/javascript-approvers

@naseemkullah
Copy link
Member

FWIW I do find them useful to know which plugin I forgot to install. But it does not merit Error level as @vmarchaud mentioned. Warn, Info or Debug are all more appropriate than Error.

@naseemkullah
Copy link
Member

It's probably more Warn worthy for non default plugins that are explicitly enabled but not found, but implementing that check for explicitly enabled might be more logic than its worth. Let's just choose A lower level for 'em all.

@vmarchaud
Copy link
Member Author

Yeah my main issue is that when a default plugin is added, i need to add an entry in the tracing sdk to ignore this plugin specially. There is currently 13 default plugins, so by default if someone just want to use the http one, there will be 12 error logs at startup.

It's probably more Warn worthy for non default plugins that are explicitly enabled but not found

That would have been the best solution but since the config are merged there no way at the plugin loader level to know if the plugin was explicitly enabled or not :/

@obecny
Copy link
Member

obecny commented Sep 14, 2020

FYI I'm refactoring default plugin instrumentation (started by @dyladan ) once this is done this might go away as the approach for plugin will be different. If this is not urgent you might think of delaying that until then.

@vmarchaud
Copy link
Member Author

That's fine for me, waiting for your PR then

@vmarchaud
Copy link
Member Author

Closing since it will be fixed with the instrumentation class

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Discussion Issue or PR that needs/is extended discussion.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants