perf(loader): use synchronous hooks during initialization

benchmark/sirun/startup/README.md

@@ -6,7 +6,7 @@ curated toward modules dd-trace instruments. Both fixture entries read the same
 
 - `index.js` loads them with CommonJS `require`, exercising require-in-the-middle.
 - `index.mjs` loads them with ESM `import`; the `with-tracer-everything-esm`
-  variant registers the iitm ESM loader via `--import ../../../register.js`, so
+  variant registers the iitm ESM loader via `--import ../../../initialize.mjs`, so
   this is the variant that measures the synchronous-vs-asynchronous loader cost.
 
 ## Updating the fixture

benchmark/sirun/startup/everything-fixture/index.mjs

@@ -3,7 +3,7 @@ import { createRequire } from 'node:module'
 // Bare-specifier dynamic imports resolve against this sub-project's own
 // node_modules and go through the ESM resolve/load hooks, unlike the CJS
 // `require` in index.js. This is what puts the iitm ESM loader on the measured
-// path when the startup bench registers it via `--import ../../../register.js`.
+// path when the startup bench registers it via `--import ../../../initialize.mjs`.
 const { dependencies } = createRequire(import.meta.url)('./package.json')
 
 await Promise.all(Object.keys(dependencies).map((name) => import(name)))

benchmark/sirun/startup/meta.json

@@ -22,7 +22,7 @@
         "USE_TRACER": "1",
         "EVERYTHING": "1",
         "ESM": "1",
-        "NODE_OPTIONS": "--import ../../../register.js"
+        "NODE_OPTIONS": "--import ../../../initialize.mjs"
       }
     }
   }

benchmark/sirun/startup/startup-test.js

