-
Notifications
You must be signed in to change notification settings - Fork 842
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
fix: Hide require.resolve
from bundlers
#4660
Conversation
1c55788
to
8c12e73
Compare
FWIW, I'd love to add a related test case, as mentioned at #4173 (comment) However, that doesn't have to be part of this change. I've opened #4744 to discuss adding bundler-related test cases. |
@@ -10,6 +10,8 @@ All notable changes to experimental packages in this project will be documented | |||
|
|||
### :bug: (Bug Fix) | |||
|
|||
* fix: Hide `require.resolve` from bundlers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
* fix: Hide `require.resolve` from bundlers | |
* fix(instrumentation): Hide `require.resolve` from bundlers [#4660](https://github.com/open-telemetry/opentelemetry-js/pull/4660) |
mod: { require: { resolve: (name: string) => string } }, | ||
name: string | ||
): string { | ||
return mod.require.resolve(name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work. The module
object has module.require
, but not module.require.resolve
. Its module.require
is not the same as the require
in module scope.
That throws:
XXX boom TypeError: mod.require.resolve is not a function
at hiddenFromBundlersRequireResolve (/Users/trentm/tm/opentelemetry-js3/experimental/packages/opentelemetry-instrumentation/build/src/platform/node/instrumentation.js:35:24)
at /Users/trentm/tm/opentelemetry-js3/experimental/packages/opentelemetry-instrumentation/build/src/platform/node/instrumentation.js:123:40
at Array.forEach (<anonymous>)
at BunyanInstrumentation._warnOnPreloadedModules (/Users/trentm/tm/opentelemetry-js3/experimental/packages/opentelemetry-instrumentation/build/src/platform/node/instrumentation.js:119:23)
...
which is getting silently missed in the try/cache in _warnOnPreloadedModules
.
I guess this package doesn't have a test case for _warnOnPreloadedModules
.
Here is a small script that results in that diag.warn:
const api = require('@opentelemetry/api');
api.diag.setLogger(new api.DiagConsoleLogger(), api.DiagLogLevel.DEBUG);
const {
BunyanInstrumentation,
} = require('@opentelemetry/instrumentation-bunyan');
const { registerInstrumentations } = require('@opentelemetry/instrumentation');
const bunyan = require('bunyan'); // oops, required before instrumentation created
registerInstrumentations({
instrumentations: [new BunyanInstrumentation()],
});
const log = bunyan.createLogger({ name: 'hit' });
log.info('hi');
Running that:
% node hit-warn-on-preloaded.js
@opentelemetry/api: Registered a global for diag v1.8.0.
@opentelemetry/instrumentation-bunyan Module bunyan has been loaded before @opentelemetry/instrumentation-bunyan so it might not work, please initialize it before requiring bunyan
{"name":"hit","pid":77526,"level":30,"msg":"hi","time":"2024-05-31T22:07:54.248Z","v":0}
You are correct! I need to think of a different approach |
Which problem is this PR solving?
Bundlers pick up and attempt to parse
require.resolve
and give warnings or errors because the module name is dynamic.See #4173 (comment)
Fixes #4173
Short description of the changes
I proposed this fix here #4173 (comment)
Type of change