[DI] Do not fail if a custom logger is configured (#6127)

* [DI] Do not fail if a custom logger is configured

Do not try to serialize a custom logger if provided when sending the
config object to the DI worker thread. Instead, only serialize the
config options that are needed.

This change doesn't address the issue of the custom logger not being
used by the worker thread, only that it shouldn't fail to start it. That
will be addressed later.

Author: watson

integration-tests/debugger/custom-logger.spec.js

@@ -0,0 +1,18 @@
+'use strict'
+
+const assert = require('node:assert')
+const { setup } = require('./utils')
+
+describe('Dynamic Instrumentation', function () {
+  const t = setup()
+
+  it('should not abort if a custom logger is used', function (done) {
+    t.agent.on('debugger-input', ({ payload: [payload] }) => {
+      assert.strictEqual(payload.message, 'Hello World!')
+      done()
+    })
+
+    t.agent.addRemoteConfig(t.rcConfig)
+    t.triggerBreakpoint()
+  })
+})

integration-tests/debugger/target-app/custom-logger.js

@@ -0,0 +1,23 @@
+'use strict'
+
+require('dd-trace').init({
+  logger: {
+    error: err => console.error(err), // eslint-disable-line no-console
+    warn: message => console.warn(message), // eslint-disable-line no-console
+    info: message => console.info(message), // eslint-disable-line no-console
+    debug: message => console.debug(message) // eslint-disable-line no-console
+  }
+})
+
+const http = require('http')
+
+const server = http.createServer((req, res) => {
+  res.end('hello world') // BREAKPOINT: /
+  setImmediate(() => {
+    server.close()
+  })
+})
+
+server.listen(process.env.APP_PORT || 0, () => {
+  process.send?.({ port: server.address().port })
+})

packages/dd-trace/src/ci-visibility/dynamic-instrumentation/index.js

@@ -5,6 +5,7 @@ const { Worker, threadId: parentThreadId } = require('worker_threads')
 const { randomUUID } = require('crypto')
 const log = require('../../log')
 const { getEnvironmentVariables } = require('../../config-helper')
+const getDebuggerConfig = require('../../debugger/config')
 
 const probeIdToResolveBreakpointSet = new Map()
 const probeIdToResolveBreakpointRemove = new Map()
@@ -82,7 +83,7 @@ class TestVisDynamicInstrumentation {
           DD_INSTRUMENTATION_TELEMETRY_ENABLED: 'false'
         },
         workerData: {
-          config: this._config.serialize(),
+          config: getDebuggerConfig(this._config),
           parentThreadId,
           probePort: probeChannel.port1,
           configPort: configChannel.port1,

packages/dd-trace/src/config.js

@@ -1546,22 +1546,6 @@ class Config {
       }
     }
   }
-
-  // TODO: Refactor the Config class so it never produces any config objects that are incompatible with MessageChannel
-  /**
-   * Serializes the config object so it can be passed over a Worker Thread MessageChannel.
-   * @returns {Object} The serialized config object.
-   */
-  serialize () {
-    // URL objects cannot be serialized over the MessageChannel, so we need to convert them to strings first
-    if (this.url instanceof URL) {
-      const config = { ...this }
-      config.url = this.url.toString()
-      return config
-    }
-
-    return this
-  }
 }
 
 function handleOtel (tagString) {

packages/dd-trace/src/debugger/config.js

@@ -0,0 +1,14 @@
+'use strict'
+
+module.exports = function getDebuggerConfig (config) {
+  return {
+    commitSHA: config.commitSHA,
+    dynamicInstrumentation: config.dynamicInstrumentation,
+    hostname: config.hostname,
+    port: config.port,
+    repositoryUrl: config.repositoryUrl,
+    runtimeId: config.tags['runtime-id'],
+    service: config.service,
+    url: config.url?.toString(),
+  }
+}

packages/dd-trace/src/debugger/devtools_client/config.js

@@ -5,11 +5,7 @@ const { format } = require('node:url')
 const log = require('../../log')
 
 const config = module.exports = {
-  dynamicInstrumentation: parentConfig.dynamicInstrumentation,
-  runtimeId: parentConfig.tags['runtime-id'],
-  service: parentConfig.service,
-  commitSHA: parentConfig.commitSHA,
-  repositoryUrl: parentConfig.repositoryUrl,
+  ...parentConfig,
   parentThreadId,
   maxTotalPayloadSize: 5 * 1024 * 1024 // 5MB
 }

packages/dd-trace/src/debugger/index.js

@@ -4,6 +4,7 @@ const { readFile } = require('fs')
 const { types } = require('util')
 const { join } = require('path')
 const { Worker, MessageChannel, threadId: parentThreadId } = require('worker_threads')
+const getDebuggerConfig = require('./config')
 const log = require('../log')
 
 let worker = null
@@ -60,7 +61,7 @@ function start (config, rc) {
       execArgv: [], // Avoid worker thread inheriting the `-r` command line argument
       env, // Avoid worker thread inheriting the `NODE_OPTIONS` environment variable (in case it contains `-r`)
       workerData: {
-        config: config.serialize(),
+        config: getDebuggerConfig(config),
         parentThreadId,
         probePort: probeChannel.port1,
         configPort: configChannel.port1
@@ -100,7 +101,7 @@ function start (config, rc) {
 
 function configure (config) {
   if (configChannel === null) return
-  configChannel.port2.postMessage(config.serialize())
+  configChannel.port2.postMessage(getDebuggerConfig(config))
 }
 
 function readProbeFile (path, cb) {

packages/dd-trace/test/debugger/config.spec.js

@@ -0,0 +1,43 @@
+'use strict'
+
+require('../setup/mocha')
+
+const assert = require('node:assert')
+const getDebuggerConfig = require('../../src/debugger/config')
+const Config = require('../../src/config')
+
+describe('getDebuggerConfig', function () {
+  it('should only contain the allowed properties', function () {
+    const tracerConfig = new Config({
+      url: new URL('http://example.com:1234')
+    })
+    const config = getDebuggerConfig(tracerConfig)
+    assert.deepStrictEqual(Object.keys(config), [
+      'commitSHA',
+      'dynamicInstrumentation',
+      'hostname',
+      'port',
+      'repositoryUrl',
+      'runtimeId',
+      'service',
+      'url',
+    ])
+    assert.strictEqual(config.commitSHA, tracerConfig.commitSHA)
+    assert.deepStrictEqual(config.dynamicInstrumentation, tracerConfig.dynamicInstrumentation)
+    assert.strictEqual(config.hostname, tracerConfig.hostname)
+    assert.strictEqual(config.port, tracerConfig.port)
+    assert.strictEqual(config.repositoryUrl, tracerConfig.repositoryUrl)
+    assert.strictEqual(config.runtimeId, tracerConfig.tags['runtime-id'])
+    assert.strictEqual(config.service, tracerConfig.service)
+    assert.strictEqual(config.url, tracerConfig.url.toString())
+  })
+
+  it('should be able to send the config over a MessageChannel', function () {
+    const config = getDebuggerConfig(new Config())
+    const channel = new MessageChannel()
+    channel.port1.on('message', (message) => {
+      assert.deepStrictEqual(message, config)
+    })
+    channel.port2.postMessage(config)
+  })
+})