fix(instrumentation): lazily initialize require-in-the-middle for empty instrumentations#6590
fix(instrumentation): lazily initialize require-in-the-middle for empty instrumentations#6590biw wants to merge 1 commit into
Conversation
|
|
It's not clear why ESM, bundling and It feels like this PR works around an issue rather than actually finding and fixing the root cause! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6590 +/- ##
=======================================
Coverage 95.76% 95.76%
=======================================
Files 375 375
Lines 12739 12739
Branches 3013 3013
=======================================
Hits 12200 12200
Misses 539 539 🚀 New features to boost your workflow:
|
|
Ah, I see that essentially your PR here is a reproduction of the issue. I'll take a look! |
|
@biw your example from the Electron PR referenced some really old dependencies. When I updated everything to below, it doesn't result in a runtime error. "dependencies": {
"electron-squirrel-startup": "^1.0.1"
},
"devDependencies": {
"@sentry/electron": "7.11.0",
"@sentry/vue": "10.47.0",
"@vitejs/plugin-vue": "^6.0.6",
"electron": "^41.2.0",
"electron-vite": "^5.0.0",
"vite": "^8.0.8",
"vue": "^3.5.32"
}If you can supply a failing reproduction of the issue I might be able to help further. |
Good catch, I copied those from another example in the sentry-electron, but I can recreate it not failing when upgrading the dependencies. I will work on a full example that does experience the issue. Thanks for your review and patience! |
|
Hi @biw - were you able to repro this on more recent versions? 🙂 |
|
I would say this should be closed. Rather than init |
Which problem is this PR solving?
InstrumentationBasecurrently createsRequireInTheMiddleSingletoneagerly during construction.That means simply instantiating a Node instrumentation can install the global
require-in-the-middlehook, even when that instrumentation does not declare any modules to patch ininit().This is a problem for bundled ESM applications, including Electron main-process apps, which use
createRequire(import.meta.url)for native.nodeaddons.In this case, an instrumentation which only uses
diagnostics_channeland does not need module patching can still causerequire-in-the-middleto be installed. In bundled ESM environments, that unnecessary hook can lead to runtime failures when the app later usescreateRequire(), for exampleReferenceError: require is not defined.This PR makes
RequireInTheMiddleSingletonlazy. It is now only initialized when an instrumentation actually has module definitions to register.That keeps module-patching instrumentations working as before, while avoiding an unnecessary global side effect for instrumentations that do not use
require-in-the-middle.Short description of the changes
RequireInTheMiddleSingletonlazy inInstrumentationBaserequire-in-the-middleinitialization entirely wheninit()returns no module definitionsinit()resultsType of change
How Has This Been Tested?
Targeted tests run in
experimental/packages/opentelemetry-instrumentation:npx mocha -r ts-node/register test/node/InstrumentationBase.test.tsnpx mocha -r ts-node/register test/node/RequireInTheMiddleSingleton.test.tsAdded coverage verifies that
RequireInTheMiddleSingleton.getInstance()is not called:init()returns no module definitionsenable()for that same kind of instrumentationI also smoke-tested this change against a local Electron repro using Sentry + bundled ESM +
createRequire(import.meta.url). With this patch applied, the repro no longer throwsrequire is not defined, and the diagnostics-channel-based fetch instrumentation still remains enabled.Checklist: