fix: improved instrumentation code & related telemetry (#6910)

* 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.
- Removed unused ritm code.
- Removed HOOK_SYMBOL code.

* 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 the data on the
first flush 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.

---------

Co-authored-by: Pablo Erhard <pabloerhard02@gmail.com>

Author: BridgeAR

.github/CODEOWNERS

@@ -231,6 +231,7 @@
 /integration-tests/bun/ @DataDog/lang-platform-js
 /integration-tests/init.spec.js @DataDog/lang-platform-js
 /integration-tests/package-guardrails.spec.js @DataDog/lang-platform-js
+/integration-tests/package-guardrails/flush.js @DataDog/lang-platform-js
 /integration-tests/startup.spec.js @DataDog/lang-platform-js
 
 /packages/datadog-core @DataDog/lang-platform-js

eslint.config.mjs

@@ -269,6 +269,7 @@ export default [
       }],
       'import/no-useless-path-segments': 'error',
       'import/no-webpack-loader-syntax': 'error',
+      'jsdoc/check-param-names': ['error', { disableMissingParamChecks: true }],
       'jsdoc/check-tag-names': ['error', { definedTags: ['datadog'] }],
       // TODO: Enable the rules that we want to use.
       // no-defaults: This should be activated, since the defaults will not be picked up in a description.

integration-tests/package-guardrails.spec.js

@@ -14,6 +14,7 @@ const NODE_OPTIONS = '--require dd-trace/init.js'
 const DD_TRACE_DEBUG = 'true'
 const DD_INJECTION_ENABLED = 'tracing'
 const DD_LOG_LEVEL = 'info'
+const DD_TRACE_FLUSH_INTERVAL = '0'
 const NODE_MAJOR = Number(process.versions.node.split('.')[0])
 const FASTIFY_DEP = NODE_MAJOR < 20 ? 'fastify@4' : 'fastify'
 
@@ -41,6 +42,17 @@ describe('package guardrails', () => {
         ))
     })
 
+    context('when flushing and DD_INJECTION_ENABLED', () => {
+      useEnv({ DD_INJECTION_ENABLED, DD_TRACE_FLUSH_INTERVAL })
+
+      it('should send abort.integration on first flush via diagnostic channel', () =>
+        testFile('package-guardrails/flush.js', 'false\n',
+          ['complete', 'injection_forced:false',
+            'abort.integration', 'integration:bluebird,integration_version:1.0.0',
+          ]
+        ))
+    })
+
     context('with logging disabled', () => {
       it('should not instrument the package', () => runTest('false\n', []))
     })
@@ -50,8 +62,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
 `, []))
     })
   })

integration-tests/package-guardrails/flush.js

@@ -0,0 +1,19 @@
+'use strict'
+
+// Remove only the register.js beforeExit handler so this test verifies
+// that abort.integration comes from the first flush diagnostic channel.
+const beforeExitHandlers = globalThis[Symbol.for('dd-trace')].beforeExitHandlers
+for (const handler of beforeExitHandlers) {
+  if (handler.name === 'logAbortedIntegrations') {
+    beforeExitHandlers.delete(handler)
+  }
+}
+
+const tracer = require('dd-trace')
+const P = require('bluebird')
+
+const isWrapped = P.prototype._then.toString().includes('AsyncResource')
+tracer.trace('first.flush.guardrails', () => {})
+
+// eslint-disable-next-line no-console
+console.log(isWrapped)

packages/datadog-esbuild/index.js

@@ -2,7 +2,6 @@
 
 const { execSync } = require('node:child_process')
 const fs = require('node:fs')
-const RAW_BUILTINS = require('node:module').builtinModules
 const path = require('node:path')
 const { pathToFileURL, fileURLToPath } = require('node:url')
 
@@ -25,23 +24,35 @@ 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 [name, instrumentation] of Object.entries(instrumentations)) {
   for (const entry of instrumentation) {
-    if (entry.file) {
-      modulesOfInterest.add(`${entry.name}/${entry.file}`) // e.g. "redis/my/file.js"
-    } else {
-      modulesOfInterest.add(entry.name) // e.g. "redis"
-    }
+    addModuleOfInterest(name, entry.file)
   }
 }
 
 const CHANNEL = 'dd-trace:bundler:load'
 
 const builtins = new Set()
 
-for (const builtin of RAW_BUILTINS) {
+for (const builtin of builtinModules) {
   builtins.add(builtin)
   builtins.add(`node:${builtin}`)
 }
@@ -247,7 +258,7 @@ ${build.initialOptions.banner.js}`
       }
 
       try {
-        const packageJson = JSON.parse(fs.readFileSync(/** @type {string} */ (pathToPackageJson)).toString())
+        const packageJson = JSON.parse(fs.readFileSync(/** @type {string} */(pathToPackageJson)).toString())
 
         const isESM = isESMFile(fullPathToModule, pathToPackageJson, packageJson)
         if (isESM && !interceptedESMModules.has(fullPathToModule)) {

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', 'fork']
 
-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,19 +32,14 @@ function returnSpawnSyncError (error, context) {
   return context.result
 }
 
-for (const name of names) {
-  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) {
   const childProcessInfo = {

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

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))

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)

packages/datadog-instrumentations/src/fs.js

@@ -84,37 +84,35 @@ const paramsByFileHandleMethods = {
   writeFile: ['data', 'options'],
   writev: ['buffers', 'position'],
 }
-const names = ['fs', 'node:fs']
-for (const name of names) {
-  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'
 }