diff --git a/benchmark/sirun/startup/README.md b/benchmark/sirun/startup/README.md index da245095ffa..9667607a599 100644 --- a/benchmark/sirun/startup/README.md +++ b/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 diff --git a/benchmark/sirun/startup/everything-fixture/index.mjs b/benchmark/sirun/startup/everything-fixture/index.mjs index 25747518694..669f02a8efd 100644 --- a/benchmark/sirun/startup/everything-fixture/index.mjs +++ b/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))) diff --git a/benchmark/sirun/startup/meta.json b/benchmark/sirun/startup/meta.json index 00beb898e2c..3fcb8bc6ef0 100644 --- a/benchmark/sirun/startup/meta.json +++ b/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" } } } diff --git a/benchmark/sirun/startup/startup-test.js b/benchmark/sirun/startup/startup-test.js index fcc45bd788a..dad5b283ce4 100644 --- a/benchmark/sirun/startup/startup-test.js +++ b/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. diff --git a/initialize.mjs b/initialize.mjs index 91f84807efa..b9e95c331a1 100644 --- a/initialize.mjs +++ b/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') } } diff --git a/loader-hook.mjs b/loader-hook.mjs index c02a5f4e83e..557e2951c72 100644 --- a/loader-hook.mjs +++ b/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, }) diff --git a/packages/dd-trace/test/loader-hook.spec.js b/packages/dd-trace/test/loader-hook.spec.js index 05c8ee2a82c..024c3e1bc1e 100644 --- a/packages/dd-trace/test/loader-hook.spec.js +++ b/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') + }) })