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/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-esbuild/index.js b/packages/datadog-esbuild/index.js index 97a318ac3ba..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 instrumentation of Object.values(instrumentations)) { for (const entry of instrumentation) { - if (!entry.file) { - modulesOfInterest.add(entry.name) // e.g. "redis" - } else { - modulesOfInterest.add(`${entry.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/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..7253729dcc7 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,73 @@ 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 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(nodeName)) { + return + } + instrumentedNodeModules.add(nodeName) + } + + 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 { name, 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..c16335bc43d 100644 --- a/packages/datadog-instrumentations/src/helpers/hook.js +++ b/packages/datadog-instrumentations/src/helpers/hook.js @@ -2,16 +2,37 @@ 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 * 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') { @@ -32,6 +53,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 && @@ -60,10 +88,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..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'), @@ -42,31 +54,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,18 +102,8 @@ module.exports = { multer: () => require('../multer'), mysql: () => require('../mysql'), mysql2: () => require('../mysql2'), - net: () => require('../net'), next: () => require('../next'), 'node-serialize': () => require('../node-serialize'), - 'node:child_process': () => require('../child_process'), - 'node:crypto': () => require('../crypto'), - 'node:dns': () => require('../dns'), - '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') }, @@ -135,9 +130,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..fe672e49676 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,12 +43,12 @@ 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') { 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..6e96a028cc3 100644 --- a/packages/datadog-instrumentations/src/helpers/register.js +++ b/packages/datadog-instrumentations/src/helpers/register.js @@ -1,10 +1,12 @@ '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 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') @@ -28,6 +30,7 @@ const disabledInstrumentations = new Set( ) const loadChannel = channel('dd-trace:instrumentation:load') +const HOOK_SYMBOL = Symbol('hookExportsSet') // Globals if (!disabledInstrumentations.has('fetch')) { @@ -38,23 +41,45 @@ 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) } -const seenCombo = new Set() -const allInstrumentations = {} +/** @type {Map} */ +const instrumentedNodeModules = new Map() +/** @type {Map} */ +const instrumentedIntegrationsSuccess = new Map() +/** @type {Set} */ +const alreadyLoggedIncompatibleIntegrations = new Set() + +// 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() +} -// TODO: make this more efficient -for (const packageName of names) { - if (disabledInstrumentations.has(packageName)) continue +let timeout + +for (const name of names) { + if (disabledInstrumentations.has(name)) continue const hookOptions = {} - let hook = hooks[packageName] + let hook = hooks[name] if (hook !== null && typeof hook === 'object') { if (hook.serverless === false && isInServerlessEnvironment()) continue @@ -63,172 +88,126 @@ for (const packageName of names) { hook = hook.fn } - // get the instrumentation file name to save all hooked versions - const instrumentationFileName = parseHookInstrumentationFileName(packageName) - - Hook([packageName], hookOptions, (moduleExports, moduleName, moduleBaseDir, moduleVersion, isIitm) => { - moduleName = moduleName.replace(pathSepExpr, '/') + 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(`${name}@${moduleVersion}`)) { + instrumentedIntegrationsSuccess.set(`${name}@${moduleVersion}`, false) + } // This executes the integration file thus adding its entries to `instrumentations` hook() - if (!instrumentations[packageName]) { + if (!instrumentations[name] || moduleExports === instrumentedNodeModules.get(name)) { return moduleExports } - 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 - } + // Used for node: prefixed modules to prevent double instrumentation. + if (moduleBaseDir) { + moduleName = moduleName.replace(pathSepExpr, '/') + } else { + instrumentedNodeModules.set(name, moduleExports) + } - let fullFilePattern = filePattern - const fullFilename = filename(name, file) - if (fullFilePattern) { - fullFilePattern = filename(name, fullFilePattern) + for (const { file, versions, hook, filePattern, patchDefault } of instrumentations[name]) { + if (isIitm && patchDefault === !!moduleExports.default) { + if (patchDefault) { + moduleExports = moduleExports.default + } else { + return moduleExports + } } - // 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() + const fullFilename = filename(name, file) + 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. - matchesFile = matchesFile || new RegExp(fullFilePattern).test(moduleName) + matchesFile ||= new RegExp(fullFilePattern).test(moduleName) } - if (matchesFile) { - 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) { - // 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[`${name}@${version}`] = false + if (matchesFile && matchVersion(moduleVersion, versions)) { + if (hook[HOOK_SYMBOL].has(moduleExports)) { + return moduleExports } - - 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 - } - - 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) - } catch (e) { - log.info('Error during ddtrace instrumentation of application, aborting.', e) - telemetry('error', [ - `error_type:${e.constructor.name}`, - `integration:${name}`, - `integration_version:${version}` - ], { - result: 'error', - result_class: 'internal_error', - result_reason: `Error during instrumentation of ${name}@${version}: ${e.message}` - }) - } - namesAndSuccesses[`${name}@${version}`] = true + // 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', [ + `error_type:${error.constructor.name}`, + `integration:${name}`, + `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 [name, version] = nameVersion.split('@') - const success = namesAndSuccesses[nameVersion] - // we check allVersions to see if any version of the integration was successfully instrumented - if (!success && !seenCombo.has(nameVersion) && !allInstrumentations[instrumentationFileName]) { - 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 }) } -function matchVersion (version, ranges) { - return !version || !ranges || ranges.some(range => satisfies(version, range)) -} +// Used in case the process exits before the timeout is triggered. +process.on('beforeExit', () => { + logAbortedIntegrations() +}) -function getVersion (moduleBaseDir) { - if (moduleBaseDir) { - return requirePackageJson(moduleBaseDir, module).version +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() } -function filename (name, file) { - return [name, file].filter(Boolean).join('/') +/** + * @param {string|undefined} version + * @param {string[]|undefined} ranges + */ +function matchVersion (version, ranges) { + return !version || !ranges || ranges.some(range => satisfies(version, range)) } -// 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 -// -function parseHookInstrumentationFileName (packageName) { - let hook = hooks[packageName] - if (hook.fn) { - hook = hook.fn - } - 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 - } - - return null +/** + * @param {string} name + * @param {string} [file] + * @returns {string} + */ +function filename (name, file) { + return file ? `${name}/${file}` : name } 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/datadog-instrumentations/test/helpers/check-require-cache.spec.js b/packages/datadog-instrumentations/test/helpers/check-require-cache.spec.js index 8431aa01102..f2276ce5641 100644 --- a/packages/datadog-instrumentations/test/helpers/check-require-cache.spec.js +++ b/packages/datadog-instrumentations/test/helpers/check-require-cache.spec.js @@ -17,6 +17,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) => { assert.strictEqual(error, null) + expect(stdout).to.not.include('Found incompatible integration version') expect(stderr).to.not.include("Package 'express' was loaded") done() }) 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/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/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/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/src/ritm.js b/packages/dd-trace/src/ritm.js index 07c03c84468..b1253517df5 100644 --- a/packages/dd-trace/src/ritm.js +++ b/packages/dd-trace/src/ritm.js @@ -19,19 +19,44 @@ 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 +} + +/** + * @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 + */ function Hook (modules, options, onrequire) { if (!(this instanceof Hook)) return new Hook(modules, options, onrequire) - if (typeof modules === 'function') { - onrequire = modules - modules = null - options = {} - } else if (typeof options === 'function') { + if (typeof options === 'function') { onrequire = options options = {} } - modules = modules || [] - options = options || {} + options ??= {} + modules ??= [] this.modules = modules this.options = options @@ -60,31 +85,35 @@ 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) } - 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 - 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 + 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, @@ -103,12 +132,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 @@ -125,6 +154,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 +163,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 @@ -148,17 +179,21 @@ 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 } } +/** + * 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 +201,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/config/disabled_instrumentations.spec.js b/packages/dd-trace/test/config/disabled_instrumentations.spec.js index 81ff70d5a1e..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 assert = require('node:assert/strict') +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 - - assert.strictEqual(handleBefore, handleAfterImport) - assert.strictEqual(handleBefore, 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 - - assert.strictEqual(handleBefore, handleAfterImport) - assert.strictEqual(handleBefore, 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 + }) + } + }) }) }) diff --git a/packages/dd-trace/test/ritm.spec.js b/packages/dd-trace/test/ritm.spec.js index 1fb642dc46c..18eb9b5faa1 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,24 +32,27 @@ 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', () => { - require('util') + require('node:util') assert.equal(startListener.callCount, 1) assert.equal(endListener.callCount, 1) }) @@ -88,14 +83,16 @@ describe('Ritm', () => { }) it('should fall back to monkey patched module', () => { - assert.equal(require('http').foo, 1, 'normal hooking still works') + // @ts-expect-error - Patching module works as expected + assert.equal(require('node: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'