From bfe7e84a6560069dd985e27f8076c35537619837 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 11 Nov 2025 14:34:17 +0100 Subject: [PATCH 01/11] chore: improve instrumentation hooks This improves our instrumentations in multiple ways: - Add JSDoc to many methods that it is easier to work with. - Fix multiple type errors. - Remove code that was only used in testing such as `unhook()`. - Fix small issues in esbuild instrumentation where a module might not return a moduleExports object. - Improve code to run slightly faster (these are not hot code paths). - Automatically instruments unprefixed Node.js modules when defining a instrumentation for a module. This removes the need for much special handling (duplicating the instrumentation, using the symbol to detect already instrumented calls, etc.). - Fix using an array as file for some modules. This did not cause any issues, but it was not correct. - Removed unused isIitm arguments in instrumentations. --- packages/datadog-esbuild/index.js | 6 +- .../src/child_process.js | 22 +-- .../datadog-instrumentations/src/crypto.js | 3 +- packages/datadog-instrumentations/src/dns.js | 3 +- .../datadog-instrumentations/src/express.js | 8 +- packages/datadog-instrumentations/src/fs.js | 54 ++++--- .../src/helpers/bundler-register.js | 52 +++++-- .../src/helpers/hook.js | 13 +- .../src/helpers/hooks.js | 12 +- .../src/helpers/instrument.js | 40 ++++-- .../src/helpers/instrumentations.js | 12 ++ .../src/helpers/register.js | 134 ++++++++++-------- .../src/http/client.js | 5 +- .../src/http/server.js | 7 +- .../src/http2/client.js | 4 +- .../src/http2/server.js | 4 +- .../src/limitd-client.js | 2 +- packages/datadog-instrumentations/src/net.js | 10 +- packages/datadog-instrumentations/src/pino.js | 2 +- .../datadog-instrumentations/src/sequelize.js | 2 +- packages/datadog-instrumentations/src/url.js | 3 +- packages/datadog-instrumentations/src/vm.js | 3 +- .../dd-trace/src/appsec/iast/iast-plugin.js | 2 +- packages/dd-trace/src/lambda/runtime/ritm.js | 12 +- packages/dd-trace/src/ritm.js | 47 +++--- packages/dd-trace/test/ritm.spec.js | 39 +++-- 26 files changed, 267 insertions(+), 234 deletions(-) diff --git a/packages/datadog-esbuild/index.js b/packages/datadog-esbuild/index.js index 97a318ac3ba..64582c0fe17 100644 --- a/packages/datadog-esbuild/index.js +++ b/packages/datadog-esbuild/index.js @@ -26,12 +26,12 @@ for (const hook of Object.values(hooks)) { const modulesOfInterest = new Set() -for (const instrumentation of Object.values(instrumentations)) { +for (const [name, instrumentation] of Object.entries(instrumentations)) { for (const entry of instrumentation) { if (!entry.file) { - modulesOfInterest.add(entry.name) // e.g. "redis" + modulesOfInterest.add(name) // e.g. "redis" } else { - modulesOfInterest.add(`${entry.name}/${entry.file}`) // e.g. "redis/my/file.js" + modulesOfInterest.add(`${name}/${entry.file}`) // e.g. "redis/my/file.js" } } } diff --git a/packages/datadog-instrumentations/src/child_process.js b/packages/datadog-instrumentations/src/child_process.js index 71c199b7991..360fe8fa9c5 100644 --- a/packages/datadog-instrumentations/src/child_process.js +++ b/packages/datadog-instrumentations/src/child_process.js @@ -14,11 +14,6 @@ const childProcessChannel = dc.tracingChannel('datadog:child_process:execution') // ignored exec method because it calls to execFile directly const execAsyncMethods = ['execFile', 'spawn'] -const names = ['child_process', 'node:child_process'] - -// child_process and node:child_process returns the same object instance, we only want to add hooks once -let patched = false - function throwSyncError (error) { throw error } @@ -37,18 +32,13 @@ function returnSpawnSyncError (error, context) { return context.result } -names.forEach(name => { - addHook({ name }, childProcess => { - if (!patched) { - patched = true - shimmer.massWrap(childProcess, execAsyncMethods, wrapChildProcessAsyncMethod(childProcess.ChildProcess)) - shimmer.wrap(childProcess, 'execSync', wrapChildProcessSyncMethod(throwSyncError, true)) - shimmer.wrap(childProcess, 'execFileSync', wrapChildProcessSyncMethod(throwSyncError)) - shimmer.wrap(childProcess, 'spawnSync', wrapChildProcessSyncMethod(returnSpawnSyncError)) - } +addHook({ name: 'child_process' }, childProcess => { + shimmer.massWrap(childProcess, execAsyncMethods, wrapChildProcessAsyncMethod(childProcess.ChildProcess)) + shimmer.wrap(childProcess, 'execSync', wrapChildProcessSyncMethod(throwSyncError, true)) + shimmer.wrap(childProcess, 'execFileSync', wrapChildProcessSyncMethod(throwSyncError)) + shimmer.wrap(childProcess, 'spawnSync', wrapChildProcessSyncMethod(returnSpawnSyncError)) - return childProcess - }) + return childProcess }) function normalizeArgs (args, shell) { diff --git a/packages/datadog-instrumentations/src/crypto.js b/packages/datadog-instrumentations/src/crypto.js index 7c95614cee7..3113c16ef1d 100644 --- a/packages/datadog-instrumentations/src/crypto.js +++ b/packages/datadog-instrumentations/src/crypto.js @@ -11,9 +11,8 @@ const cryptoCipherCh = channel('datadog:crypto:cipher:start') const hashMethods = ['createHash', 'createHmac', 'createSign', 'createVerify', 'sign', 'verify'] const cipherMethods = ['createCipheriv', 'createDecipheriv'] -const names = ['crypto', 'node:crypto'] -addHook({ name: names }, crypto => { +addHook({ name: 'crypto' }, crypto => { shimmer.massWrap(crypto, hashMethods, wrapCryptoMethod(cryptoHashCh)) shimmer.massWrap(crypto, cipherMethods, wrapCryptoMethod(cryptoCipherCh)) return crypto diff --git a/packages/datadog-instrumentations/src/dns.js b/packages/datadog-instrumentations/src/dns.js index ac37784c58d..fe0e73804f6 100644 --- a/packages/datadog-instrumentations/src/dns.js +++ b/packages/datadog-instrumentations/src/dns.js @@ -18,9 +18,8 @@ const rrtypes = { } const rrtypeMap = new WeakMap() -const names = ['dns', 'node:dns'] -addHook({ name: names }, dns => { +addHook({ name: 'dns' }, dns => { shimmer.wrap(dns, 'lookup', fn => wrap('apm:dns:lookup', fn, 2)) shimmer.wrap(dns, 'lookupService', fn => wrap('apm:dns:lookup_service', fn, 2)) shimmer.wrap(dns, 'resolve', fn => wrap('apm:dns:resolve', fn, 2)) diff --git a/packages/datadog-instrumentations/src/express.js b/packages/datadog-instrumentations/src/express.js index d9f0743e085..eeceec10b95 100644 --- a/packages/datadog-instrumentations/src/express.js +++ b/packages/datadog-instrumentations/src/express.js @@ -146,7 +146,7 @@ function wrapAppUse (use) { } } -addHook({ name: 'express', versions: ['>=4'], file: ['lib/express.js'] }, express => { +addHook({ name: 'express', versions: ['>=4'], file: 'lib/express.js' }, express => { shimmer.wrap(express.application, 'handle', wrapHandle) shimmer.wrap(express.application, 'all', wrapAppAll) shimmer.wrap(express.application, 'route', wrapAppRoute) @@ -224,19 +224,19 @@ function wrapProcessParamsMethod (requestPositionInArguments) { } } -addHook({ name: 'express', versions: ['>=4.0.0 <4.3.0'], file: ['lib/express.js'] }, express => { +addHook({ name: 'express', versions: ['>=4.0.0 <4.3.0'], file: 'lib/express.js' }, express => { shimmer.wrap(express.Router, 'process_params', wrapProcessParamsMethod(1)) return express }) -addHook({ name: 'express', versions: ['>=4.3.0 <5.0.0'], file: ['lib/express.js'] }, express => { +addHook({ name: 'express', versions: ['>=4.3.0 <5.0.0'], file: 'lib/express.js' }, express => { shimmer.wrap(express.Router, 'process_params', wrapProcessParamsMethod(2)) return express }) const queryReadCh = channel('datadog:express:query:finish') -addHook({ name: 'express', file: ['lib/request.js'], versions: ['>=5.0.0'] }, request => { +addHook({ name: 'express', file: 'lib/request.js', versions: ['>=5.0.0'] }, request => { shimmer.wrap(request, 'query', function (originalGet) { return function wrappedGet () { const query = originalGet.call(this) diff --git a/packages/datadog-instrumentations/src/fs.js b/packages/datadog-instrumentations/src/fs.js index e284fee4d62..bc1f8d6cfb8 100644 --- a/packages/datadog-instrumentations/src/fs.js +++ b/packages/datadog-instrumentations/src/fs.js @@ -84,37 +84,35 @@ const paramsByFileHandleMethods = { writeFile: ['data', 'options'], writev: ['buffers', 'position'] } -const names = ['fs', 'node:fs'] -names.forEach(name => { - addHook({ name }, fs => { - const asyncMethods = Object.keys(paramsByMethod) - const syncMethods = asyncMethods.map(name => `${name}Sync`) - - massWrap(fs, asyncMethods, createWrapFunction()) - massWrap(fs, syncMethods, createWrapFunction()) - massWrap(fs.promises, asyncMethods, createWrapFunction('promises.')) - - wrap(fs.realpath, 'native', createWrapFunction('', 'realpath.native')) - wrap(fs.realpathSync, 'native', createWrapFunction('', 'realpath.native')) - wrap(fs.promises.realpath, 'native', createWrapFunction('', 'realpath.native')) - - wrap(fs, 'createReadStream', wrapCreateStream) - wrap(fs, 'createWriteStream', wrapCreateStream) - if (fs.Dir) { - wrap(fs.Dir.prototype, 'close', createWrapFunction('dir.')) - wrap(fs.Dir.prototype, 'closeSync', createWrapFunction('dir.')) - wrap(fs.Dir.prototype, 'read', createWrapFunction('dir.')) - wrap(fs.Dir.prototype, 'readSync', createWrapFunction('dir.')) - wrap(fs.Dir.prototype, Symbol.asyncIterator, createWrapDirAsyncIterator()) - } +addHook({ name: 'fs' }, fs => { + const asyncMethods = Object.keys(paramsByMethod) + const syncMethods = asyncMethods.map(name => `${name}Sync`) + + massWrap(fs, asyncMethods, createWrapFunction()) + massWrap(fs, syncMethods, createWrapFunction()) + massWrap(fs.promises, asyncMethods, createWrapFunction('promises.')) + + wrap(fs.realpath, 'native', createWrapFunction('', 'realpath.native')) + wrap(fs.realpathSync, 'native', createWrapFunction('', 'realpath.native')) + wrap(fs.promises.realpath, 'native', createWrapFunction('', 'realpath.native')) + + wrap(fs, 'createReadStream', wrapCreateStream) + wrap(fs, 'createWriteStream', wrapCreateStream) + if (fs.Dir) { + wrap(fs.Dir.prototype, 'close', createWrapFunction('dir.')) + wrap(fs.Dir.prototype, 'closeSync', createWrapFunction('dir.')) + wrap(fs.Dir.prototype, 'read', createWrapFunction('dir.')) + wrap(fs.Dir.prototype, 'readSync', createWrapFunction('dir.')) + wrap(fs.Dir.prototype, Symbol.asyncIterator, createWrapDirAsyncIterator()) + } - wrap(fs, 'unwatchFile', createWatchWrapFunction()) - wrap(fs, 'watch', createWatchWrapFunction()) - wrap(fs, 'watchFile', createWatchWrapFunction()) + wrap(fs, 'unwatchFile', createWatchWrapFunction()) + wrap(fs, 'watch', createWatchWrapFunction()) + wrap(fs, 'watchFile', createWatchWrapFunction()) - return fs - }) + return fs }) + function isFirstMethodReturningFileHandle (original) { return !kHandle && original.name === 'open' } diff --git a/packages/datadog-instrumentations/src/helpers/bundler-register.js b/packages/datadog-instrumentations/src/helpers/bundler-register.js index 422d035127a..854444e63e4 100644 --- a/packages/datadog-instrumentations/src/helpers/bundler-register.js +++ b/packages/datadog-instrumentations/src/helpers/bundler-register.js @@ -1,5 +1,6 @@ 'use strict' +/** @type {typeof import('node:diagnostics_channel')} */ const dc = require('dc-polyfill') const { @@ -22,45 +23,70 @@ if (!dc.unsubscribe) { dc.unsubscribe = (channel, cb) => { if (dc.channel(channel).hasSubscribers) { dc.channel(channel).unsubscribe(cb) + return true } + return false } } -function doHook (payload) { - const hook = hooks[payload.package] +/** + * @param {string} name + */ +function doHook (name) { + const hook = hooks[name] ?? hooks[`node:${name}`] if (!hook) { - log.error('esbuild-wrapped %s missing in list of hooks', payload.package) + log.error('esbuild-wrapped %s missing in list of hooks', name) return } const hookFn = hook.fn ?? hook if (typeof hookFn !== 'function') { - log.error('esbuild-wrapped hook %s is not a function', payload.package) + log.error('esbuild-wrapped hook %s is not a function', name) return } try { hookFn() } catch { - log.error('esbuild-wrapped %s hook failed', payload.package) + log.error('esbuild-wrapped %s hook failed', name) } } -dc.subscribe(CHANNEL, (payload) => { - doHook(payload) +/** @type {Set} */ +const instrumentedNodeModules = new Set() - if (!instrumentations[payload.package]) { - log.error('esbuild-wrapped %s missing in list of instrumentations', payload.package) +/** @typedef {{ package: string, module: unknown, version: string, path: string }} Payload */ +dc.subscribe(CHANNEL, (message) => { + const payload = /** @type {Payload} */ (message) + const name = payload.package + + const isNodeModule = name.startsWith('node:') || !hooks[name] + + if (isNodeModule) { + // Used for node: prefixed modules to prevent double instrumentation. + if (instrumentedNodeModules.has(name)) { + return + } + instrumentedNodeModules.add(name) + } + + doHook(name) + + const instrumentation = instrumentations[name] ?? instrumentations[`node:${name}`] + + if (!instrumentation) { + log.error('esbuild-wrapped %s missing in list of instrumentations', name) return } - for (const { name, file, versions, hook } of instrumentations[payload.package]) { - if (payload.path !== filename(name, file)) continue - if (!matchVersion(payload.version, versions)) continue + for (const { file, versions, hook } of instrumentation) { + if (payload.path !== filename(name, file) || !matchVersion(payload.version, versions)) { + continue + } try { loadChannel.publish({ name, version: payload.version, file }) - payload.module = hook(payload.module, payload.version) + payload.module = hook(payload.module, payload.version) ?? payload.module } catch (e) { log.error('Error executing bundler hook', e) } diff --git a/packages/datadog-instrumentations/src/helpers/hook.js b/packages/datadog-instrumentations/src/helpers/hook.js index dc3de38b301..f73125faf22 100644 --- a/packages/datadog-instrumentations/src/helpers/hook.js +++ b/packages/datadog-instrumentations/src/helpers/hook.js @@ -7,11 +7,18 @@ const ritm = require('../../../dd-trace/src/ritm') * This is called for every package/internal-module that dd-trace supports instrumentation for * In practice, `modules` is always an array with a single entry. * + * @overload + * @param {string[]} modules list of modules to hook into + * @param {object} hookOptions hook options + * @param {Function} onrequire callback to be executed upon encountering module + * + * @overload * @param {string[]} modules list of modules to hook into * @param {object} hookOptions hook options * @param {Function} onrequire callback to be executed upon encountering module */ function Hook (modules, hookOptions, onrequire) { + // TODO: Rewrite this to use class syntax. The same should be done for ritm. if (!(this instanceof Hook)) return new Hook(modules, hookOptions, onrequire) if (typeof hookOptions === 'function') { @@ -60,10 +67,4 @@ function Hook (modules, hookOptions, onrequire) { }) } -Hook.prototype.unhook = function () { - this._ritmHook.unhook() - this._iitmHook.unhook() - this._patched = Object.create(null) -} - module.exports = Hook diff --git a/packages/datadog-instrumentations/src/helpers/hooks.js b/packages/datadog-instrumentations/src/helpers/hooks.js index c46827b3537..d27b670b07b 100644 --- a/packages/datadog-instrumentations/src/helpers/hooks.js +++ b/packages/datadog-instrumentations/src/helpers/hooks.js @@ -42,31 +42,24 @@ module.exports = { 'body-parser': () => require('../body-parser'), bunyan: () => require('../bunyan'), 'cassandra-driver': () => require('../cassandra-driver'), - child_process: () => require('../child_process'), connect: () => require('../connect'), cookie: () => require('../cookie'), 'cookie-parser': () => require('../cookie-parser'), couchbase: () => require('../couchbase'), - crypto: () => require('../crypto'), cypress: () => require('../cypress'), 'dd-trace-api': () => require('../dd-trace-api'), - dns: () => require('../dns'), elasticsearch: () => require('../elasticsearch'), express: () => require('../express'), 'express-mongo-sanitize': () => require('../express-mongo-sanitize'), 'express-session': () => require('../express-session'), fastify: () => require('../fastify'), 'find-my-way': () => require('../find-my-way'), - fs: { serverless: false, fn: () => require('../fs') }, 'generic-pool': () => require('../generic-pool'), graphql: () => require('../graphql'), grpc: () => require('../grpc'), handlebars: () => require('../handlebars'), hapi: () => require('../hapi'), hono: { esmFirst: true, fn: () => require('../hono') }, - http: () => require('../http'), - http2: () => require('../http2'), - https: () => require('../http'), ioredis: () => require('../ioredis'), iovalkey: () => require('../iovalkey'), 'jest-circus': () => require('../jest'), @@ -97,12 +90,13 @@ module.exports = { multer: () => require('../multer'), mysql: () => require('../mysql'), mysql2: () => require('../mysql2'), - net: () => require('../net'), next: () => require('../next'), 'node-serialize': () => require('../node-serialize'), + // Only list prefixed node modules. They will automatically be instrumented as prefixed and unprefixed. 'node:child_process': () => require('../child_process'), 'node:crypto': () => require('../crypto'), 'node:dns': () => require('../dns'), + 'node:fs': { serverless: false, fn: () => require('../fs') }, 'node:http': () => require('../http'), 'node:http2': () => require('../http2'), 'node:https': () => require('../http'), @@ -135,9 +129,7 @@ module.exports = { tedious: () => require('../tedious'), tinypool: { esmFirst: true, fn: () => require('../vitest') }, undici: () => require('../undici'), - url: () => require('../url'), vitest: { esmFirst: true, fn: () => require('../vitest') }, - vm: () => require('../vm'), when: () => require('../when'), winston: () => require('../winston'), workerpool: () => require('../mocha'), diff --git a/packages/datadog-instrumentations/src/helpers/instrument.js b/packages/datadog-instrumentations/src/helpers/instrument.js index a3c0cd7a327..9237ef91793 100644 --- a/packages/datadog-instrumentations/src/helpers/instrument.js +++ b/packages/datadog-instrumentations/src/helpers/instrument.js @@ -1,10 +1,22 @@ 'use strict' -const dc = require('dc-polyfill') +const dc = /** @type {typeof import('node:diagnostics_channel')} */ (require('dc-polyfill')) const instrumentations = require('./instrumentations') const { AsyncResource } = require('async_hooks') +/** + * @typedef {import('node:diagnostics_channel').Channel} Channel + * @typedef {import('node:diagnostics_channel').TracingChannel} TracingChannel + */ + +/** + * @type {Record} + */ const channelMap = {} +/** + * @param {string} name + * @returns {Channel} + */ exports.channel = function (name) { const maybe = channelMap[name] if (maybe) return maybe @@ -13,7 +25,14 @@ exports.channel = function (name) { return ch } +/** + * @type {Record} + */ const tracingChannelMap = {} +/** + * @param {string} name + * @returns {TracingChannel} + */ exports.tracingChannel = function (name) { const maybe = tracingChannelMap[name] if (maybe) return maybe @@ -24,24 +43,19 @@ exports.tracingChannel = function (name) { /** * @param {object} args - * @param {string|string[]} args.name module name - * @param {string[]} args.versions array of semver range strings + * @param {string} args.name module name + * @param {string[]} [args.versions] array of semver range strings * @param {string} [args.file='index.js'] path to file within package to instrument * @param {string} [args.filePattern] pattern to match files within package to instrument - * @param {boolean} [args.patchDefault] whether to patch the default export - * @param {(moduleExports: unknown, version: string) => unknown} hook + * @param {boolean} [args.patchDefault=true] whether to patch the default export + * @param {import('./instrumentations').Hook} hook */ exports.addHook = function addHook ({ name, versions, file, filePattern, patchDefault }, hook) { - if (typeof name === 'string') { - name = [name] + if (!instrumentations[name]) { + instrumentations[name] = [] } - for (const val of name) { - if (!instrumentations[val]) { - instrumentations[val] = [] - } - instrumentations[val].push({ name: val, versions, file, filePattern, hook, patchDefault }) - } + instrumentations[name].push({ versions, file, filePattern, hook, patchDefault }) } exports.AsyncResource = AsyncResource diff --git a/packages/datadog-instrumentations/src/helpers/instrumentations.js b/packages/datadog-instrumentations/src/helpers/instrumentations.js index 6ce24ab38dc..94c862d48e2 100644 --- a/packages/datadog-instrumentations/src/helpers/instrumentations.js +++ b/packages/datadog-instrumentations/src/helpers/instrumentations.js @@ -4,4 +4,16 @@ const sym = Symbol.for('_ddtrace_instrumentations') global[sym] = global[sym] || {} +/** @template T */ +/** + * @typedef {(moduleExports: T, version: string) => T} Hook + * + * @type {Record} + * @typedef {Object} Instrumentation + * @property {string[]} [versions] + * @property {string} [file] + * @property {string} [filePattern] + * @property {Hook} hook + * @property {boolean} [patchDefault] + */ module.exports = global[sym] diff --git a/packages/datadog-instrumentations/src/helpers/register.js b/packages/datadog-instrumentations/src/helpers/register.js index 6e5f0f3985a..c83950d6fb7 100644 --- a/packages/datadog-instrumentations/src/helpers/register.js +++ b/packages/datadog-instrumentations/src/helpers/register.js @@ -38,23 +38,39 @@ if (!disabledInstrumentations.has('process')) { require('../process') } -const HOOK_SYMBOL = Symbol('hookExportsSet') - if (DD_TRACE_DEBUG && DD_TRACE_DEBUG.toLowerCase() !== 'false') { checkRequireCache.checkForRequiredModules() setImmediate(checkRequireCache.checkForPotentialConflicts) } +/** @type {Set} */ const seenCombo = new Set() +/** @type {Record} */ const allInstrumentations = {} +/** @type {Set} */ +const instrumentedNodeModules = new Set() + +for (const name of names) { + if (name.startsWith('node:')) { + // Add all unprefixed node modules to the instrumentations list. + names.push(name.slice(5)) + // Always disable prefixed and unprefixed node modules if one is disabled. + // TODO: Activate & write a regression test for this. + // if (disabledInstrumentations.has(name) !== disabledInstrumentations.has(`node:${name}`)) { + // disabledInstrumentations.add(name) + // disabledInstrumentations.add(`node:${name}`) + // } + } +} + +for (const name of names) { + if (disabledInstrumentations.has(name)) continue -// TODO: make this more efficient -for (const packageName of names) { - if (disabledInstrumentations.has(packageName)) continue + const isNodeModule = name.startsWith('node:') || !hooks[name] const hookOptions = {} - let hook = hooks[packageName] + let hook = hooks[name] ?? hooks[`node:${name}`] if (hook !== null && typeof hook === 'object') { if (hook.serverless === false && isInServerlessEnvironment()) continue @@ -63,42 +79,41 @@ for (const packageName of names) { hook = hook.fn } + const nameWithoutPrefix = name.startsWith('node:') ? name.slice(5) : name + // get the instrumentation file name to save all hooked versions - const instrumentationFileName = parseHookInstrumentationFileName(packageName) + const instrumentationFileName = parseHookInstrumentationFileName(hook) - Hook([packageName], hookOptions, (moduleExports, moduleName, moduleBaseDir, moduleVersion, isIitm) => { + Hook([name], hookOptions, (moduleExports, moduleName, moduleBaseDir, moduleVersion, isIitm) => { moduleName = moduleName.replace(pathSepExpr, '/') // This executes the integration file thus adding its entries to `instrumentations` hook() - if (!instrumentations[packageName]) { + if (!instrumentations[nameWithoutPrefix] || instrumentedNodeModules.has(nameWithoutPrefix)) { return moduleExports } + // Used for node: prefixed modules to prevent double instrumentation. + if (isNodeModule) { + instrumentedNodeModules.add(nameWithoutPrefix) + } + const namesAndSuccesses = {} - for (const { name, file, versions, hook, filePattern, patchDefault } of instrumentations[packageName]) { - if (patchDefault === false && !moduleExports.default && isIitm) { - return moduleExports - } else if (patchDefault === true && moduleExports.default && isIitm) { - moduleExports = moduleExports.default + for (const { file, versions, hook, filePattern, patchDefault } of instrumentations[nameWithoutPrefix]) { + if (isIitm && patchDefault === !!moduleExports.default) { + if (patchDefault) { + moduleExports = moduleExports.default + } else { + return moduleExports + } } - let fullFilePattern = filePattern const fullFilename = filename(name, file) - if (fullFilePattern) { - fullFilePattern = filename(name, fullFilePattern) - } - // Create a WeakSet associated with the hook function so that patches on the same moduleExport only happens once - // for example by instrumenting both dns and node:dns double the spans would be created - // since they both patch the same moduleExport, this WeakSet is used to mitigate that - // TODO(BridgeAR): Instead of using a WeakSet here, why not just use aliases for the hook in register? - // That way it would also not be duplicated. The actual name being used has to be identified else wise. - // Maybe it is also not important to know what name was actually used? - hook[HOOK_SYMBOL] ??= new WeakSet() let matchesFile = moduleName === fullFilename + const fullFilePattern = filePattern && filename(name, filePattern) if (fullFilePattern) { // Some libraries include a hash in their filenames when installed, // so our instrumentation has to include a '.*' to match them for more than a single version. @@ -109,12 +124,11 @@ for (const packageName of names) { let version = moduleVersion try { version = version || getVersion(moduleBaseDir) - allInstrumentations[instrumentationFileName] = allInstrumentations[instrumentationFileName] || false } catch (e) { log.error('Error getting version for "%s": %s', name, e.message, e) continue } - if (namesAndSuccesses[`${name}@${version}`] === undefined && !file) { + if (namesAndSuccesses[`${nameWithoutPrefix}@${version}`] === undefined && !file) { // TODO If `file` is present, we might elsewhere instrument the result of the module // for a version range that actually matches, so we can't assume that we're _not_ // going to instrument that. However, the way the data model around instrumentation @@ -122,36 +136,21 @@ for (const packageName of names) { // ignore this if there is a `file` in the hook. The thing to do here is rework // everything so that we can be sure that there are _no_ instrumentations that it // could match. - namesAndSuccesses[`${name}@${version}`] = false + namesAndSuccesses[`${nameWithoutPrefix}@${version}`] = false } if (matchVersion(version, versions)) { - allInstrumentations[instrumentationFileName] = true - - // Check if the hook already has a set moduleExport - if (hook[HOOK_SYMBOL].has(moduleExports)) { - namesAndSuccesses[`${name}@${version}`] = true - return moduleExports - } + allInstrumentations[instrumentationFileName] ||= false try { - loadChannel.publish({ name, version, file }) - // Send the name and version of the module back to the callback because now addHook - // takes in an array of names so by passing the name the callback will know which module name is being used - // TODO(BridgeAR): This is only true in case the name is identical - // in all loads. If they deviate, the deviating name would not be - // picked up due to the unification. Check what modules actually use the name. - // TODO(BridgeAR): Only replace moduleExports if the hook returns a new value. - // This allows to reduce the instrumentation code (no return needed). - - moduleExports = hook(moduleExports, version, name, isIitm) ?? moduleExports - // Set the moduleExports in the hooks WeakSet - hook[HOOK_SYMBOL].add(moduleExports) + loadChannel.publish({ name }) + + moduleExports = hook(moduleExports, version) ?? moduleExports } catch (e) { log.info('Error during ddtrace instrumentation of application, aborting.', e) telemetry('error', [ `error_type:${e.constructor.name}`, - `integration:${name}`, + `integration:${nameWithoutPrefix}`, `integration_version:${version}` ], { result: 'error', @@ -159,15 +158,21 @@ for (const packageName of names) { result_reason: `Error during instrumentation of ${name}@${version}: ${e.message}` }) } - namesAndSuccesses[`${name}@${version}`] = true + namesAndSuccesses[`${nameWithoutPrefix}@${version}`] = true } } } for (const nameVersion of Object.keys(namesAndSuccesses)) { - const [name, version] = nameVersion.split('@') const success = namesAndSuccesses[nameVersion] // we check allVersions to see if any version of the integration was successfully instrumented + // TODO: The allInstrumentations check is meant to fix https://github.com/DataDog/dd-trace-js/issues/5092 + // This is a workaround to actually detect if any hook inside of the instrumentation file was successful or not. + // This could be simplified by checking it in a different way. Another alternative is to mention which hooks were + // successful or not. That would also be better for debugging. If no filename is detected, the allInstrumentations + // check will always be false - which is a mistake. + // The current seenCombo also seems to be redundant to namesAndSuccesses. Refactor this if possible. if (!success && !seenCombo.has(nameVersion) && !allInstrumentations[instrumentationFileName]) { + const [name, version] = nameVersion.split('@') telemetry('abort.integration', [ `integration:${name}`, `integration_version:${version}` @@ -185,18 +190,31 @@ for (const packageName of names) { }) } +/** + * @param {string|undefined} version + * @param {string[]|undefined} ranges + */ function matchVersion (version, ranges) { return !version || !ranges || ranges.some(range => satisfies(version, range)) } +/** + * @param {string} moduleBaseDir + * @returns {string|undefined} + */ function getVersion (moduleBaseDir) { if (moduleBaseDir) { - return requirePackageJson(moduleBaseDir, module).version + return requirePackageJson(moduleBaseDir, /** @type {import('module').Module} */ (module)).version } } +/** + * @param {string} name + * @param {string} [file] + * @returns {string} + */ function filename (name, file) { - return [name, file].filter(Boolean).join('/') + return file ? `${name}/${file}` : name } // This function captures the instrumentation file name for a given package by parsing the hook require @@ -208,11 +226,11 @@ function filename (name, file) { // redis -> "() => require('../redis')" -> redis // @redis/client -> "() => require('../redis')" -> redis // -function parseHookInstrumentationFileName (packageName) { - let hook = hooks[packageName] - if (hook.fn) { - hook = hook.fn - } +/** + * @param {Function} hook + * @returns {string|undefined} + */ +function parseHookInstrumentationFileName (hook) { const hookString = hook.toString() const regex = /require\('([^']*)'\)/ @@ -227,8 +245,6 @@ function parseHookInstrumentationFileName (packageName) { } return moduleName } - - return null } module.exports = { diff --git a/packages/datadog-instrumentations/src/http/client.js b/packages/datadog-instrumentations/src/http/client.js index 508675692cc..9f9e356b80c 100644 --- a/packages/datadog-instrumentations/src/http/client.js +++ b/packages/datadog-instrumentations/src/http/client.js @@ -15,9 +15,8 @@ const endChannel = channel('apm:http:client:request:end') const asyncStartChannel = channel('apm:http:client:request:asyncStart') const errorChannel = channel('apm:http:client:request:error') -const names = ['http', 'https', 'node:http', 'node:https'] - -addHook({ name: names }, hookFn) +addHook({ name: 'http' }, hookFn) +addHook({ name: 'https' }, hookFn) function hookFn (http) { patch(http, 'request') diff --git a/packages/datadog-instrumentations/src/http/server.js b/packages/datadog-instrumentations/src/http/server.js index 0624c886787..bf1ce86eec8 100644 --- a/packages/datadog-instrumentations/src/http/server.js +++ b/packages/datadog-instrumentations/src/http/server.js @@ -16,10 +16,7 @@ const startSetHeaderCh = channel('datadog:http:server:response:set-header:start' const requestFinishedSet = new WeakSet() -const httpNames = ['http', 'node:http'] -const httpsNames = ['https', 'node:https'] - -addHook({ name: httpNames }, http => { +addHook({ name: 'http' }, http => { shimmer.wrap(http.ServerResponse.prototype, 'emit', wrapResponseEmit) shimmer.wrap(http.Server.prototype, 'emit', wrapEmit) shimmer.wrap(http.ServerResponse.prototype, 'writeHead', wrapWriteHead) @@ -34,7 +31,7 @@ addHook({ name: httpNames }, http => { return http }) -addHook({ name: httpsNames }, http => { +addHook({ name: 'https' }, http => { // http.ServerResponse not present on https shimmer.wrap(http.Server.prototype, 'emit', wrapEmit) return http diff --git a/packages/datadog-instrumentations/src/http2/client.js b/packages/datadog-instrumentations/src/http2/client.js index b335df7c4cd..d38837fb2d9 100644 --- a/packages/datadog-instrumentations/src/http2/client.js +++ b/packages/datadog-instrumentations/src/http2/client.js @@ -10,8 +10,6 @@ const asyncStartChannel = channel('apm:http2:client:request:asyncStart') const asyncEndChannel = channel('apm:http2:client:request:asyncEnd') const errorChannel = channel('apm:http2:client:request:error') -const names = ['http2', 'node:http2'] - function createWrapEmit (ctx) { return function wrapEmit (emit) { return function (event, arg1) { @@ -68,7 +66,7 @@ function wrapConnect (connect) { } } -addHook({ name: names }, http2 => { +addHook({ name: 'http2' }, http2 => { shimmer.wrap(http2, 'connect', wrapConnect) if (http2.default) http2.default.connect = http2.connect diff --git a/packages/datadog-instrumentations/src/http2/server.js b/packages/datadog-instrumentations/src/http2/server.js index 5977d738554..5285f582d6c 100644 --- a/packages/datadog-instrumentations/src/http2/server.js +++ b/packages/datadog-instrumentations/src/http2/server.js @@ -13,9 +13,7 @@ const startServerCh = channel('apm:http2:server:request:start') const errorServerCh = channel('apm:http2:server:request:error') const emitCh = channel('apm:http2:server:response:emit') -const names = ['http2', 'node:http2'] - -addHook({ name: names }, http2 => { +addHook({ name: 'http2' }, http2 => { shimmer.wrap(http2, 'createSecureServer', wrapCreateServer) shimmer.wrap(http2, 'createServer', wrapCreateServer) }) diff --git a/packages/datadog-instrumentations/src/limitd-client.js b/packages/datadog-instrumentations/src/limitd-client.js index e0684c406b6..6a7e763464f 100644 --- a/packages/datadog-instrumentations/src/limitd-client.js +++ b/packages/datadog-instrumentations/src/limitd-client.js @@ -14,7 +14,7 @@ function wrapRequest (original) { addHook({ name: 'limitd-client', versions: ['>=2.8'], - file: ['client.js'] + file: 'client.js' }, LimitdClient => { shimmer.wrap(LimitdClient.prototype, '_directRequest', wrapRequest) shimmer.wrap(LimitdClient.prototype, '_retriedRequest', wrapRequest) diff --git a/packages/datadog-instrumentations/src/net.js b/packages/datadog-instrumentations/src/net.js index b0b6163efc7..07969582c76 100644 --- a/packages/datadog-instrumentations/src/net.js +++ b/packages/datadog-instrumentations/src/net.js @@ -16,16 +16,10 @@ const errorTCPCh = channel('apm:net:tcp:error') const readyCh = channel('apm:net:tcp:ready') const connectionCh = channel('apm:net:tcp:connection') -const names = ['net', 'node:net'] - -addHook({ name: names }, (net, version, name) => { +addHook({ name: 'net' }, (net) => { // explicitly require dns so that net gets an instrumented instance // so that we don't miss the dns calls - if (name === 'net') { - require('dns') - } else { - require('node:dns') - } + require('node:dns') shimmer.wrap(net.Socket.prototype, 'connect', connect => function () { if (!startICPCh.hasSubscribers || !startTCPCh.hasSubscribers) { diff --git a/packages/datadog-instrumentations/src/pino.js b/packages/datadog-instrumentations/src/pino.js index 958e8c858c8..a0b049c500e 100644 --- a/packages/datadog-instrumentations/src/pino.js +++ b/packages/datadog-instrumentations/src/pino.js @@ -97,7 +97,7 @@ addHook({ name: 'pino', versions: ['>=5.14.0 <6.8.0'] }, (pino) => { return wrapped }) -addHook({ name: 'pino', versions: ['>=6.8.0'], patchDefault: false }, (pino, _1, _2, isIitm) => { +addHook({ name: 'pino', versions: ['>=6.8.0'], patchDefault: false }, (pino) => { const mixinSym = pino.symbols.mixinSym const wrapped = shimmer.wrapFunction(pino, pino => wrapPino(mixinSym, wrapMixin, pino)) diff --git a/packages/datadog-instrumentations/src/sequelize.js b/packages/datadog-instrumentations/src/sequelize.js index 48939654a4e..c99d019a5a4 100644 --- a/packages/datadog-instrumentations/src/sequelize.js +++ b/packages/datadog-instrumentations/src/sequelize.js @@ -7,7 +7,7 @@ const { const shimmer = require('../../datadog-shimmer') -addHook({ name: 'sequelize', versions: ['>=4'], file: ['lib/sequelize.js'] }, Sequelize => { +addHook({ name: 'sequelize', versions: ['>=4'], file: 'lib/sequelize.js' }, Sequelize => { const startCh = channel('datadog:sequelize:query:start') const finishCh = channel('datadog:sequelize:query:finish') diff --git a/packages/datadog-instrumentations/src/url.js b/packages/datadog-instrumentations/src/url.js index 57248099b4e..407f683ef5c 100644 --- a/packages/datadog-instrumentations/src/url.js +++ b/packages/datadog-instrumentations/src/url.js @@ -2,13 +2,12 @@ const { addHook, channel } = require('./helpers/instrument') const shimmer = require('../../datadog-shimmer') -const names = ['url', 'node:url'] const parseFinishedChannel = channel('datadog:url:parse:finish') const urlGetterChannel = channel('datadog:url:getter:finish') const instrumentedGetters = ['host', 'origin', 'hostname'] -addHook({ name: names }, function (url) { +addHook({ name: 'url' }, function (url) { shimmer.wrap(url, 'parse', (parse) => { return function wrappedParse (input) { const parsedValue = parse.apply(this, arguments) diff --git a/packages/datadog-instrumentations/src/vm.js b/packages/datadog-instrumentations/src/vm.js index 9df229556fa..2e4a3b7e625 100644 --- a/packages/datadog-instrumentations/src/vm.js +++ b/packages/datadog-instrumentations/src/vm.js @@ -2,12 +2,11 @@ const { channel, addHook } = require('./helpers/instrument') const shimmer = require('../../datadog-shimmer') -const names = ['vm', 'node:vm'] const runScriptStartChannel = channel('datadog:vm:run-script:start') const sourceTextModuleStartChannel = channel('datadog:vm:source-text-module:start') -addHook({ name: names }, function (vm) { +addHook({ name: 'vm' }, function (vm) { vm.Script = class extends vm.Script { constructor (code) { super(...arguments) diff --git a/packages/dd-trace/src/appsec/iast/iast-plugin.js b/packages/dd-trace/src/appsec/iast/iast-plugin.js index 9c6dc57c921..4b8e4689d0a 100644 --- a/packages/dd-trace/src/appsec/iast/iast-plugin.js +++ b/packages/dd-trace/src/appsec/iast/iast-plugin.js @@ -162,7 +162,7 @@ class IastPlugin extends Plugin { loadChannel.subscribe(this.onInstrumentationLoadedListener) // check for already instrumented modules - for (const name in instrumentations) { + for (const name of Object.keys(instrumentations)) { this._onInstrumentationLoaded(name) } } diff --git a/packages/dd-trace/src/lambda/runtime/ritm.js b/packages/dd-trace/src/lambda/runtime/ritm.js index 960e7fd7cd9..fcc6f0a2530 100644 --- a/packages/dd-trace/src/lambda/runtime/ritm.js +++ b/packages/dd-trace/src/lambda/runtime/ritm.js @@ -89,12 +89,12 @@ const registerLambdaHook = () => { const lambdaFilePaths = _getLambdaFilePaths(lambdaStylePath) // TODO: Redo this like any other instrumentation. - Hook(lambdaFilePaths, (moduleExports, name) => { + Hook(lambdaFilePaths, (moduleExports, name, _, moduleVersion) => { require('./patch') for (const { hook } of instrumentations[name]) { try { - moduleExports = hook(moduleExports) + moduleExports = hook(moduleExports, moduleVersion) ?? moduleExports } catch (e) { log.error('Error executing lambda hook', e) } @@ -104,16 +104,16 @@ const registerLambdaHook = () => { }) } else { const moduleToPatch = 'datadog-lambda-js' - Hook([moduleToPatch], (moduleExports, moduleName, _) => { + Hook([moduleToPatch], (moduleExports, moduleName, _, moduleVersion) => { moduleName = moduleName.replace(pathSepExpr, '/') require('./patch') - for (const { name, file, hook } of instrumentations[moduleToPatch]) { - const fullFilename = filename(name, file) + for (const { file, hook } of instrumentations[moduleToPatch]) { + const fullFilename = filename(moduleToPatch, file) if (moduleName === fullFilename) { try { - moduleExports = hook(moduleExports) + moduleExports = hook(moduleExports, moduleVersion) ?? moduleExports } catch (e) { log.error('Error executing lambda hook for datadog-lambda-js', e) } diff --git a/packages/dd-trace/src/ritm.js b/packages/dd-trace/src/ritm.js index 07c03c84468..92b36e91077 100644 --- a/packages/dd-trace/src/ritm.js +++ b/packages/dd-trace/src/ritm.js @@ -19,19 +19,32 @@ let patchedRequire = null const moduleLoadStartChannel = dc.channel('dd-trace:moduleLoadStart') const moduleLoadEndChannel = dc.channel('dd-trace:moduleLoadEnd') +/** + * @overload + * @param {string[]} modules list of modules to hook into + * @param {object} options hook options + * @param {Function} onrequire callback to be executed upon encountering module + * + * @overload + * @param {string[]} modules list of modules to hook into + * @param {Function} onrequire callback to be executed upon encountering module + * + * @overload + * @param {Function} onrequire callback to be executed upon encountering module + */ function Hook (modules, options, onrequire) { if (!(this instanceof Hook)) return new Hook(modules, options, onrequire) if (typeof modules === 'function') { onrequire = modules - modules = null + modules = undefined options = {} } else if (typeof options === 'function') { onrequire = options options = {} } - modules = modules || [] - options = options || {} + modules ??= [] + options ??= {} this.modules = modules this.options = options @@ -60,6 +73,7 @@ function Hook (modules, options, onrequire) { */ let filename try { + // @ts-expect-error - Module._resolveFilename is not typed filename = Module._resolveFilename(request, this) } catch { return _origRequire.apply(this, arguments) @@ -70,8 +84,9 @@ function Hook (modules, options, onrequire) { // return known patched modules immediately if (cache[filename]) { // require.cache was potentially altered externally - if (require.cache[filename] && require.cache[filename].exports !== cache[filename].original) { - return require.cache[filename].exports + const cacheEntry = require.cache[filename] + if (cacheEntry && cacheEntry.exports !== cache[filename].original) { + return cacheEntry.exports } return cache[filename].exports @@ -125,6 +140,7 @@ function Hook (modules, options, onrequire) { if (!hooks) return exports // abort if module name isn't on whitelist // figure out if this is the main module file, or a file inside the module + // @ts-expect-error - Module._resolveLookupPaths is meant to be internal and is not typed const paths = Module._resolveLookupPaths(name, this, true) if (!paths) { // abort if _resolveLookupPaths return null @@ -133,6 +149,7 @@ function Hook (modules, options, onrequire) { let res try { + // @ts-expect-error - Module._findPath is meant to be internal and is not typed res = Module._findPath(name, [basedir, ...paths]) } catch { // case where the file specified in package.json "main" doesn't exist @@ -159,6 +176,10 @@ function Hook (modules, options, onrequire) { } } +/** + * Reset the Ritm hook. This is used to reset the hook after a test. + * TODO: Remove this and instead use proxyquire to reset the hook. + */ Hook.reset = function () { Module.prototype.require = origRequire patchedRequire = null @@ -166,19 +187,3 @@ Hook.reset = function () { cache = Object.create(null) moduleHooks = Object.create(null) } - -Hook.prototype.unhook = function () { - for (const mod of this.modules) { - const hooks = (moduleHooks[mod] || []).filter(hook => hook !== this.onrequire) - - if (hooks.length > 0) { - moduleHooks[mod] = hooks - } else { - delete moduleHooks[mod] - } - } - - if (Object.keys(moduleHooks).length === 0) { - Hook.reset() - } -} diff --git a/packages/dd-trace/test/ritm.spec.js b/packages/dd-trace/test/ritm.spec.js index 1fb642dc46c..ca5b45a8a3c 100644 --- a/packages/dd-trace/test/ritm.spec.js +++ b/packages/dd-trace/test/ritm.spec.js @@ -2,7 +2,7 @@ const sinon = require('sinon') const dc = require('dc-polyfill') -const { describe, it, before, beforeEach, afterEach } = require('tap').mocha +const { describe, it, before, beforeEach } = require('tap').mocha const assert = require('node:assert') const Module = require('node:module') @@ -13,23 +13,15 @@ const Hook = require('../src/ritm') describe('Ritm', () => { let moduleLoadStartChannel, moduleLoadEndChannel, startListener, endListener - let utilHook, aHook, bHook, httpHook + const mockedModuleName = '@azure/functions-core' before(() => { moduleLoadStartChannel = dc.channel('dd-trace:moduleLoadStart') moduleLoadEndChannel = dc.channel('dd-trace:moduleLoadEnd') - }) - - beforeEach(() => { - startListener = sinon.fake() - endListener = sinon.fake() - - moduleLoadStartChannel.subscribe(startListener) - moduleLoadEndChannel.subscribe(endListener) Module.prototype.require = new Proxy(Module.prototype.require, { apply (target, thisArg, argArray) { - if (argArray[0] === '@azure/functions-core') { + if (argArray[0] === mockedModuleName) { return { version: '1.0.0', registerHook: () => { } @@ -40,20 +32,23 @@ describe('Ritm', () => { } }) - utilHook = Hook('util') - aHook = Hook('module-a') - bHook = Hook('module-b') - httpHook = new Hook(['http'], function onRequire (exports, name, basedir) { + function onRequire () {} + + Hook(['util'], onRequire) + Hook(['module-a'], onRequire) + Hook(['module-b'], onRequire) + Hook(['http'], function onRequire (exports, name, basedir) { exports.foo = 1 return exports }) }) - afterEach(() => { - utilHook.unhook() - aHook.unhook() - bHook.unhook() - httpHook.unhook() + beforeEach(() => { + startListener = sinon.fake() + endListener = sinon.fake() + + moduleLoadStartChannel.subscribe(startListener) + moduleLoadEndChannel.subscribe(endListener) }) it('should shim util', () => { @@ -88,14 +83,16 @@ describe('Ritm', () => { }) it('should fall back to monkey patched module', () => { + // @ts-expect-error - Patching module works as expected assert.equal(require('http').foo, 1, 'normal hooking still works') - const fnCore = require('@azure/functions-core') + const fnCore = require(mockedModuleName) assert.ok(fnCore, 'requiring monkey patched in module works') assert.equal(fnCore.version, '1.0.0') assert.equal(typeof fnCore.registerHook, 'function') assert.throws( + // @ts-expect-error - Package does not exist () => require('package-does-not-exist'), /Cannot find module 'package-does-not-exist'/, 'a failing `require(...)` can still throw as expected' From 42d8401c9725808f7b1877667764b7eea38f2c2f Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 11 Nov 2025 14:49:47 +0100 Subject: [PATCH 02/11] fixup! remove more unused code This is supported by ritm, we just do not utilize it and do not need it as such. --- packages/dd-trace/src/ritm.js | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/packages/dd-trace/src/ritm.js b/packages/dd-trace/src/ritm.js index 92b36e91077..26d541ff258 100644 --- a/packages/dd-trace/src/ritm.js +++ b/packages/dd-trace/src/ritm.js @@ -28,17 +28,10 @@ const moduleLoadEndChannel = dc.channel('dd-trace:moduleLoadEnd') * @overload * @param {string[]} modules list of modules to hook into * @param {Function} onrequire callback to be executed upon encountering module - * - * @overload - * @param {Function} onrequire callback to be executed upon encountering module */ function Hook (modules, options, onrequire) { if (!(this instanceof Hook)) return new Hook(modules, options, onrequire) - if (typeof modules === 'function') { - onrequire = modules - modules = undefined - options = {} - } else if (typeof options === 'function') { + if (typeof options === 'function') { onrequire = options options = {} } From acfe7223029054e1e6350c40bab27716c44ddd40 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 13 Nov 2025 16:41:01 +0100 Subject: [PATCH 03/11] fixup! mistake in bundler instrumentation --- .../src/helpers/bundler-register.js | 9 ++++++--- .../test/helpers/check-require-cache.spec.js | 1 + 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/datadog-instrumentations/src/helpers/bundler-register.js b/packages/datadog-instrumentations/src/helpers/bundler-register.js index 854444e63e4..4c96b94a36d 100644 --- a/packages/datadog-instrumentations/src/helpers/bundler-register.js +++ b/packages/datadog-instrumentations/src/helpers/bundler-register.js @@ -60,14 +60,17 @@ dc.subscribe(CHANNEL, (message) => { const payload = /** @type {Payload} */ (message) const name = payload.package - const isNodeModule = name.startsWith('node:') || !hooks[name] + const isPrefixedWithNode = name.startsWith('node:') + + const isNodeModule = isPrefixedWithNode || !hooks[name] if (isNodeModule) { + const nodeName = isPrefixedWithNode ? name.slice(5) : name // Used for node: prefixed modules to prevent double instrumentation. - if (instrumentedNodeModules.has(name)) { + if (instrumentedNodeModules.has(nodeName)) { return } - instrumentedNodeModules.add(name) + instrumentedNodeModules.add(nodeName) } doHook(name) diff --git a/packages/datadog-instrumentations/test/helpers/check-require-cache.spec.js b/packages/datadog-instrumentations/test/helpers/check-require-cache.spec.js index fa4e64831d5..cdef758dcce 100644 --- a/packages/datadog-instrumentations/test/helpers/check-require-cache.spec.js +++ b/packages/datadog-instrumentations/test/helpers/check-require-cache.spec.js @@ -16,6 +16,7 @@ describe('check-require-cache', () => { it('should be no warnings when tracer is loaded first', (done) => { exec(`${process.execPath} ./check-require-cache/good-order.js`, opts, (error, stdout, stderr) => { expect(error).to.be.null + expect(stdout).to.not.include("Found incompatible integration version") expect(stderr).to.not.include("Package 'express' was loaded") done() }) From 8b9a44e97384d91d69a0bfd17d281bd5b733106b Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 13 Nov 2025 16:53:37 +0100 Subject: [PATCH 04/11] fixup! mistake --- packages/datadog-instrumentations/src/helpers/register.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/datadog-instrumentations/src/helpers/register.js b/packages/datadog-instrumentations/src/helpers/register.js index c83950d6fb7..6c1e301ae0c 100644 --- a/packages/datadog-instrumentations/src/helpers/register.js +++ b/packages/datadog-instrumentations/src/helpers/register.js @@ -140,7 +140,7 @@ for (const name of names) { } if (matchVersion(version, versions)) { - allInstrumentations[instrumentationFileName] ||= false + allInstrumentations[instrumentationFileName] = true try { loadChannel.publish({ name }) From db40ab3e2de96f477713cf0ade34fc7334d05402 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 13 Nov 2025 16:56:59 +0100 Subject: [PATCH 05/11] fix: always send telemetry for not instrumented code This makes sure not instrumented code will always send out the proper telemetry. Formerly, it would rely on any part of the file being instrumented. Now, we properly check by version again. To guarantee it is only logging it once, it is send with a timeout or before the service ends in case that happens earlier. It also fixes DD_TRACE_DISABLED_INSTRUMENTATIONS to work for prefixed and unprefixed Node automatically in case either is deactivated. Before, both would have to be deactivated. In addition, it simplifies code, adds some JSDoc and makes sure the Node.js version is added to Node.js modules in the telemetry / failures. This also fixes a couple of tests that were not properly checking the behavior as some state was captured in the instrumentation. --- integration-tests/package-guardrails.spec.js | 3 +- .../src/helpers/hook.js | 20 ++ .../src/helpers/register.js | 185 +++++++----------- .../dd-trace/src/exporters/common/request.js | 9 + packages/dd-trace/src/require-package-json.js | 12 +- .../config/disabled_instrumentations.spec.js | 112 ++++++++--- 6 files changed, 194 insertions(+), 147 deletions(-) diff --git a/integration-tests/package-guardrails.spec.js b/integration-tests/package-guardrails.spec.js index 8d194978243..43ee970025e 100644 --- a/integration-tests/package-guardrails.spec.js +++ b/integration-tests/package-guardrails.spec.js @@ -48,8 +48,9 @@ describe('package guardrails', () => { it('should not instrument the package', () => runTest(`Application instrumentation bootstrapping complete -Found incompatible integration version: bluebird@1.0.0 false +instrumentation source: manual +Found incompatible integration version: bluebird@1.0.0 `, [])) }) }) diff --git a/packages/datadog-instrumentations/src/helpers/hook.js b/packages/datadog-instrumentations/src/helpers/hook.js index f73125faf22..ae01816c25f 100644 --- a/packages/datadog-instrumentations/src/helpers/hook.js +++ b/packages/datadog-instrumentations/src/helpers/hook.js @@ -2,6 +2,19 @@ const iitm = require('../../../dd-trace/src/iitm') const path = require('path') const ritm = require('../../../dd-trace/src/ritm') +const log = require('../../../dd-trace/src/log') +const requirePackageJson = require('../../../dd-trace/src/require-package-json') + +/** + * @param {string} moduleBaseDir + * @returns {string|undefined} + */ +function getVersion (moduleBaseDir) { + if (moduleBaseDir) { + return requirePackageJson(moduleBaseDir, /** @type {import('module').Module} */ (module)).version + } + return process.version +} /** * This is called for every package/internal-module that dd-trace supports instrumentation for @@ -39,6 +52,13 @@ function Hook (modules, hookOptions, onrequire) { let defaultWrapResult + try { + moduleVersion ||= getVersion(moduleBaseDir) + } catch (error) { + log.error('Error getting version for "%s": %s', moduleName, error.message, error) + return + } + if ( isIitm && moduleExports.default && diff --git a/packages/datadog-instrumentations/src/helpers/register.js b/packages/datadog-instrumentations/src/helpers/register.js index 6c1e301ae0c..e16c2824d47 100644 --- a/packages/datadog-instrumentations/src/helpers/register.js +++ b/packages/datadog-instrumentations/src/helpers/register.js @@ -4,7 +4,6 @@ const { channel } = require('dc-polyfill') const path = require('path') const satisfies = require('semifies') const Hook = require('./hook') -const requirePackageJson = require('../../../dd-trace/src/require-package-json') const log = require('../../../dd-trace/src/log') const checkRequireCache = require('./check-require-cache') const telemetry = require('../../../dd-trace/src/guardrails/telemetry') @@ -43,26 +42,28 @@ if (DD_TRACE_DEBUG && DD_TRACE_DEBUG.toLowerCase() !== 'false') { setImmediate(checkRequireCache.checkForPotentialConflicts) } -/** @type {Set} */ -const seenCombo = new Set() -/** @type {Record} */ -const allInstrumentations = {} /** @type {Set} */ const instrumentedNodeModules = new Set() +/** @type {Map} */ +const instrumentedIntegrationsSuccess = new Map() +/** @type {Set} */ +const alreadyLoggedIncompatibleIntegrations = new Set() for (const name of names) { if (name.startsWith('node:')) { // Add all unprefixed node modules to the instrumentations list. - names.push(name.slice(5)) + const unprefixedName = name.slice(5) + names.push(unprefixedName) // Always disable prefixed and unprefixed node modules if one is disabled. - // TODO: Activate & write a regression test for this. - // if (disabledInstrumentations.has(name) !== disabledInstrumentations.has(`node:${name}`)) { - // disabledInstrumentations.add(name) - // disabledInstrumentations.add(`node:${name}`) - // } + if (disabledInstrumentations.has(name) !== disabledInstrumentations.has(unprefixedName)) { + disabledInstrumentations.add(name) + disabledInstrumentations.add(unprefixedName) + } } } +let timeout + for (const name of names) { if (disabledInstrumentations.has(name)) continue @@ -81,10 +82,19 @@ for (const name of names) { const nameWithoutPrefix = name.startsWith('node:') ? name.slice(5) : name - // get the instrumentation file name to save all hooked versions - const instrumentationFileName = parseHookInstrumentationFileName(hook) - Hook([name], hookOptions, (moduleExports, moduleName, moduleBaseDir, moduleVersion, isIitm) => { + if (timeout === undefined) { + // Delay the logging of aborted integrations to handle async loading graphs. + timeout = setTimeout(() => { + logAbortedIntegrations() + }, 100).unref() + } else { + timeout.refresh() + } + // All loaded versions are first expected to fail instrumentation. + if (!instrumentedIntegrationsSuccess.has(`${nameWithoutPrefix}@${moduleVersion}`)) { + instrumentedIntegrationsSuccess.set(`${nameWithoutPrefix}@${moduleVersion}`, false) + } moduleName = moduleName.replace(pathSepExpr, '/') // This executes the integration file thus adding its entries to `instrumentations` @@ -99,7 +109,6 @@ for (const name of names) { instrumentedNodeModules.add(nameWithoutPrefix) } - const namesAndSuccesses = {} for (const { file, versions, hook, filePattern, patchDefault } of instrumentations[nameWithoutPrefix]) { if (isIitm && patchDefault === !!moduleExports.default) { if (patchDefault) { @@ -117,79 +126,61 @@ for (const name of names) { if (fullFilePattern) { // Some libraries include a hash in their filenames when installed, // so our instrumentation has to include a '.*' to match them for more than a single version. - matchesFile = matchesFile || new RegExp(fullFilePattern).test(moduleName) + matchesFile ||= new RegExp(fullFilePattern).test(moduleName) } - if (matchesFile) { - let version = moduleVersion + if (matchesFile && matchVersion(moduleVersion, versions)) { + // Do not log in case of an error to prevent duplicate telemetry for the same integration version. + instrumentedIntegrationsSuccess.set(`${nameWithoutPrefix}@${moduleVersion}`, true) try { - version = version || getVersion(moduleBaseDir) - } catch (e) { - log.error('Error getting version for "%s": %s', name, e.message, e) - continue - } - if (namesAndSuccesses[`${nameWithoutPrefix}@${version}`] === undefined && !file) { - // TODO If `file` is present, we might elsewhere instrument the result of the module - // for a version range that actually matches, so we can't assume that we're _not_ - // going to instrument that. However, the way the data model around instrumentation - // works, we can't know either way just yet, so to avoid false positives, we'll just - // ignore this if there is a `file` in the hook. The thing to do here is rework - // everything so that we can be sure that there are _no_ instrumentations that it - // could match. - namesAndSuccesses[`${nameWithoutPrefix}@${version}`] = false - } - - if (matchVersion(version, versions)) { - allInstrumentations[instrumentationFileName] = true - - try { - loadChannel.publish({ name }) - - moduleExports = hook(moduleExports, version) ?? moduleExports - } catch (e) { - log.info('Error during ddtrace instrumentation of application, aborting.', e) - telemetry('error', [ - `error_type:${e.constructor.name}`, - `integration:${nameWithoutPrefix}`, - `integration_version:${version}` - ], { - result: 'error', - result_class: 'internal_error', - result_reason: `Error during instrumentation of ${name}@${version}: ${e.message}` - }) - } - namesAndSuccesses[`${nameWithoutPrefix}@${version}`] = true + loadChannel.publish({ name }) + + moduleExports = hook(moduleExports, moduleVersion) ?? moduleExports + } catch (error) { + log.info('Error during ddtrace instrumentation of application, aborting.', error) + telemetry('error', [ + `error_type:${error.constructor.name}`, + `integration:${nameWithoutPrefix}`, + `integration_version:${moduleVersion}` + ], { + result: 'error', + result_class: 'internal_error', + result_reason: `Error during instrumentation of ${name}@${moduleVersion}: ${error.message}` + }) } } } - for (const nameVersion of Object.keys(namesAndSuccesses)) { - const success = namesAndSuccesses[nameVersion] - // we check allVersions to see if any version of the integration was successfully instrumented - // TODO: The allInstrumentations check is meant to fix https://github.com/DataDog/dd-trace-js/issues/5092 - // This is a workaround to actually detect if any hook inside of the instrumentation file was successful or not. - // This could be simplified by checking it in a different way. Another alternative is to mention which hooks were - // successful or not. That would also be better for debugging. If no filename is detected, the allInstrumentations - // check will always be false - which is a mistake. - // The current seenCombo also seems to be redundant to namesAndSuccesses. Refactor this if possible. - if (!success && !seenCombo.has(nameVersion) && !allInstrumentations[instrumentationFileName]) { - const [name, version] = nameVersion.split('@') - telemetry('abort.integration', [ - `integration:${name}`, - `integration_version:${version}` - ], { - result: 'abort', - result_class: 'incompatible_library', - result_reason: `Incompatible integration version: ${name}@${version}` - }) - log.info('Found incompatible integration version: %s', nameVersion) - seenCombo.add(nameVersion) - } - } return moduleExports }) } +// Used in case the process exits before the timeout is triggered. +process.on('beforeExit', () => { + logAbortedIntegrations() +}) + +function logAbortedIntegrations () { + for (const [nameVersion, success] of instrumentedIntegrationsSuccess) { + // Only ever log a single version of an integration, even if it is loaded later. + if (!success && !alreadyLoggedIncompatibleIntegrations.has(nameVersion)) { + const [name, version] = nameVersion.split('@') + telemetry('abort.integration', [ + `integration:${name}`, + `integration_version:${version}` + ], { + result: 'abort', + result_class: 'incompatible_library', + result_reason: `Incompatible integration version: ${name}@${version}` + }) + log.info('Found incompatible integration version: %s', nameVersion) + alreadyLoggedIncompatibleIntegrations.add(nameVersion) + } + } + // Clear the map to avoid reporting the same integration version again. + instrumentedIntegrationsSuccess.clear() +} + /** * @param {string|undefined} version * @param {string[]|undefined} ranges @@ -198,16 +189,6 @@ function matchVersion (version, ranges) { return !version || !ranges || ranges.some(range => satisfies(version, range)) } -/** - * @param {string} moduleBaseDir - * @returns {string|undefined} - */ -function getVersion (moduleBaseDir) { - if (moduleBaseDir) { - return requirePackageJson(moduleBaseDir, /** @type {import('module').Module} */ (module)).version - } -} - /** * @param {string} name * @param {string} [file] @@ -217,36 +198,6 @@ function filename (name, file) { return file ? `${name}/${file}` : name } -// This function captures the instrumentation file name for a given package by parsing the hook require -// function given the module name. It is used to ensure that instrumentations such as redis -// that have several different modules being hooked, ie: 'redis' main package, and @redis/client submodule -// return a consistent instrumentation name. This is used later to ensure that at least some portion of -// the integration was successfully instrumented. Prevents incorrect `Found incompatible integration version: ` messages -// Example: -// redis -> "() => require('../redis')" -> redis -// @redis/client -> "() => require('../redis')" -> redis -// -/** - * @param {Function} hook - * @returns {string|undefined} - */ -function parseHookInstrumentationFileName (hook) { - const hookString = hook.toString() - - const regex = /require\('([^']*)'\)/ - const match = hookString.match(regex) - - // try to capture the hook require file location. - if (match && match[1]) { - let moduleName = match[1] - // Remove leading '../' if present - if (moduleName.startsWith('../')) { - moduleName = moduleName.slice(3) - } - return moduleName - } -} - module.exports = { filename, pathSepExpr, diff --git a/packages/dd-trace/src/exporters/common/request.js b/packages/dd-trace/src/exporters/common/request.js index a87170bbe57..79fa42b1299 100644 --- a/packages/dd-trace/src/exporters/common/request.js +++ b/packages/dd-trace/src/exporters/common/request.js @@ -18,6 +18,10 @@ const maxActiveRequests = 8 let activeRequests = 0 +/** + * @param {string|URL|object} urlObjOrString + * @returns {object} + */ function parseUrl (urlObjOrString) { if (urlObjOrString !== null && typeof urlObjOrString === 'object') return urlToHttpOptions(urlObjOrString) @@ -33,6 +37,11 @@ function parseUrl (urlObjOrString) { return url } +/** + * @param {Buffer|string|Readable|Array} data + * @param {object} options + * @param {(error: Error|null, result: string, statusCode: number) => void} callback + */ function request (data, options, callback) { if (!options.headers) { options.headers = {} diff --git a/packages/dd-trace/src/require-package-json.js b/packages/dd-trace/src/require-package-json.js index 5d0187016c4..1a054878954 100644 --- a/packages/dd-trace/src/require-package-json.js +++ b/packages/dd-trace/src/require-package-json.js @@ -21,10 +21,14 @@ function requirePackageJson (name, module) { } for (const modulePath of module.paths) { const candidate = path.join(modulePath, name, 'package.json') - try { - return JSON.parse(fs.readFileSync(candidate, 'utf8')) - } catch { - continue + // fs.existsSync is faster than fs.readFileSync due to not throwing an error if the file does not exist. + // The race condition should also not matter here as the time window is very small. + if (fs.existsSync(candidate)) { + try { + return JSON.parse(fs.readFileSync(candidate, 'utf8')) + } catch { + continue + } } } throw new Error(`could not find ${name}/package.json`) diff --git a/packages/dd-trace/test/config/disabled_instrumentations.spec.js b/packages/dd-trace/test/config/disabled_instrumentations.spec.js index 905c9bbd922..ecd417c4a65 100644 --- a/packages/dd-trace/test/config/disabled_instrumentations.spec.js +++ b/packages/dd-trace/test/config/disabled_instrumentations.spec.js @@ -1,34 +1,96 @@ 'use strict' -const { expect } = require('chai') +const assert = require('node:assert') + +const proxyquire = require('proxyquire') const { describe, it } = require('tap').mocha require('../setup/core') -describe('config/disabled_instrumentations', () => { - it('should disable loading instrumentations completely', () => { - process.env.DD_TRACE_DISABLED_INSTRUMENTATIONS = 'express' - const handleBefore = require('express').application.handle - const tracer = require('../../../..') - const handleAfterImport = require('express').application.handle - tracer.init() - const handleAfterInit = require('express').application.handle - - expect(handleBefore).to.equal(handleAfterImport) - expect(handleBefore).to.equal(handleAfterInit) - delete process.env.DD_TRACE_DISABLED_INSTRUMENTATIONS - }) +describe('config/instrumentations', () => { + const httpRequest = require('http').request + const expressHandle = require('express').application.handle + + function getTracer () { + const register = proxyquire.noPreserveCache()('../../../datadog-instrumentations/src/helpers/register', {}) + const instrumentations = proxyquire('../../../datadog-instrumentations/src/helpers/instrumentations', { + './src/helpers/register': register + }) + const pluginManager = proxyquire('../../src/plugin_manager', { + '../../datadog-instrumentations': instrumentations + }) + const proxy = proxyquire('../../src/proxy', { + './plugin_manager': pluginManager + }) + const TracerProxy = proxyquire('../../src', { + './proxy': proxy + }) + return proxyquire('../../', { + './src': TracerProxy + }) + } + + ['disable', 'enable'].forEach((mode) => { + /** @type {(a: unknown, b: unknown) => void} */ + const assertionMethod = mode === 'disable' ? assert.strictEqual : assert.notStrictEqual + + describe(`config/${mode}_instrumentations`, () => { + it(`should ${mode} node prefixed and unprefixed http instrumentations completely`, () => { + if (mode === 'disable') { + process.env.DD_TRACE_DISABLED_INSTRUMENTATIONS = 'http,express' + } + const tracer = getTracer() + const prefixedHandleAfterImport = require('node:http').request + const handleAfterImport = require('http').request + tracer.init() + const prefixedHandleAfterInit = require('http').request + const handleAfterInit = require('http').request + + assertionMethod(httpRequest, handleAfterImport) + assertionMethod(httpRequest, handleAfterInit) + assertionMethod(httpRequest, prefixedHandleAfterImport) + assertionMethod(httpRequest, prefixedHandleAfterInit) + assert.strictEqual(handleAfterImport, handleAfterInit) + assert.strictEqual(prefixedHandleAfterImport, prefixedHandleAfterInit) + delete process.env.DD_TRACE_DISABLED_INSTRUMENTATIONS + }) + + it(`should ${mode} loading instrumentations completely`, () => { + if (mode === 'disable') { + process.env.DD_TRACE_DISABLED_INSTRUMENTATIONS = 'express' + } + const tracer = getTracer() + // Ensure Express is reloaded through the instrumentation hook by clearing Node's require cache. + delete require.cache[require.resolve('express')] + // @ts-expect-error Express handle is not typed as it is an internal property + const handleAfterImport = require('express').application.handle + tracer.init() + // Reload again post-init to validate behavior after tracer initialization. + // @ts-expect-error Express handle is not typed as it is an internal property + const handleAfterInit = require('express').application.handle + + assertionMethod(expressHandle, handleAfterImport) + assertionMethod(expressHandle, handleAfterInit) + delete process.env.DD_TRACE_DISABLED_INSTRUMENTATIONS + }) + + if (mode === 'disable') { + it('should not disable loading instrumentations using DD_TRACE__ENABLED', () => { + process.env.DD_TRACE_EXPRESS_ENABLED = 'false' + const tracer = getTracer() + delete require.cache[require.resolve('express')] + // @ts-expect-error Express handle is not typed as it is an internal property + const handleAfterImport = require('express').application.handle + tracer.init() + // @ts-expect-error Express handle is not typed as it is an internal property + const handleAfterInit = require('express').application.handle - it('should disable loading instrumentations using DD_TRACE__ENABLED', () => { - process.env.DD_TRACE_EXPRESS_ENABLED = 'false' - const handleBefore = require('express').application.handle - const tracer = require('../../../..') - const handleAfterImport = require('express').application.handle - tracer.init() - const handleAfterInit = require('express').application.handle - - expect(handleBefore).to.equal(handleAfterImport) - expect(handleBefore).to.equal(handleAfterInit) - delete process.env.DD_TRACE_EXPRESS_ENABLED + assert.notStrictEqual(expressHandle, handleAfterImport) + assert.notStrictEqual(expressHandle, handleAfterInit) + assert.strictEqual(handleAfterImport, handleAfterInit) + delete process.env.DD_TRACE_EXPRESS_ENABLED + }) + } + }) }) }) From 7d8e711e8c758a374aa0a361867edb50eabae544 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 13 Nov 2025 17:10:20 +0100 Subject: [PATCH 06/11] fixup! linter --- .../test/helpers/check-require-cache.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/datadog-instrumentations/test/helpers/check-require-cache.spec.js b/packages/datadog-instrumentations/test/helpers/check-require-cache.spec.js index cdef758dcce..960ae8aedad 100644 --- a/packages/datadog-instrumentations/test/helpers/check-require-cache.spec.js +++ b/packages/datadog-instrumentations/test/helpers/check-require-cache.spec.js @@ -16,7 +16,7 @@ describe('check-require-cache', () => { it('should be no warnings when tracer is loaded first', (done) => { exec(`${process.execPath} ./check-require-cache/good-order.js`, opts, (error, stdout, stderr) => { expect(error).to.be.null - expect(stdout).to.not.include("Found incompatible integration version") + expect(stdout).to.not.include('Found incompatible integration version') expect(stderr).to.not.include("Package 'express' was loaded") done() }) From dc425bd7384b9cc8133d0447f97847df092f6f39 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 13 Nov 2025 22:49:37 +0100 Subject: [PATCH 07/11] fixup! IITM already handles prefixed node modules --- .../src/helpers/hooks.js | 23 ++++---- .../src/helpers/register.js | 58 ++++++++++--------- 2 files changed, 44 insertions(+), 37 deletions(-) diff --git a/packages/datadog-instrumentations/src/helpers/hooks.js b/packages/datadog-instrumentations/src/helpers/hooks.js index d27b670b07b..bb4d70d6189 100644 --- a/packages/datadog-instrumentations/src/helpers/hooks.js +++ b/packages/datadog-instrumentations/src/helpers/hooks.js @@ -1,6 +1,18 @@ 'use strict' module.exports = { + // Only list unprefixed node modules. They will automatically be instrumented as prefixed and unprefixed. + child_process: () => require('../child_process'), + crypto: () => require('../crypto'), + dns: () => require('../dns'), + fs: { serverless: false, fn: () => require('../fs') }, + http: () => require('../http'), + http2: () => require('../http2'), + https: () => require('../http'), + net: () => require('../net'), + url: () => require('../url'), + vm: () => require('../vm'), + // Non Node.js modules '@anthropic-ai/sdk': { esmFirst: true, fn: () => require('../anthropic') }, '@apollo/server': () => require('../apollo-server'), '@apollo/gateway': () => require('../apollo'), @@ -92,17 +104,6 @@ module.exports = { mysql2: () => require('../mysql2'), next: () => require('../next'), 'node-serialize': () => require('../node-serialize'), - // Only list prefixed node modules. They will automatically be instrumented as prefixed and unprefixed. - 'node:child_process': () => require('../child_process'), - 'node:crypto': () => require('../crypto'), - 'node:dns': () => require('../dns'), - 'node:fs': { serverless: false, fn: () => require('../fs') }, - 'node:http': () => require('../http'), - 'node:http2': () => require('../http2'), - 'node:https': () => require('../http'), - 'node:net': () => require('../net'), - 'node:url': () => require('../url'), - 'node:vm': () => require('../vm'), nyc: () => require('../nyc'), oracledb: () => require('../oracledb'), openai: { esmFirst: true, fn: () => require('../openai') }, diff --git a/packages/datadog-instrumentations/src/helpers/register.js b/packages/datadog-instrumentations/src/helpers/register.js index e16c2824d47..6edefca1fb2 100644 --- a/packages/datadog-instrumentations/src/helpers/register.js +++ b/packages/datadog-instrumentations/src/helpers/register.js @@ -1,8 +1,11 @@ 'use strict' -const { channel } = require('dc-polyfill') +const { builtinModules } = require('module') const path = require('path') + +const { channel } = require('dc-polyfill') const satisfies = require('semifies') + const Hook = require('./hook') const log = require('../../../dd-trace/src/log') const checkRequireCache = require('./check-require-cache') @@ -42,24 +45,30 @@ if (DD_TRACE_DEBUG && DD_TRACE_DEBUG.toLowerCase() !== 'false') { setImmediate(checkRequireCache.checkForPotentialConflicts) } -/** @type {Set} */ -const instrumentedNodeModules = new Set() +/** @type {Map} */ +const instrumentedNodeModules = new Map() /** @type {Map} */ const instrumentedIntegrationsSuccess = new Map() /** @type {Set} */ const alreadyLoggedIncompatibleIntegrations = new Set() -for (const name of names) { - if (name.startsWith('node:')) { - // Add all unprefixed node modules to the instrumentations list. - const unprefixedName = name.slice(5) - names.push(unprefixedName) - // Always disable prefixed and unprefixed node modules if one is disabled. - if (disabledInstrumentations.has(name) !== disabledInstrumentations.has(unprefixedName)) { - disabledInstrumentations.add(name) - disabledInstrumentations.add(unprefixedName) +// Always disable prefixed and unprefixed node modules if one is disabled. +if (disabledInstrumentations.size) { + const builtinsSet = new Set(builtinModules) + for (const name of disabledInstrumentations) { + const hasPrefix = name.startsWith('node:') + if (hasPrefix || builtinsSet.has(name)) { + if (hasPrefix) { + const unprefixedName = name.slice(5) + if (!disabledInstrumentations.has(unprefixedName)) { + disabledInstrumentations.add(unprefixedName) + } + } else if (!disabledInstrumentations.has(`node:${name}`)) { + disabledInstrumentations.add(`node:${name}`) + } } } + builtinsSet.clear() } let timeout @@ -67,11 +76,9 @@ let timeout for (const name of names) { if (disabledInstrumentations.has(name)) continue - const isNodeModule = name.startsWith('node:') || !hooks[name] - const hookOptions = {} - let hook = hooks[name] ?? hooks[`node:${name}`] + let hook = hooks[name] if (hook !== null && typeof hook === 'object') { if (hook.serverless === false && isInServerlessEnvironment()) continue @@ -80,8 +87,6 @@ for (const name of names) { hook = hook.fn } - const nameWithoutPrefix = name.startsWith('node:') ? name.slice(5) : name - Hook([name], hookOptions, (moduleExports, moduleName, moduleBaseDir, moduleVersion, isIitm) => { if (timeout === undefined) { // Delay the logging of aborted integrations to handle async loading graphs. @@ -92,24 +97,25 @@ for (const name of names) { timeout.refresh() } // All loaded versions are first expected to fail instrumentation. - if (!instrumentedIntegrationsSuccess.has(`${nameWithoutPrefix}@${moduleVersion}`)) { - instrumentedIntegrationsSuccess.set(`${nameWithoutPrefix}@${moduleVersion}`, false) + if (!instrumentedIntegrationsSuccess.has(`${name}@${moduleVersion}`)) { + instrumentedIntegrationsSuccess.set(`${name}@${moduleVersion}`, false) } - moduleName = moduleName.replace(pathSepExpr, '/') // This executes the integration file thus adding its entries to `instrumentations` hook() - if (!instrumentations[nameWithoutPrefix] || instrumentedNodeModules.has(nameWithoutPrefix)) { + if (!instrumentations[name] || moduleExports === instrumentedNodeModules.get(name)) { return moduleExports } // Used for node: prefixed modules to prevent double instrumentation. - if (isNodeModule) { - instrumentedNodeModules.add(nameWithoutPrefix) + if (moduleBaseDir) { + moduleName = moduleName.replace(pathSepExpr, '/') + } else { + instrumentedNodeModules.set(name, moduleExports) } - for (const { file, versions, hook, filePattern, patchDefault } of instrumentations[nameWithoutPrefix]) { + for (const { file, versions, hook, filePattern, patchDefault } of instrumentations[name]) { if (isIitm && patchDefault === !!moduleExports.default) { if (patchDefault) { moduleExports = moduleExports.default @@ -131,7 +137,7 @@ for (const name of names) { if (matchesFile && matchVersion(moduleVersion, versions)) { // Do not log in case of an error to prevent duplicate telemetry for the same integration version. - instrumentedIntegrationsSuccess.set(`${nameWithoutPrefix}@${moduleVersion}`, true) + instrumentedIntegrationsSuccess.set(`${name}@${moduleVersion}`, true) try { loadChannel.publish({ name }) @@ -140,7 +146,7 @@ for (const name of names) { log.info('Error during ddtrace instrumentation of application, aborting.', error) telemetry('error', [ `error_type:${error.constructor.name}`, - `integration:${nameWithoutPrefix}`, + `integration:${name}`, `integration_version:${moduleVersion}` ], { result: 'error', From 21af9f5a4a25b03b514679d88fce277ccd2b1167 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 13 Nov 2025 23:30:27 +0100 Subject: [PATCH 08/11] fixup! better ritm test --- packages/dd-trace/test/ritm.spec.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/dd-trace/test/ritm.spec.js b/packages/dd-trace/test/ritm.spec.js index ca5b45a8a3c..18eb9b5faa1 100644 --- a/packages/dd-trace/test/ritm.spec.js +++ b/packages/dd-trace/test/ritm.spec.js @@ -52,7 +52,7 @@ describe('Ritm', () => { }) it('should shim util', () => { - require('util') + require('node:util') assert.equal(startListener.callCount, 1) assert.equal(endListener.callCount, 1) }) @@ -84,7 +84,7 @@ describe('Ritm', () => { it('should fall back to monkey patched module', () => { // @ts-expect-error - Patching module works as expected - assert.equal(require('http').foo, 1, 'normal hooking still works') + assert.equal(require('node:http').foo, 1, 'normal hooking still works') const fnCore = require(mockedModuleName) assert.ok(fnCore, 'requiring monkey patched in module works') From aef3b3fd4c96b8060700eb841cdb156836bfd1c6 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 20 Nov 2025 13:21:33 +0900 Subject: [PATCH 09/11] fixup! linter --- eslint.config.mjs | 3 ++- packages/datadog-instrumentations/src/helpers/hook.js | 3 ++- packages/dd-trace/src/ritm.js | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/eslint.config.mjs b/eslint.config.mjs index be9763c6576..c9dedbea924 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -219,7 +219,8 @@ export default [ 'import/no-duplicates': 'error', 'import/no-named-default': 'error', 'import/no-webpack-loader-syntax': 'error', - 'jsdoc/check-tag-names': ['error', { definedTags: ['datadog'] }], + 'jsdoc/check-param-names': ['warn', { disableMissingParamChecks: true }], + 'jsdoc/check-tag-names': ['warn', { definedTags: ['datadog'] }], // TODO: Enable the rules that we want to use. 'jsdoc/check-types': 'off', // Should be activated, but it needs a couple of fixes. // no-defaults: This should be activated, since the defaults will not be picked up in a description. diff --git a/packages/datadog-instrumentations/src/helpers/hook.js b/packages/datadog-instrumentations/src/helpers/hook.js index ae01816c25f..c16335bc43d 100644 --- a/packages/datadog-instrumentations/src/helpers/hook.js +++ b/packages/datadog-instrumentations/src/helpers/hook.js @@ -24,7 +24,8 @@ function getVersion (moduleBaseDir) { * @param {string[]} modules list of modules to hook into * @param {object} hookOptions hook options * @param {Function} onrequire callback to be executed upon encountering module - * + */ +/** * @overload * @param {string[]} modules list of modules to hook into * @param {object} hookOptions hook options diff --git a/packages/dd-trace/src/ritm.js b/packages/dd-trace/src/ritm.js index 26d541ff258..f87f2f25729 100644 --- a/packages/dd-trace/src/ritm.js +++ b/packages/dd-trace/src/ritm.js @@ -24,7 +24,8 @@ const moduleLoadEndChannel = dc.channel('dd-trace:moduleLoadEnd') * @param {string[]} modules list of modules to hook into * @param {object} options hook options * @param {Function} onrequire callback to be executed upon encountering module - * + */ +/** * @overload * @param {string[]} modules list of modules to hook into * @param {Function} onrequire callback to be executed upon encountering module From b98493488e76487b395deb13b052a5e86dd9503a Mon Sep 17 00:00:00 2001 From: pabloerhard Date: Mon, 24 Nov 2025 11:30:56 -0500 Subject: [PATCH 10/11] Added default node prefix instrumetnation for builtin modules --- packages/datadog-esbuild/index.js | 27 +++++--- .../src/helpers/bundler-register.js | 2 +- .../src/helpers/instrument.js | 11 +++- .../src/helpers/register.js | 6 ++ packages/dd-trace/src/ritm.js | 66 ++++++++++++++----- 5 files changed, 84 insertions(+), 28 deletions(-) diff --git a/packages/datadog-esbuild/index.js b/packages/datadog-esbuild/index.js index 64582c0fe17..61c30190538 100644 --- a/packages/datadog-esbuild/index.js +++ b/packages/datadog-esbuild/index.js @@ -24,19 +24,30 @@ for (const hook of Object.values(hooks)) { } } +function moduleOfInterestKey (name, file) { + return file ? `${name}/${file}` : name +} + +const builtinModules = new Set(require('module').builtinModules) + +function addModuleOfInterest (name, file) { + if (!name) return + + modulesOfInterest.add(moduleOfInterestKey(name, file)) + + if (builtinModules.has(name)) { + modulesOfInterest.add(moduleOfInterestKey(`node:${name}`, file)) + } +} + const modulesOfInterest = new Set() -for (const [name, instrumentation] of Object.entries(instrumentations)) { +for (const instrumentation of Object.values(instrumentations)) { for (const entry of instrumentation) { - if (!entry.file) { - modulesOfInterest.add(name) // e.g. "redis" - } else { - modulesOfInterest.add(`${name}/${entry.file}`) // e.g. "redis/my/file.js" - } + addModuleOfInterest(entry.name, entry.file) } } -const RAW_BUILTINS = require('module').builtinModules const CHANNEL = 'dd-trace:bundler:load' const path = require('path') const fs = require('fs') @@ -44,7 +55,7 @@ const { execSync } = require('child_process') const builtins = new Set() -for (const builtin of RAW_BUILTINS) { +for (const builtin of builtinModules) { builtins.add(builtin) builtins.add(`node:${builtin}`) } diff --git a/packages/datadog-instrumentations/src/helpers/bundler-register.js b/packages/datadog-instrumentations/src/helpers/bundler-register.js index 4c96b94a36d..7253729dcc7 100644 --- a/packages/datadog-instrumentations/src/helpers/bundler-register.js +++ b/packages/datadog-instrumentations/src/helpers/bundler-register.js @@ -82,7 +82,7 @@ dc.subscribe(CHANNEL, (message) => { return } - for (const { file, versions, hook } of instrumentation) { + for (const { name, file, versions, hook } of instrumentation) { if (payload.path !== filename(name, file) || !matchVersion(payload.version, versions)) { continue } diff --git a/packages/datadog-instrumentations/src/helpers/instrument.js b/packages/datadog-instrumentations/src/helpers/instrument.js index 9237ef91793..fe672e49676 100644 --- a/packages/datadog-instrumentations/src/helpers/instrument.js +++ b/packages/datadog-instrumentations/src/helpers/instrument.js @@ -51,11 +51,16 @@ exports.tracingChannel = function (name) { * @param {import('./instrumentations').Hook} hook */ exports.addHook = function addHook ({ name, versions, file, filePattern, patchDefault }, hook) { - if (!instrumentations[name]) { - instrumentations[name] = [] + if (typeof name === 'string') { + name = [name] } - instrumentations[name].push({ versions, file, filePattern, hook, patchDefault }) + for (const val of name) { + if (!instrumentations[val]) { + instrumentations[val] = [] + } + instrumentations[val].push({ name: val, versions, file, filePattern, hook, patchDefault }) + } } exports.AsyncResource = AsyncResource diff --git a/packages/datadog-instrumentations/src/helpers/register.js b/packages/datadog-instrumentations/src/helpers/register.js index 6edefca1fb2..6e96a028cc3 100644 --- a/packages/datadog-instrumentations/src/helpers/register.js +++ b/packages/datadog-instrumentations/src/helpers/register.js @@ -30,6 +30,7 @@ const disabledInstrumentations = new Set( ) const loadChannel = channel('dd-trace:instrumentation:load') +const HOOK_SYMBOL = Symbol('hookExportsSet') // Globals if (!disabledInstrumentations.has('fetch')) { @@ -124,6 +125,7 @@ for (const name of names) { } } + hook[HOOK_SYMBOL] ??= new WeakSet() const fullFilename = filename(name, file) let matchesFile = moduleName === fullFilename @@ -136,12 +138,16 @@ for (const name of names) { } if (matchesFile && matchVersion(moduleVersion, versions)) { + if (hook[HOOK_SYMBOL].has(moduleExports)) { + return moduleExports + } // Do not log in case of an error to prevent duplicate telemetry for the same integration version. instrumentedIntegrationsSuccess.set(`${name}@${moduleVersion}`, true) try { loadChannel.publish({ name }) moduleExports = hook(moduleExports, moduleVersion) ?? moduleExports + hook[HOOK_SYMBOL].add(moduleExports) } catch (error) { log.info('Error during ddtrace instrumentation of application, aborting.', error) telemetry('error', [ diff --git a/packages/dd-trace/src/ritm.js b/packages/dd-trace/src/ritm.js index f87f2f25729..f0b66d25c25 100644 --- a/packages/dd-trace/src/ritm.js +++ b/packages/dd-trace/src/ritm.js @@ -19,6 +19,39 @@ let patchedRequire = null const moduleLoadStartChannel = dc.channel('dd-trace:moduleLoadStart') const moduleLoadEndChannel = dc.channel('dd-trace:moduleLoadEnd') +function stripNodeProtocol (name) { + if (typeof name !== 'string') return name + return name.startsWith('node:') ? name.slice(5) : name +} + +const builtinModules = new Set(Module.builtinModules.map(stripNodeProtocol)) + +function isBuiltinModuleName (name) { + if (typeof name !== 'string') return false + return builtinModules.has(stripNodeProtocol(name)) +} + +function normalizeModuleName (name) { + if (typeof name !== 'string') return name + const stripped = stripNodeProtocol(name) + return builtinModules.has(stripped) ? stripped : name +} + +function normalizeModulesList (modules) { + const normalized = [] + const seen = new Set() + + for (const mod of modules) { + const normalizedName = normalizeModuleName(mod) + if (typeof normalizedName !== 'string') continue + if (seen.has(normalizedName)) continue + seen.add(normalizedName) + normalized.push(normalizedName) + } + + return normalized +} + /** * @overload * @param {string[]} modules list of modules to hook into @@ -37,15 +70,15 @@ function Hook (modules, options, onrequire) { options = {} } - modules ??= [] + const normalizedModules = Array.isArray(modules) ? normalizeModulesList(modules) : [] options ??= {} - this.modules = modules + this.modules = normalizedModules this.options = options this.onrequire = onrequire if (Array.isArray(modules)) { - for (const mod of modules) { + for (const mod of normalizedModules) { const hooks = moduleHooks[mod] if (hooks) { @@ -73,27 +106,28 @@ function Hook (modules, options, onrequire) { return _origRequire.apply(this, arguments) } - const core = !filename.includes(path.sep) + const builtin = isBuiltinModuleName(filename) + const moduleId = builtin ? normalizeModuleName(filename) : filename let name, basedir, hooks // return known patched modules immediately - if (cache[filename]) { + if (cache[moduleId]) { // require.cache was potentially altered externally const cacheEntry = require.cache[filename] if (cacheEntry && cacheEntry.exports !== cache[filename].original) { return cacheEntry.exports } - return cache[filename].exports + return cache[moduleId].exports } // Check if this module has a patcher in-progress already. // Otherwise, mark this module as patching in-progress. - const patched = patching[filename] + const patched = patching[moduleId] if (patched) { // If it's already patched, just return it as-is. return origRequire.apply(this, arguments) } - patching[filename] = true + patching[moduleId] = true const payload = { filename, @@ -112,12 +146,12 @@ function Hook (modules, options, onrequire) { // The module has already been loaded, // so the patching mark can be cleaned up. - delete patching[filename] + delete patching[moduleId] - if (core) { - hooks = moduleHooks[filename] + if (builtin) { + hooks = moduleHooks[moduleId] if (!hooks) return exports // abort if module name isn't on whitelist - name = filename + name = moduleId } else { const inAWSLambda = getEnvironmentVariable('AWS_LAMBDA_FUNCTION_NAME') !== undefined const hasLambdaHandler = getEnvironmentVariable('DD_LAMBDA_HANDLER') !== undefined @@ -159,14 +193,14 @@ function Hook (modules, options, onrequire) { // ensure that the cache entry is assigned a value before calling // onrequire, in case calling onrequire requires the same module. - cache[filename] = { exports } - cache[filename].original = exports + cache[moduleId] = { exports } + cache[moduleId].original = exports for (const hook of hooks) { - cache[filename].exports = hook(cache[filename].exports, name, basedir) + cache[moduleId].exports = hook(cache[moduleId].exports, name, basedir) } - return cache[filename].exports + return cache[moduleId].exports } } From 5284a1071321a285651c542707f641531e025a2c Mon Sep 17 00:00:00 2001 From: pabloerhard Date: Tue, 25 Nov 2025 16:37:20 -0500 Subject: [PATCH 11/11] removed unnecesary normalization of list --- packages/dd-trace/src/ritm.js | 22 ++++------------------ 1 file changed, 4 insertions(+), 18 deletions(-) diff --git a/packages/dd-trace/src/ritm.js b/packages/dd-trace/src/ritm.js index f0b66d25c25..b1253517df5 100644 --- a/packages/dd-trace/src/ritm.js +++ b/packages/dd-trace/src/ritm.js @@ -37,21 +37,6 @@ function normalizeModuleName (name) { return builtinModules.has(stripped) ? stripped : name } -function normalizeModulesList (modules) { - const normalized = [] - const seen = new Set() - - for (const mod of modules) { - const normalizedName = normalizeModuleName(mod) - if (typeof normalizedName !== 'string') continue - if (seen.has(normalizedName)) continue - seen.add(normalizedName) - normalized.push(normalizedName) - } - - return normalized -} - /** * @overload * @param {string[]} modules list of modules to hook into @@ -70,15 +55,15 @@ function Hook (modules, options, onrequire) { options = {} } - const normalizedModules = Array.isArray(modules) ? normalizeModulesList(modules) : [] options ??= {} + modules ??= [] - this.modules = normalizedModules + this.modules = modules this.options = options this.onrequire = onrequire if (Array.isArray(modules)) { - for (const mod of normalizedModules) { + for (const mod of modules) { const hooks = moduleHooks[mod] if (hooks) { @@ -108,6 +93,7 @@ function Hook (modules, options, onrequire) { const builtin = isBuiltinModuleName(filename) const moduleId = builtin ? normalizeModuleName(filename) : filename + let name, basedir, hooks // return known patched modules immediately if (cache[moduleId]) {