@@ -9,15 +9,15 @@ if (Number(process.env.USE_TRACER)) {
 if (Number(process.env.EVERYTHING)) {
   if (Number(process.env.ESM)) {
     // The ESM variant registers the iitm ESM loader through
-    // NODE_OPTIONS=--import ../../../register.js, so importing the fixture routes
+    // NODE_OPTIONS=--import ../../../initialize.mjs, so importing the fixture routes
     // every dependency and its transitive graph through the loader's resolve/load
     // hooks. The CJS branch below goes through require-in-the-middle and never
     // touches the ESM loader, so this is the only startup variant that measures
     // the synchronous-vs-asynchronous loader cost the iitm hooks change.
     assert.match(
       process.env.NODE_OPTIONS ?? '',
-      /--import\b.+register\.js/,
-      'ESM startup variant must register the iitm loader via --import register.js'
+      /--import\b.+initialize\.mjs/,
+      'ESM startup variant must register the iitm loader via --import initialize.mjs'
     )
     // The floating import is the measured workload: it keeps the process alive
     // until the graph finishes loading, and a rejection surfaces as a non-zero exit.

initialize.mjs

@@ -79,8 +79,6 @@ if (isMainThread) {
   // Only register the loader hook when instrumentation initialized. On a bailout the
   // loader has nothing to instrument and can keep a short-lived process from exiting.
   if (Module.register && initialized) {
-    // The loader builds its own include/exclude matcher in `initialize`, so no
-    // options need to cross the registration boundary.
-    Module.register('./loader-hook.mjs', import.meta.url)
+    require('./register.js')
   }
 }

loader-hook.mjs

@@ -119,7 +119,17 @@ function load (url, context, nextLoad) {
 }
 
 function loadSync (url, context, nextLoad) {
-  if (isCommonJSLoad(context)) {
+  if (hasRequireCondition(context)) {
+    const { format } = context
+    // CommonJS is instrumented by require-in-the-middle, while builtins and
+    // JSON are not rewritten. Keep ESM, TypeScript, and unknown formats on the
+    // existing path so their source can still be transformed when necessary.
+    if (format === undefined || format === 'commonjs' || format === 'builtin' || format === 'json') {
+      return nextLoad(url, context)
+    }
+  }
+
+  if (context.format === 'commonjs') {
     return getSyncImportInTheMiddleHook().loadSync(url, context, nextLoad)
   }
 
@@ -128,12 +138,17 @@ function loadSync (url, context, nextLoad) {
   })
 }
 
-function isCommonJSLoad (context) {
-  if (context.format) return context.format === 'commonjs'
+function resolveSync (specifier, context, nextResolve) {
+  // import-in-the-middle leaves require() resolutions untouched after calling
+  // nextResolve, so skip its additional bookkeeping on that path.
+  if (hasRequireCondition(context)) {
+    return nextResolve(specifier, context)
+  }
+
+  return getSyncImportInTheMiddleHook().resolveSync(specifier, context, nextResolve)
+}
 
-  // Sync hooks report CommonJS require() dependency loads with a `require`
-  // condition but no format. If a format is present, trust it instead: ESM
-  // loaded through require() reports `format: 'module'` and still needs rewrite.
+function hasRequireCondition (context) {
   const conditions = context.conditions
   if (!conditions) return false
 
@@ -184,7 +199,7 @@ function registerSyncLoaderHooks (data = {}) {
   // that `import http from 'node:http'` is wrapped on both paths.
   syncHook.applyOptions(prepareImportInTheMiddleOptions(data))
   Module.registerHooks({
-    resolve: syncHook.resolveSync,
+    resolve: resolveSync,
     load: loadSync,
   })
 

packages/dd-trace/test/loader-hook.spec.js

@@ -8,8 +8,10 @@ const { join } = require('node:path')
 const { pathToFileURL } = require('node:url')
 
 const repositoryRoot = join(__dirname, '..', '..', '..')
+const initializeUrl = pathToFileURL(join(repositoryRoot, 'initialize.mjs')).href
 const loaderHookUrl = pathToFileURL(join(repositoryRoot, 'loader-hook.mjs')).href
 const configDefaultsPath = join(repositoryRoot, 'packages', 'dd-trace', 'src', 'config', 'defaults.js')
+const graphqlParserPath = join(repositoryRoot, 'node_modules', 'graphql', 'language', 'parser.mjs')
 
 const securityControls = 'SANITIZER:COMMAND_INJECTION:sanitizer/index.js:sanitize'
 const sanitizerUrl = 'file:///app/sanitizer/index.js'
@@ -79,4 +81,58 @@ describe('loader hook', () => {
 
     assert.strictEqual(initializeLoaderHook().includesSecurityControl, true)
   })
+
+  it('rewrites ESM loaded through require with synchronous hooks', async function () {
+    const { supportsSyncHooks } = await import('import-in-the-middle/supports-sync-hooks.mjs')
+    if (!supportsSyncHooks()) this.skip()
+
+    const result = spawnSync(process.execPath, ['--eval', `
+      const { tracingChannel } = require('./packages/datadog-instrumentations/src/helpers/instrument')
+
+      let publishes = 0
+      tracingChannel('orchestrion:graphql:apm:graphql:parser').start.subscribe(() => { publishes++ })
+
+      const { parse } = require(${JSON.stringify(graphqlParserPath)})
+      parse('{ field }')
+
+      process.stdout.write(String(publishes), () => process.exit())
+    `], {
+      cwd: repositoryRoot,
+      encoding: 'utf8',
+      env: {
+        PATH: process.env.PATH,
+        DD_INJECT_FORCE: process.env.DD_INJECT_FORCE,
+        DD_INSTRUMENTATION_TELEMETRY_ENABLED: 'false',
+        DD_REMOTE_CONFIG_ENABLED: 'false',
+        DD_TRACE_STARTUP_LOGS: 'false',
+        NODE_OPTIONS: `--import ${initializeUrl}`,
+      },
+    })
+
+    assert.strictEqual(result.status, 0, result.stderr)
+    assert.strictEqual(result.stdout, '1')
+  })
+
+  it('loads CommonJS through ESM import', () => {
+    const dependencyPath = join(temporaryDirectory, 'dependency.cjs')
+    writeFileSync(dependencyPath, 'module.exports = 42\n')
+
+    const result = spawnSync(process.execPath, ['--input-type=module', '--eval', `
+      const { default: value } = await import(${JSON.stringify(pathToFileURL(dependencyPath).href)})
+      process.stdout.write(String(value), () => process.exit())
+    `], {
+      encoding: 'utf8',
+      env: {
+        PATH: process.env.PATH,
+        DD_INJECT_FORCE: process.env.DD_INJECT_FORCE,
+        DD_INSTRUMENTATION_TELEMETRY_ENABLED: 'false',
+        DD_REMOTE_CONFIG_ENABLED: 'false',
+        DD_TRACE_STARTUP_LOGS: 'false',
+        NODE_OPTIONS: `--import ${initializeUrl}`,
+      },
+    })
+
+    assert.strictEqual(result.status, 0, result.stderr)
+    assert.strictEqual(result.stdout, '42')
+  })
 })