From e1ba0fad3011f05ce40afa37edcb95334f0a2e2d Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 3 Sep 2026 13:40:52 +0200 Subject: [PATCH 1/3] fix(tracer): preserve wrapped function metadata Express classifies middleware by function arity, so tracer.wrap() turns four-argument error handlers into length-0 functions that are skipped. Preserve the wrapped function shape through the existing shimmer helper so callback, return, throw, and constructor behavior remain transparent. --- .../datadog-plugin-express/test/index.spec.js | 61 +++++++++++++++++++ packages/dd-trace/src/tracer.js | 9 +-- packages/dd-trace/test/tracer.spec.js | 51 ++++++++++++++++ 3 files changed, 117 insertions(+), 4 deletions(-) diff --git a/packages/datadog-plugin-express/test/index.spec.js b/packages/datadog-plugin-express/test/index.spec.js index dc420d413ae..4a3cca30b46 100644 --- a/packages/datadog-plugin-express/test/index.spec.js +++ b/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 diff --git a/packages/dd-trace/src/tracer.js b/packages/dd-trace/src/tracer.js index 3711cf2d2aa..919e094d59e 100644 --- a/packages/dd-trace/src/tracer.js +++ b/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) { diff --git a/packages/dd-trace/test/tracer.spec.js b/packages/dd-trace/test/tracer.spec.js index 82bca277d4a..d04007ac96f 100644 --- a/packages/dd-trace/test/tracer.spec.js +++ b/packages/dd-trace/test/tracer.spec.js @@ -448,6 +448,44 @@ 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 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 +519,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' } } From 3dd654eb9a203fa6a8d5f652e6aaf131eff20aeb Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 3 Sep 2026 16:12:34 +0200 Subject: [PATCH 2/3] fix(shimmer): preserve frozen function wrapping Frozen functions have the same non-writable prototype descriptor as native classes, so assertNotClass rejects valid tracer.wrap() inputs. Let frozen targets through and retain descriptor-copy failures only for immutable wrapper properties. --- packages/datadog-shimmer/src/shimmer.js | 25 +++++++------ packages/datadog-shimmer/test/shimmer.spec.js | 36 +++++++++++++++++++ packages/dd-trace/test/tracer.spec.js | 12 +++++++ 3 files changed, 63 insertions(+), 10 deletions(-) diff --git a/packages/datadog-shimmer/src/shimmer.js b/packages/datadog-shimmer/src/shimmer.js index d1828f51e5e..9d8c929dac8 100644 --- a/packages/datadog-shimmer/src/shimmer.js +++ b/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.') } } diff --git a/packages/datadog-shimmer/test/shimmer.spec.js b/packages/datadog-shimmer/test/shimmer.spec.js index ceed18ab689..a1e3d6069a0 100644 --- a/packages/datadog-shimmer/test/shimmer.spec.js +++ b/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 diff --git a/packages/dd-trace/test/tracer.spec.js b/packages/dd-trace/test/tracer.spec.js index d04007ac96f..6fa6080a240 100644 --- a/packages/dd-trace/test/tracer.spec.js +++ b/packages/dd-trace/test/tracer.spec.js @@ -473,6 +473,18 @@ describe('Tracer', () => { 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 From 4c3d227f197e445439eedffd38f4144869efe332 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 3 Sep 2026 21:37:53 +0200 Subject: [PATCH 3/3] refactor(tracer): load shimmer at module scope --- packages/dd-trace/src/tracer.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dd-trace/src/tracer.js b/packages/dd-trace/src/tracer.js index 919e094d59e..98de23e900d 100644 --- a/packages/dd-trace/src/tracer.js +++ b/packages/dd-trace/src/tracer.js @@ -7,6 +7,7 @@ const { flushFrameworkWarnings, flushLoadOrderWarnings, } = require('../../datadog-instrumentations/src/helpers/check-require-cache') +const shimmer = require('../../datadog-shimmer') const Tracer = require('./opentracing/tracer') const Scope = require('./scope') const { isError } = require('./util') @@ -118,7 +119,6 @@ class DatadogTracer extends Tracer { wrap (name, options, fn) { const tracer = this - const shimmer = require('../../datadog-shimmer') return shimmer.wrapFunction(fn, original => function (...args) { let optionsObj = options