From 72d203045bf0c91384813c9a0943ee2e7fab8c11 Mon Sep 17 00:00:00 2001 From: William Conti Date: Tue, 12 Nov 2024 12:28:58 -0500 Subject: [PATCH 1/4] update express to not wrap router methods if version is v5 --- packages/datadog-instrumentations/src/express.js | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/packages/datadog-instrumentations/src/express.js b/packages/datadog-instrumentations/src/express.js index b093eab783..ab330072cb 100644 --- a/packages/datadog-instrumentations/src/express.js +++ b/packages/datadog-instrumentations/src/express.js @@ -57,7 +57,7 @@ function wrapResponseRender (render) { } } -addHook({ name: 'express', versions: ['>=4'] }, express => { +addHook({ name: 'express', versions: ['>=4 <5.0.0'] }, express => { shimmer.wrap(express.application, 'handle', wrapHandle) shimmer.wrap(express.Router, 'use', wrapRouterMethod) shimmer.wrap(express.Router, 'route', wrapRouterMethod) @@ -69,6 +69,16 @@ addHook({ name: 'express', versions: ['>=4'] }, express => { return express }) +addHook({ name: 'express', versions: ['>=5.0.0'] }, express => { + shimmer.wrap(express.application, 'handle', wrapHandle) + + shimmer.wrap(express.response, 'json', wrapResponseJson) + shimmer.wrap(express.response, 'jsonp', wrapResponseJson) + shimmer.wrap(express.response, 'render', wrapResponseRender) + + return express +}) + const queryParserReadCh = channel('datadog:query:read:finish') function publishQueryParsedAndNext (req, res, next) { @@ -129,7 +139,7 @@ addHook({ name: 'express', versions: ['>=4.0.0 <4.3.0'] }, express => { return express }) -addHook({ name: 'express', versions: ['>=4.3.0'] }, express => { +addHook({ name: 'express', versions: ['>=4.3.0 <5.0.0'] }, express => { shimmer.wrap(express.Router, 'process_params', wrapProcessParamsMethod(2)) return express }) From 5e8e0acafe13485b8e7ff7721140e533de92c907 Mon Sep 17 00:00:00 2001 From: William Conti Date: Wed, 13 Nov 2024 11:53:38 -0500 Subject: [PATCH 2/4] update express v5 plugin, change failing tests to only run for 3 ? 1 : 0] + wrapParams(req) const next = arguments[lastIndex] if (typeof next === 'function') { @@ -167,12 +168,37 @@ function createWrapRouterMethod (name) { return wrapMethod } +const processParamsStartCh = channel('datadog:express:process_params:start') +function wrapParams (req) { + let params + Object.defineProperty(req, 'params', { + get () { return params }, + set (val) { + params = val + if (val && processParamsStartCh.hasSubscribers) { + const abortController = new AbortController() + + processParamsStartCh.publish({ + req, + res: req?.res, + abortController, + params: req?.params + }) + } + } + }) +} + const wrapRouterMethod = createWrapRouterMethod('router') +const wrapExpressRouterMethod = createWrapRouterMethod('express') addHook({ name: 'router', versions: ['>=1'] }, Router => { shimmer.wrap(Router.prototype, 'use', wrapRouterMethod) shimmer.wrap(Router.prototype, 'route', wrapRouterMethod) + shimmer.wrap(Router.prototype, 'use', wrapExpressRouterMethod) + shimmer.wrap(Router.prototype, 'route', wrapExpressRouterMethod) + return Router }) diff --git a/packages/datadog-plugin-express/test/index.spec.js b/packages/datadog-plugin-express/test/index.spec.js index 55a608f4ad..92e378df93 100644 --- a/packages/datadog-plugin-express/test/index.spec.js +++ b/packages/datadog-plugin-express/test/index.spec.js @@ -5,10 +5,11 @@ const axios = require('axios') const { ERROR_MESSAGE, ERROR_STACK, ERROR_TYPE } = require('../../dd-trace/src/constants') const agent = require('../../dd-trace/test/plugins/agent') const plugin = require('../src') +const semver = require('semver') const sort = spans => spans.sort((a, b) => a.start.toString() >= b.start.toString() ? 1 : -1) -describe('Plugin', () => { +describe('Plugin', function () { let tracer let express let appListener @@ -197,146 +198,6 @@ describe('Plugin', () => { }) }) - it('should do automatic instrumentation on middleware', done => { - const app = express() - const router = express.Router() - - router.get('/user/:id', (req, res) => { - res.status(200).send() - }) - - app.use(function named (req, res, next) { next() }) - app.use('/app', router) - - appListener = app.listen(0, 'localhost', () => { - const port = appListener.address().port - - agent - .use(traces => { - const spans = sort(traces[0]) - - expect(spans[0]).to.have.property('resource', 'GET /app/user/:id') - expect(spans[0]).to.have.property('name', 'express.request') - expect(spans[0].meta).to.have.property('component', 'express') - expect(spans[1]).to.have.property('resource', 'query') - expect(spans[1]).to.have.property('name', 'express.middleware') - expect(spans[1].parent_id.toString()).to.equal(spans[0].span_id.toString()) - expect(spans[1].meta).to.have.property('component', 'express') - expect(spans[2]).to.have.property('resource', 'expressInit') - expect(spans[2]).to.have.property('name', 'express.middleware') - expect(spans[2].parent_id.toString()).to.equal(spans[0].span_id.toString()) - expect(spans[2].meta).to.have.property('component', 'express') - expect(spans[3]).to.have.property('resource', 'named') - expect(spans[3]).to.have.property('name', 'express.middleware') - expect(spans[3].parent_id.toString()).to.equal(spans[0].span_id.toString()) - expect(spans[3].meta).to.have.property('component', 'express') - expect(spans[4]).to.have.property('resource', 'router') - expect(spans[4]).to.have.property('name', 'express.middleware') - expect(spans[4].parent_id.toString()).to.equal(spans[0].span_id.toString()) - expect(spans[4].meta).to.have.property('component', 'express') - expect(spans[5].resource).to.match(/^bound\s.*$/) - expect(spans[5]).to.have.property('name', 'express.middleware') - expect(spans[5].parent_id.toString()).to.equal(spans[4].span_id.toString()) - expect(spans[5].meta).to.have.property('component', 'express') - expect(spans[6]).to.have.property('resource', '') - expect(spans[6]).to.have.property('name', 'express.middleware') - expect(spans[6].parent_id.toString()).to.equal(spans[5].span_id.toString()) - expect(spans[6].meta).to.have.property('component', 'express') - }) - .then(done) - .catch(done) - - axios - .get(`http://localhost:${port}/app/user/1`) - .catch(done) - }) - }) - - it('should do automatic instrumentation on middleware that break the async context', done => { - let next - - const app = express() - const interval = setInterval(() => { - if (next) { - next() - clearInterval(interval) - } - }) - - app.use(function breaking (req, res, _next) { - next = _next - }) - app.get('/user/:id', (req, res) => { - res.status(200).send() - }) - - appListener = app.listen(0, 'localhost', () => { - const port = appListener.address().port - - agent - .use(traces => { - const spans = sort(traces[0]) - - expect(spans[0]).to.have.property('resource', 'GET /user/:id') - expect(spans[0]).to.have.property('name', 'express.request') - expect(spans[0].meta).to.have.property('component', 'express') - expect(spans[3]).to.have.property('resource', 'breaking') - expect(spans[3]).to.have.property('name', 'express.middleware') - expect(spans[3].meta).to.have.property('component', 'express') - }) - .then(done) - .catch(done) - - axios - .get(`http://localhost:${port}/user/1`) - .catch(done) - }) - }) - - it('should handle errors on middleware that break the async context', done => { - let next - - const error = new Error('boom') - const app = express() - const interval = setInterval(() => { - if (next) { - next() - clearInterval(interval) - } - }) - - app.use(function breaking (req, res, _next) { - next = _next - }) - app.use(() => { throw error }) - // eslint-disable-next-line n/handle-callback-err - app.use((err, req, res, next) => next()) - app.get('/user/:id', (req, res) => { - res.status(200).send() - }) - - appListener = app.listen(0, 'localhost', () => { - const port = appListener.address().port - - agent - .use(traces => { - const spans = sort(traces[0]) - - expect(spans[0]).to.have.property('name', 'express.request') - expect(spans[4]).to.have.property('name', 'express.middleware') - expect(spans[4].meta).to.have.property(ERROR_TYPE, error.name) - expect(spans[0].meta).to.have.property('component', 'express') - expect(spans[4].meta).to.have.property('component', 'express') - }) - .then(done) - .catch(done) - - axios - .get(`http://localhost:${port}/user/1`) - .catch(done) - }) - }) - it('should surround matchers based on regular expressions', done => { const app = express() const router = express.Router() @@ -393,40 +254,6 @@ describe('Plugin', () => { }) }) - it('should only keep the last matching path of a middleware stack', done => { - const app = express() - const router = express.Router() - - router.use('/', (req, res, next) => next()) - router.use('*', (req, res, next) => next()) - router.use('/bar', (req, res, next) => next()) - router.use('/bar', (req, res, next) => { - res.status(200).send() - }) - - app.use('/', (req, res, next) => next()) - app.use('*', (req, res, next) => next()) - app.use('/foo/bar', (req, res, next) => next()) - app.use('/foo', router) - - appListener = app.listen(0, 'localhost', () => { - const port = appListener.address().port - - agent - .use(traces => { - const spans = sort(traces[0]) - - expect(spans[0]).to.have.property('resource', 'GET /foo/bar') - }) - .then(done) - .catch(done) - - axios - .get(`http://localhost:${port}/foo/bar`) - .catch(done) - }) - }) - it('should support asynchronous routers', done => { const app = express() const router = express.Router() @@ -1124,121 +951,6 @@ describe('Plugin', () => { }) }) - it('should handle middleware errors', done => { - const app = express() - const error = new Error('boom') - - app.use((req, res, next) => next(error)) - // eslint-disable-next-line n/handle-callback-err - app.use((error, req, res, next) => res.status(500).send()) - - appListener = app.listen(0, 'localhost', () => { - const port = appListener.address().port - - agent - .use(traces => { - const spans = sort(traces[0]) - - expect(spans[0]).to.have.property('error', 1) - expect(spans[0].meta).to.have.property(ERROR_TYPE, error.name) - expect(spans[0].meta).to.have.property(ERROR_MESSAGE, error.message) - expect(spans[0].meta).to.have.property(ERROR_STACK, error.stack) - expect(spans[0].meta).to.have.property('component', 'express') - expect(spans[3]).to.have.property('error', 1) - expect(spans[3].meta).to.have.property(ERROR_TYPE, error.name) - expect(spans[3].meta).to.have.property(ERROR_MESSAGE, error.message) - expect(spans[3].meta).to.have.property(ERROR_STACK, error.stack) - expect(spans[3].meta).to.have.property('component', 'express') - }) - .then(done) - .catch(done) - - axios - .get(`http://localhost:${port}/user`, { - validateStatus: status => status === 500 - }) - .catch(done) - }) - }) - - it('should handle middleware exceptions', done => { - const app = express() - const error = new Error('boom') - - app.use((req, res) => { throw error }) - // eslint-disable-next-line n/handle-callback-err - app.use((error, req, res, next) => res.status(500).send()) - - appListener = app.listen(0, 'localhost', () => { - const port = appListener.address().port - - agent - .use(traces => { - const spans = sort(traces[0]) - - expect(spans[0]).to.have.property('error', 1) - expect(spans[0].meta).to.have.property(ERROR_TYPE, error.name) - expect(spans[0].meta).to.have.property(ERROR_MESSAGE, error.message) - expect(spans[0].meta).to.have.property(ERROR_STACK, error.stack) - expect(spans[0].meta).to.have.property('component', 'express') - expect(spans[3]).to.have.property('error', 1) - expect(spans[3].meta).to.have.property(ERROR_TYPE, error.name) - expect(spans[3].meta).to.have.property(ERROR_MESSAGE, error.message) - expect(spans[3].meta).to.have.property(ERROR_STACK, error.stack) - expect(spans[0].meta).to.have.property('component', 'express') - }) - .then(done) - .catch(done) - - axios - .get(`http://localhost:${port}/user`, { - validateStatus: status => status === 500 - }) - .catch(done) - }) - }) - - it('should support capturing groups in routes', done => { - const app = express() - - app.get('/:path(*)', (req, res) => { - res.status(200).send() - }) - - appListener = app.listen(0, 'localhost', () => { - const port = appListener.address().port - - agent - .use(traces => { - const spans = sort(traces[0]) - - expect(spans[0]).to.have.property('resource', 'GET /:path(*)') - expect(spans[0].meta).to.have.property('http.url', `http://localhost:${port}/user`) - }) - .then(done) - .catch(done) - - axios - .get(`http://localhost:${port}/user`) - .catch(done) - }) - }) - - it('should keep the properties untouched on nested router handlers', () => { - const router = express.Router() - const childRouter = express.Router() - - childRouter.get('/:id', (req, res) => { - res.status(200).send() - }) - - router.use('/users', childRouter) - - const layer = router.stack.find(layer => layer.regexp.test('/users')) - - expect(layer.handle).to.have.ownProperty('stack') - }) - it('should keep user stores untouched', done => { const app = express() const storage = new AsyncLocalStorage() @@ -1372,6 +1084,298 @@ describe('Plugin', () => { }) }) }) + + if (semver.intersects(version, '<5.0.0')) { + it('should do automatic instrumentation on middleware', done => { + const app = express() + const router = express.Router() + + router.get('/user/:id', (req, res) => { + res.status(200).send() + }) + + app.use(function named (req, res, next) { next() }) + app.use('/app', router) + + appListener = app.listen(0, 'localhost', () => { + const port = appListener.address().port + + agent + .use(traces => { + const spans = sort(traces[0]) + + expect(spans[0]).to.have.property('resource', 'GET /app/user/:id') + expect(spans[0]).to.have.property('name', 'express.request') + expect(spans[0].meta).to.have.property('component', 'express') + expect(spans[1]).to.have.property('resource', 'query') + expect(spans[1]).to.have.property('name', 'express.middleware') + expect(spans[1].parent_id.toString()).to.equal(spans[0].span_id.toString()) + expect(spans[1].meta).to.have.property('component', 'express') + expect(spans[2]).to.have.property('resource', 'expressInit') + expect(spans[2]).to.have.property('name', 'express.middleware') + expect(spans[2].parent_id.toString()).to.equal(spans[0].span_id.toString()) + expect(spans[2].meta).to.have.property('component', 'express') + expect(spans[3]).to.have.property('resource', 'named') + expect(spans[3]).to.have.property('name', 'express.middleware') + expect(spans[3].parent_id.toString()).to.equal(spans[0].span_id.toString()) + expect(spans[3].meta).to.have.property('component', 'express') + expect(spans[4]).to.have.property('resource', 'router') + expect(spans[4]).to.have.property('name', 'express.middleware') + expect(spans[4].parent_id.toString()).to.equal(spans[0].span_id.toString()) + expect(spans[4].meta).to.have.property('component', 'express') + expect(spans[5].resource).to.match(/^bound\s.*$/) + expect(spans[5]).to.have.property('name', 'express.middleware') + expect(spans[5].parent_id.toString()).to.equal(spans[4].span_id.toString()) + expect(spans[5].meta).to.have.property('component', 'express') + expect(spans[6]).to.have.property('resource', '') + expect(spans[6]).to.have.property('name', 'express.middleware') + expect(spans[6].parent_id.toString()).to.equal(spans[5].span_id.toString()) + expect(spans[6].meta).to.have.property('component', 'express') + }) + .then(done) + .catch(done) + + axios + .get(`http://localhost:${port}/app/user/1`) + .catch(done) + }) + }) + + it('should do automatic instrumentation on middleware that break the async context', done => { + let next + + const app = express() + const interval = setInterval(() => { + if (next) { + next() + clearInterval(interval) + } + }) + + app.use(function breaking (req, res, _next) { + next = _next + }) + app.get('/user/:id', (req, res) => { + res.status(200).send() + }) + + appListener = app.listen(0, 'localhost', () => { + const port = appListener.address().port + + agent + .use(traces => { + const spans = sort(traces[0]) + + expect(spans[0]).to.have.property('resource', 'GET /user/:id') + expect(spans[0]).to.have.property('name', 'express.request') + expect(spans[0].meta).to.have.property('component', 'express') + expect(spans[3]).to.have.property('resource', 'breaking') + expect(spans[3]).to.have.property('name', 'express.middleware') + expect(spans[3].meta).to.have.property('component', 'express') + }) + .then(done) + .catch(done) + + axios + .get(`http://localhost:${port}/user/1`) + .catch(done) + }) + }) + + it('should handle errors on middleware that break the async context', done => { + let next + + const error = new Error('boom') + const app = express() + const interval = setInterval(() => { + if (next) { + next() + clearInterval(interval) + } + }) + + app.use(function breaking (req, res, _next) { + next = _next + }) + app.use(() => { throw error }) + // eslint-disable-next-line n/handle-callback-err + app.use((err, req, res, next) => next()) + app.get('/user/:id', (req, res) => { + res.status(200).send() + }) + + appListener = app.listen(0, 'localhost', () => { + const port = appListener.address().port + + agent + .use(traces => { + const spans = sort(traces[0]) + + expect(spans[0]).to.have.property('name', 'express.request') + expect(spans[4]).to.have.property('name', 'express.middleware') + expect(spans[4].meta).to.have.property(ERROR_TYPE, error.name) + expect(spans[0].meta).to.have.property('component', 'express') + expect(spans[4].meta).to.have.property('component', 'express') + }) + .then(done) + .catch(done) + + axios + .get(`http://localhost:${port}/user/1`) + .catch(done) + }) + }) + + // express v5 doesn't support regex paths + it('should only keep the last matching path of a middleware stack', done => { + const app = express() + const router = express.Router() + + router.use('/', (req, res, next) => next()) + router.use('*', (req, res, next) => next()) + router.use('/bar', (req, res, next) => next()) + router.use('/bar', (req, res, next) => { + res.status(200).send() + }) + + app.use('/', (req, res, next) => next()) + app.use('*', (req, res, next) => next()) + app.use('/foo/bar', (req, res, next) => next()) + app.use('/foo', router) + + appListener = app.listen(0, 'localhost', () => { + const port = appListener.address().port + + agent + .use(traces => { + const spans = sort(traces[0]) + + expect(spans[0]).to.have.property('resource', 'GET /foo/bar') + }) + .then(done) + .catch(done) + + axios + .get(`http://localhost:${port}/foo/bar`) + .catch(done) + }) + }) + + it('should handle middleware errors', done => { + const app = express() + const error = new Error('boom') + + app.use((req, res, next) => next(error)) + // eslint-disable-next-line n/handle-callback-err + app.use((error, req, res, next) => res.status(500).send()) + + appListener = app.listen(0, 'localhost', () => { + const port = appListener.address().port + + agent + .use(traces => { + const spans = sort(traces[0]) + + expect(spans[0]).to.have.property('error', 1) + expect(spans[0].meta).to.have.property(ERROR_TYPE, error.name) + expect(spans[0].meta).to.have.property(ERROR_MESSAGE, error.message) + expect(spans[0].meta).to.have.property(ERROR_STACK, error.stack) + expect(spans[0].meta).to.have.property('component', 'express') + expect(spans[3]).to.have.property('error', 1) + expect(spans[3].meta).to.have.property(ERROR_TYPE, error.name) + expect(spans[3].meta).to.have.property(ERROR_MESSAGE, error.message) + expect(spans[3].meta).to.have.property(ERROR_STACK, error.stack) + expect(spans[3].meta).to.have.property('component', 'express') + }) + .then(done) + .catch(done) + + axios + .get(`http://localhost:${port}/user`, { + validateStatus: status => status === 500 + }) + .catch(done) + }) + }) + + it('should handle middleware exceptions', done => { + const app = express() + const error = new Error('boom') + + app.use((req, res) => { throw error }) + // eslint-disable-next-line n/handle-callback-err + app.use((error, req, res, next) => res.status(500).send()) + + appListener = app.listen(0, 'localhost', () => { + const port = appListener.address().port + + agent + .use(traces => { + const spans = sort(traces[0]) + + expect(spans[0]).to.have.property('error', 1) + expect(spans[0].meta).to.have.property(ERROR_TYPE, error.name) + expect(spans[0].meta).to.have.property(ERROR_MESSAGE, error.message) + expect(spans[0].meta).to.have.property(ERROR_STACK, error.stack) + expect(spans[0].meta).to.have.property('component', 'express') + expect(spans[3]).to.have.property('error', 1) + expect(spans[3].meta).to.have.property(ERROR_TYPE, error.name) + expect(spans[3].meta).to.have.property(ERROR_MESSAGE, error.message) + expect(spans[3].meta).to.have.property(ERROR_STACK, error.stack) + expect(spans[0].meta).to.have.property('component', 'express') + }) + .then(done) + .catch(done) + + axios + .get(`http://localhost:${port}/user`, { + validateStatus: status => status === 500 + }) + .catch(done) + }) + }) + + it('should support capturing groups in routes', done => { + const app = express() + + app.get('/:path(*)', (req, res) => { + res.status(200).send() + }) + + appListener = app.listen(0, 'localhost', () => { + const port = appListener.address().port + + agent + .use(traces => { + const spans = sort(traces[0]) + + expect(spans[0]).to.have.property('resource', 'GET /:path(*)') + expect(spans[0].meta).to.have.property('http.url', `http://localhost:${port}/user`) + }) + .then(done) + .catch(done) + + axios + .get(`http://localhost:${port}/user`) + .catch(done) + }) + }) + + it('should keep the properties untouched on nested router handlers', () => { + const router = express.Router() + const childRouter = express.Router() + + childRouter.get('/:id', (req, res) => { + res.status(200).send() + }) + + router.use('/users', childRouter) + + const layer = router.stack.find(layer => layer.regexp.test('/users')) + + expect(layer.handle).to.have.ownProperty('stack') + }) + } }) describe('with configuration', () => { diff --git a/packages/datadog-plugin-express/test/integration-test/client.spec.js b/packages/datadog-plugin-express/test/integration-test/client.spec.js index a5c08d60ec..054dcb81c5 100644 --- a/packages/datadog-plugin-express/test/integration-test/client.spec.js +++ b/packages/datadog-plugin-express/test/integration-test/client.spec.js @@ -7,6 +7,7 @@ const { spawnPluginIntegrationTestProc } = require('../../../../integration-tests/helpers') const { assert } = require('chai') +const semver = require('semver') describe('esm', () => { let agent @@ -34,18 +35,36 @@ describe('esm', () => { await agent.stop() }) - it('is instrumented', async () => { - proc = await spawnPluginIntegrationTestProc(sandbox.folder, 'server.mjs', agent.port) - - return curlAndAssertMessage(agent, proc, ({ headers, payload }) => { - assert.propertyVal(headers, 'host', `127.0.0.1:${agent.port}`) - assert.isArray(payload) - assert.strictEqual(payload.length, 1) - assert.isArray(payload[0]) - assert.strictEqual(payload[0].length, 4) - assert.propertyVal(payload[0][0], 'name', 'express.request') - assert.propertyVal(payload[0][1], 'name', 'express.middleware') - }) - }).timeout(50000) + // express less than <5.0 uses their own router, which creates more middleware spans than the router + // that is used for v5+ + if (semver.intersects(version, '<5.0.0')) { + it('is instrumented', async () => { + proc = await spawnPluginIntegrationTestProc(sandbox.folder, 'server.mjs', agent.port) + + return curlAndAssertMessage(agent, proc, ({ headers, payload }) => { + assert.propertyVal(headers, 'host', `127.0.0.1:${agent.port}`) + assert.isArray(payload) + assert.strictEqual(payload.length, 1) + assert.isArray(payload[0]) + assert.strictEqual(payload[0].length, 4) + assert.propertyVal(payload[0][0], 'name', 'express.request') + assert.propertyVal(payload[0][1], 'name', 'express.middleware') + }) + }).timeout(50000) + } else { + it('is instrumented', async () => { + proc = await spawnPluginIntegrationTestProc(sandbox.folder, 'server.mjs', agent.port) + + return curlAndAssertMessage(agent, proc, ({ headers, payload }) => { + assert.propertyVal(headers, 'host', `127.0.0.1:${agent.port}`) + assert.isArray(payload) + assert.strictEqual(payload.length, 1) + assert.isArray(payload[0]) + assert.strictEqual(payload[0].length, 3) + assert.propertyVal(payload[0][0], 'name', 'express.request') + assert.propertyVal(payload[0][1], 'name', 'express.middleware') + }) + }).timeout(50000) + } }) }) From 8373c2b1d1bffaff855cadfcfd30c418a5150f85 Mon Sep 17 00:00:00 2001 From: William Conti Date: Wed, 13 Nov 2024 12:44:11 -0500 Subject: [PATCH 3/4] change tests around --- .../datadog-plugin-express/test/index.spec.js | 241 ++++++++++++++++++ 1 file changed, 241 insertions(+) diff --git a/packages/datadog-plugin-express/test/index.spec.js b/packages/datadog-plugin-express/test/index.spec.js index 92e378df93..60c421eff1 100644 --- a/packages/datadog-plugin-express/test/index.spec.js +++ b/packages/datadog-plugin-express/test/index.spec.js @@ -1375,6 +1375,247 @@ describe('Plugin', function () { expect(layer.handle).to.have.ownProperty('stack') }) + } else { + it('should do automatic instrumentation on middleware', done => { + const app = express() + const router = express.Router() + + router.get('/user/:id', (req, res) => { + res.status(200).send() + }) + + app.use(function named (req, res, next) { next() }) + app.use('/app', router) + + appListener = app.listen(0, 'localhost', () => { + const port = appListener.address().port + + agent + .use(traces => { + const spans = sort(traces[0]) + + expect(spans[0]).to.have.property('resource', 'GET /app/user/:id') + expect(spans[0]).to.have.property('name', 'express.request') + expect(spans[0].meta).to.have.property('component', 'express') + expect(spans[1]).to.have.property('resource', 'named') + expect(spans[1]).to.have.property('name', 'express.middleware') + expect(spans[1].parent_id.toString()).to.equal(spans[0].span_id.toString()) + expect(spans[1].meta).to.have.property('component', 'express') + expect(spans[2]).to.have.property('resource', 'router') + expect(spans[2]).to.have.property('name', 'express.middleware') + expect(spans[2].parent_id.toString()).to.equal(spans[0].span_id.toString()) + expect(spans[2].meta).to.have.property('component', 'express') + expect(spans[3]).to.have.property('resource', 'handle') + expect(spans[3]).to.have.property('name', 'express.middleware') + expect(spans[3].meta).to.have.property('component', 'express') + expect(spans[4]).to.have.property('name', 'express.middleware') + expect(spans[4].parent_id.toString()).to.equal(spans[3].span_id.toString()) + expect(spans[4].meta).to.have.property('component', 'express') + expect(spans[4]).to.have.property('resource', '') + }) + .then(done) + .catch(done) + + axios + .get(`http://localhost:${port}/app/user/1`) + .catch(done) + }) + }) + + it('should do automatic instrumentation on middleware that break the async context', done => { + let next + + const app = express() + const interval = setInterval(() => { + if (next) { + next() + clearInterval(interval) + } + }) + + app.use(function breaking (req, res, _next) { + next = _next + }) + app.get('/user/:id', (req, res) => { + res.status(200).send() + }) + + appListener = app.listen(0, 'localhost', () => { + const port = appListener.address().port + + agent + .use(traces => { + const spans = sort(traces[0]) + + expect(spans[0]).to.have.property('resource', 'GET /user/:id') + expect(spans[0]).to.have.property('name', 'express.request') + expect(spans[0].meta).to.have.property('component', 'express') + expect(spans[1]).to.have.property('resource', 'breaking') + expect(spans[1]).to.have.property('name', 'express.middleware') + expect(spans[1].meta).to.have.property('component', 'express') + }) + .then(done) + .catch(done) + + axios + .get(`http://localhost:${port}/user/1`) + .catch(done) + }) + }) + + it('should handle errors on middleware that break the async context', done => { + let next + + const error = new Error('boom') + const app = express() + const interval = setInterval(() => { + if (next) { + next() + clearInterval(interval) + } + }) + + app.use(function breaking (req, res, _next) { + next = _next + }) + app.use(() => { throw error }) + // eslint-disable-next-line n/handle-callback-err + app.use((err, req, res, next) => next()) + app.get('/user/:id', (req, res) => { + res.status(200).send() + }) + + appListener = app.listen(0, 'localhost', () => { + const port = appListener.address().port + + agent + .use(traces => { + const spans = sort(traces[0]) + + expect(spans[0]).to.have.property('name', 'express.request') + expect(spans[1]).to.have.property('name', 'express.middleware') + expect(spans[2]).to.have.property('name', 'express.middleware') + expect(spans[2].meta).to.have.property(ERROR_TYPE, error.name) + expect(spans[0].meta).to.have.property('component', 'express') + expect(spans[2].meta).to.have.property('component', 'express') + }) + .then(done) + .catch(done) + + axios + .get(`http://localhost:${port}/user/1`) + .catch(done) + }) + }) + + // express v5 doesn't support regex paths unless theres a name after the regex ie: *foo + it('should only keep the last matching path of a middleware stack', done => { + const app = express() + const router = express.Router() + + router.use('/', (req, res, next) => next()) + router.use('*foo', (req, res, next) => next()) + router.use('/bar', (req, res, next) => next()) + router.use('/bar', (req, res, next) => { + res.status(200).send() + }) + + app.use('/', (req, res, next) => next()) + app.use('*foo', (req, res, next) => next()) + app.use('/foo/bar', (req, res, next) => next()) + app.use('/foo', router) + + appListener = app.listen(0, 'localhost', () => { + const port = appListener.address().port + + agent + .use(traces => { + const spans = sort(traces[0]) + + expect(spans[0]).to.have.property('resource', 'GET /foo/bar') + }) + .then(done) + .catch(done) + + axios + .get(`http://localhost:${port}/foo/bar`) + .catch(done) + }) + }) + + it('should handle middleware errors', done => { + const app = express() + const error = new Error('boom') + + app.use((req, res, next) => next(error)) + // eslint-disable-next-line n/handle-callback-err + app.use((error, req, res, next) => res.status(500).send()) + + appListener = app.listen(0, 'localhost', () => { + const port = appListener.address().port + + agent + .use(traces => { + const spans = sort(traces[0]) + + expect(spans[0]).to.have.property('error', 1) + expect(spans[0].meta).to.have.property(ERROR_TYPE, error.name) + expect(spans[0].meta).to.have.property(ERROR_MESSAGE, error.message) + expect(spans[0].meta).to.have.property(ERROR_STACK, error.stack) + expect(spans[0].meta).to.have.property('component', 'express') + expect(spans[1]).to.have.property('error', 1) + expect(spans[1].meta).to.have.property(ERROR_TYPE, error.name) + expect(spans[1].meta).to.have.property(ERROR_MESSAGE, error.message) + expect(spans[1].meta).to.have.property(ERROR_STACK, error.stack) + expect(spans[1].meta).to.have.property('component', 'express') + }) + .then(done) + .catch(done) + + axios + .get(`http://localhost:${port}/user`, { + validateStatus: status => status === 500 + }) + .catch(done) + }) + }) + + it('should handle middleware exceptions', done => { + const app = express() + const error = new Error('boom') + + app.use((req, res) => { throw error }) + // eslint-disable-next-line n/handle-callback-err + app.use((error, req, res, next) => res.status(500).send()) + + appListener = app.listen(0, 'localhost', () => { + const port = appListener.address().port + + agent + .use(traces => { + const spans = sort(traces[0]) + + expect(spans[0]).to.have.property('error', 1) + expect(spans[0].meta).to.have.property(ERROR_TYPE, error.name) + expect(spans[0].meta).to.have.property(ERROR_MESSAGE, error.message) + expect(spans[0].meta).to.have.property(ERROR_STACK, error.stack) + expect(spans[0].meta).to.have.property('component', 'express') + expect(spans[1]).to.have.property('error', 1) + expect(spans[1].meta).to.have.property(ERROR_TYPE, error.name) + expect(spans[1].meta).to.have.property(ERROR_MESSAGE, error.message) + expect(spans[1].meta).to.have.property(ERROR_STACK, error.stack) + expect(spans[0].meta).to.have.property('component', 'express') + }) + .then(done) + .catch(done) + + axios + .get(`http://localhost:${port}/user`, { + validateStatus: status => status === 500 + }) + .catch(done) + }) + }) } }) From ab8dfcdf6b8365014b47054246e54cc2b951a7ce Mon Sep 17 00:00:00 2001 From: William Conti Date: Wed, 13 Nov 2024 12:48:39 -0500 Subject: [PATCH 4/4] dont wrap params for now --- packages/datadog-instrumentations/src/router.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/datadog-instrumentations/src/router.js b/packages/datadog-instrumentations/src/router.js index 92a850f558..c1134059c5 100644 --- a/packages/datadog-instrumentations/src/router.js +++ b/packages/datadog-instrumentations/src/router.js @@ -25,7 +25,7 @@ function createWrapRouterMethod (name) { const lastIndex = arguments.length - 1 const name = original._name || original.name const req = arguments[arguments.length > 3 ? 1 : 0] - wrapParams(req) + // wrapParams(req) const next = arguments[lastIndex] if (typeof next === 'function') {