fix(tracer): preserve wrapped function metadata

packages/datadog-plugin-express/test/index.spec.js

@@ -336,6 +336,67 @@ describe('Plugin', () => {
           })
         })
 
+        it('should dispatch tracer-wrapped request middleware', async () => {
+          const app = express()
+
+          function requestMiddleware (request, response, next) {
+            response.locals.path = request.path
+            next()
+          }
+
+          const middleware = tracer.wrap('request.middleware', requestMiddleware)
+
+          app.use(middleware)
+          app.get('/wrapped', (request, response) => {
+            response.status(200).send(response.locals.path)
+          })
+
+          appListener = app.listen(0, 'localhost')
+          await once(appListener, 'listening')
+
+          const port = appListener.address().port
+          const tracePromise = agent.assertSomeTraces(traces => {
+            assert.ok(traces[0].some(span => span.name === 'request.middleware'))
+          })
+          const responsePromise = axios.get(`http://localhost:${port}/wrapped`)
+          const [, response] = await Promise.all([tracePromise, responsePromise])
+
+          assert.strictEqual(middleware.length, 3)
+          assert.strictEqual(response.status, 200)
+          assert.strictEqual(response.data, '/wrapped')
+        })
+
+        it('should dispatch tracer-wrapped error middleware', async () => {
+          const app = express()
+
+          app.use(() => { throw new Error('boom') })
+
+          function errorMiddleware (error, request, response, next) {
+            next()
+            response.status(418).send(`${error.message}:${request.path}`)
+          }
+
+          const middleware = tracer.wrap('error.middleware', errorMiddleware)
+          app.use(middleware)
+          app.use((_request, _response, _next) => {})
+
+          appListener = app.listen(0, 'localhost')
+          await once(appListener, 'listening')
+
+          const port = appListener.address().port
+          const tracePromise = agent.assertSomeTraces(traces => {
+            assert.ok(traces[0].some(span => span.name === 'error.middleware'))
+          })
+          const responsePromise = axios.get(`http://localhost:${port}/wrapped`, {
+            validateStatus: status => status === 418,
+          })
+          const [, response] = await Promise.all([tracePromise, responsePromise])
+
+          assert.strictEqual(middleware.length, 4)
+          assert.strictEqual(response.status, 418)
+          assert.strictEqual(response.data, 'boom:/wrapped')
+        })
+
         it('should do automatic instrumentation on middleware that break the async context', done => {
           let next
 

packages/datadog-shimmer/src/shimmer.js

@@ -53,8 +53,15 @@ function copyProperties (original, wrapped) {
       const descriptor = /** @type {Descriptor} */ (Object.getOwnPropertyDescriptor(original, key))
       if (descriptor.writable && descriptor.enumerable && descriptor.configurable) {
         wrapped[key] = original[key]
-      } else if (descriptor.writable || descriptor.configurable || !Object.hasOwn(wrapped, key)) {
-        Object.defineProperty(wrapped, key, descriptor)
+      } else {
+        try {
+          Object.defineProperty(wrapped, key, descriptor)
+        } catch (error) {
+          const wrappedDescriptor = Object.getOwnPropertyDescriptor(wrapped, key)
+          if (wrappedDescriptor?.configurable !== false || wrappedDescriptor.writable) {
+            throw error
+          }
+        }
       }
     }
   }
@@ -320,19 +327,17 @@ function assertMethod (target, name, method) {
 }
 
 /**
- * Asserts that a target is not a class constructor.
+ * Asserts that a target is not an identifiable class constructor.
  *
  * @param {Function} target - The target function.
- * @throws {Error} If the target is a class constructor.
+ * @throws {Error} If the target is a non-frozen class constructor.
  */
 function assertNotClass (target) {
-  // Class constructors have a non-writable `prototype` property; functions have a
-  // writable one and arrows / async / method-shorthand have none at all. The
-  // `'prototype' in target` gate skips the descriptor lookup for the no-prototype
-  // shapes; the `in` operator is cheaper than reading `target.prototype` since
-  // it returns a boolean instead of materialising the prototype reference.
+  // Class constructors have a non-writable `prototype` property, but frozen
+  // functions do as well. Frozen targets are accepted when the descriptors are ambiguous.
   if ('prototype' in target &&
-      Object.getOwnPropertyDescriptor(target, 'prototype').writable === false) {
+      Object.getOwnPropertyDescriptor(target, 'prototype').writable === false &&
+      !Object.isFrozen(target)) {
     throw new TypeError('Target is a native class constructor and cannot be wrapped.')
   }
 }

packages/datadog-shimmer/test/shimmer.spec.js

@@ -490,6 +490,42 @@ describe('shimmer', () => {
       assert.strictEqual(wrapped(1), 2)
     })
 
+    it('should wrap the frozen function', () => {
+      const count = Object.freeze(function count (inc) { return inc })
+
+      const wrapped = shimmer.wrapFunction(count, count => inc => count(inc) + 1)
+
+      assert.strictEqual(wrapped(1), 2)
+      assert.strictEqual(wrapped.name, count.name)
+      assert.strictEqual(wrapped.length, count.length)
+      assert.strictEqual(wrapped.prototype, count.prototype)
+    })
+
+    it('should keep an existing non-configurable wrapper property', () => {
+      const count = () => {}
+      const wrapped = () => {}
+
+      Object.defineProperty(count, 'property', { value: 'original' })
+      Object.defineProperty(wrapped, 'property', { value: 'wrapped' })
+
+      assert.strictEqual(shimmer.wrapFunction(count, () => wrapped).property, 'wrapped')
+    })
+
+    it('should rethrow unexpected property copy errors', () => {
+      const error = new Error('boom')
+      const count = () => {}
+      const wrapped = new Proxy(() => {}, {
+        defineProperty (target, property, descriptor) {
+          if (property === 'property') throw error
+          return Reflect.defineProperty(target, property, descriptor)
+        },
+      })
+
+      Object.defineProperty(count, 'property', { value: 'original' })
+
+      assert.throws(() => shimmer.wrapFunction(count, () => wrapped), value => value === error)
+    })
+
     it('should wrap the constructor', () => {
       const Counter = function (start) {
         this.value = start

packages/dd-trace/src/tracer.js

@@ -118,8 +118,9 @@ class DatadogTracer extends Tracer {
 
   wrap (name, options, fn) {
     const tracer = this
+    const shimmer = require('../../datadog-shimmer')
 
-    return function (...args) {
+    return shimmer.wrapFunction(fn, original => function (...args) {
       let optionsObj = options
       if (typeof optionsObj === 'function' && typeof fn === 'function') {
         optionsObj = optionsObj.apply(this, args)
@@ -136,11 +137,11 @@ class DatadogTracer extends Tracer {
             return scopeBoundCb.apply(this, arguments)
           }
 
-          return fn.apply(this, args)
+          return original.apply(this, args)
         })
       }
-      return tracer.trace(name, optionsObj, () => fn.apply(this, args))
-    }
+      return tracer.trace(name, optionsObj, () => original.apply(this, args))
+    })
   }
 
   setUrl (url) {

packages/dd-trace/test/tracer.spec.js

@@ -448,6 +448,56 @@ describe('Tracer', () => {
       assert.strictEqual(result, 'test')
     })
 
+    it('should preserve the original function shape', () => {
+      const property = Symbol('property')
+      const descriptor = {
+        configurable: false,
+        enumerable: false,
+        value: 'value',
+        writable: false,
+      }
+
+      function callback (error, request, response, next) {
+        return [error, request, response, next]
+      }
+
+      callback.custom = 'custom'
+      Object.defineProperty(callback, property, descriptor)
+
+      const fn = tracer.wrap('name', {}, callback)
+
+      assert.strictEqual(fn.length, callback.length)
+      assert.strictEqual(fn.name, callback.name)
+      assert.strictEqual(fn.prototype, callback.prototype)
+      assert.strictEqual(fn.custom, callback.custom)
+      assert.deepStrictEqual(Object.getOwnPropertyDescriptor(fn, property), descriptor)
+    })
+
+    it('should wrap a frozen function', () => {
+      const Callback = Object.freeze(function Callback (value) { return value })
+
+      const WrappedCallback = tracer.wrap('name', {}, Callback)
+
+      assert.strictEqual(WrappedCallback('value'), 'value')
+      assert.strictEqual(WrappedCallback.length, Callback.length)
+      assert.strictEqual(WrappedCallback.name, Callback.name)
+      assert.strictEqual(WrappedCallback.prototype, Callback.prototype)
+      assert.ok(new WrappedCallback() instanceof Callback)
+    })
+
+    it('should preserve constructor behavior', () => {
+      function Value (value) {
+        this.value = value
+      }
+
+      const WrappedValue = tracer.wrap('name', {}, Value)
+      const value = new WrappedValue('value')
+
+      assert.ok(value instanceof Value)
+      assert.ok(value instanceof WrappedValue)
+      assert.strictEqual(value.value, 'value')
+    })
+
     it('should wait for the callback to be called before finishing the span', done => {
       const fn = tracer.wrap('name', {}, sinon.spy(function (cb) {
         const span = tracer.scope().active()
@@ -481,6 +531,19 @@ describe('Tracer', () => {
         .then(() => done())
     })
 
+    it('should preserve promise return values', async () => {
+      const fn = tracer.wrap('name', {}, () => Promise.resolve('test'))
+
+      assert.strictEqual(await fn(), 'test')
+    })
+
+    it('should preserve thrown errors', () => {
+      const error = new Error('boom')
+      const fn = tracer.wrap('name', {}, () => { throw error })
+
+      assert.throws(() => fn(), value => value === error)
+    })
+
     it('should accept an options object', () => {
       const options = { tags: { sometag: 'somevalue' } }