Skip to content

Commit

Permalink
feat: Added instrumentation support for Express 5 beta (#2476)
Browse files Browse the repository at this point in the history
This will be experimental until express@5.0.0 is generally available
bizob2828 authored Aug 16, 2024
1 parent c2b8879 commit 06a4c2f
Showing 18 changed files with 1,425 additions and 1,520 deletions.
74 changes: 9 additions & 65 deletions lib/instrumentation/express.js
Original file line number Diff line number Diff line change
@@ -6,7 +6,6 @@
'use strict'

const { MiddlewareSpec, MiddlewareMounterSpec, RenderSpec } = require('../../lib/shim/specs')
const { MIDDLEWARE_TYPE_NAMES } = require('../../lib/shim/webframework-shim/common')

/**
* Express middleware generates traces where middleware are considered siblings
@@ -25,35 +24,30 @@ module.exports = function initialize(agent, express, moduleName, shim) {
return err !== 'route' && err !== 'router'
})

if (express.Router.use) {
wrapExpress4(shim, express)
} else {
wrapExpress3(shim, express)
}
}

function wrapExpress4(shim, express) {
// Wrap `use` and `route` which are hung off `Router` directly, not on a
// prototype.
shim.wrapMiddlewareMounter(
express.Router,
express.application,
'use',
new MiddlewareMounterSpec({
route: shim.FIRST,
wrapper: wrapMiddleware
})
)

wrapExpressRouter(shim, express.Router.use ? express.Router : express.Router.prototype)
wrapResponse(shim, express.response)
}

function wrapExpressRouter(shim, router) {
shim.wrapMiddlewareMounter(
express.application,
router,
'use',
new MiddlewareMounterSpec({
route: shim.FIRST,
wrapper: wrapMiddleware
})
)

shim.wrap(express.Router, 'route', function wrapRoute(shim, fn) {
shim.wrap(router, 'route', function wrapRoute(shim, fn) {
if (!shim.isFunction(fn)) {
return fn
}
@@ -89,7 +83,7 @@ function wrapExpress4(shim, express) {
})

shim.wrapMiddlewareMounter(
express.Router,
router,
'param',
new MiddlewareMounterSpec({
route: shim.FIRST,
@@ -105,56 +99,6 @@ function wrapExpress4(shim, express) {
}
})
)

wrapResponse(shim, express.response)
}

function wrapExpress3(shim, express) {
// In Express 3 the app returned from `express()` is actually a `connect` app
// which we have no access to before creation. We can not easily wrap the app
// because there are a lot of methods dangling on it that act on the app itself.
// Really we just care about apps being used as `request` event listeners on
// `http.Server` instances so we'll wrap that instead.

shim.wrapMiddlewareMounter(
express.Router.prototype,
'param',
new MiddlewareMounterSpec({
route: shim.FIRST,
wrapper: function wrapParamware(shim, middleware, fnName, route) {
return shim.recordParamware(
middleware,
new MiddlewareSpec({
name: route,
req: shim.FIRST,
next: shim.THIRD,
type: MIDDLEWARE_TYPE_NAMES.PARAMWARE
})
)
}
})
)
shim.wrapMiddlewareMounter(
express.Router.prototype,
'use',
new MiddlewareMounterSpec({
route: shim.FIRST,
wrapper: wrapMiddleware
})
)
shim.wrapMiddlewareMounter(
express.application,
'use',
new MiddlewareMounterSpec({
route: shim.FIRST,
wrapper: wrapMiddleware
})
)

// NOTE: Do not wrap application route methods in Express 3, they all just
// forward their arguments to the router.
wrapRouteMethods(shim, express.Router.prototype, shim.FIRST)
wrapResponse(shim, express.response)
}

function wrapRouteMethods(shim, route, path) {
2 changes: 1 addition & 1 deletion test/lib/metrics_helper.js
Original file line number Diff line number Diff line change
@@ -48,7 +48,7 @@ function assertMetrics(metrics, expected, exclusive, assertValues) {
for (let i = 0, len = expected.length; i < len; i++) {
const expectedMetric = expected[i]
const metric = metrics.getMetric(expectedMetric[0].name, expectedMetric[0].scope)
this.ok(metric)
this.ok(metric, `should find ${expectedMetric[0].name}`)
if (assertValues) {
this.same(metric.toJSON(), expectedMetric[1])
}
19 changes: 16 additions & 3 deletions test/versioned/express-esm/package.json
Original file line number Diff line number Diff line change
@@ -10,9 +10,22 @@
"node": ">=18"
},
"dependencies": {
"express": ">=4.6.0",
"express-enrouten": "1.1",
"ejs": "2.5.9"
"express": {
"versions": ">=4.6.0",
"samples": 5
}
},
"files": [
"segments.tap.mjs",
"transaction-naming.tap.mjs"
]
},
{
"engines": {
"node": ">=18"
},
"dependencies": {
"express": "5.0.0-beta.3"
},
"files": [
"segments.tap.mjs",
206 changes: 64 additions & 142 deletions test/versioned/express-esm/segments.tap.mjs

Large diffs are not rendered by default.

25 changes: 15 additions & 10 deletions test/versioned/express-esm/transaction-naming.tap.mjs
Original file line number Diff line number Diff line change
@@ -20,6 +20,8 @@ const { setup, makeRequest, makeRequestAndFinishTransaction } = expressHelpers
// const pkgVersion = expressPkg.version
import { readFileSync } from 'node:fs'
const { version: pkgVersion } = JSON.parse(readFileSync('./node_modules/express/package.json'))
// TODO: change to 5.0.0 when officially released
const isExpress5 = semver.gte(pkgVersion, '5.0.0-beta.3')

test('transaction naming tests', (t) => {
t.autoend()
@@ -51,8 +53,8 @@ test('transaction naming tests', (t) => {
})

const endpoint = '/asdf'

await runTest({ app, t, endpoint, expectedName: '(not found)' })
const txPrefix = isExpress5 ? 'WebTransaction/Nodejs' : 'WebTransaction/Expressjs'
await runTest({ app, t, endpoint, txPrefix, expectedName: '(not found)' })
})

t.test('transaction name with route that has multiple handlers', async function (t) {
@@ -248,12 +250,13 @@ test('transaction naming tests', (t) => {

t.test('when using a string pattern in path', async function (t) {
const { app } = await setup()
const path = isExpress5 ? /ab?cd/ : '/ab?cd'

app.get('/ab?cd', function (req, res) {
app.get(path, function (req, res) {
res.end()
})

await runTest({ app, t, endpoint: '/abcd', expectedName: '/ab?cd' })
await runTest({ app, t, endpoint: '/abcd', expectedName: path })
})

t.test('when using a regular expression in path', async function (t) {
@@ -609,12 +612,14 @@ test('transaction naming tests', (t) => {
return { promise, transactionHandler }
}

async function runTest({ app, t, endpoint, expectedName = endpoint }) {
async function runTest({
app,
t,
endpoint,
expectedName = endpoint,
txPrefix = 'WebTransaction/Expressjs'
}) {
const transaction = await makeRequestAndFinishTransaction({ t, app, agent, endpoint })
t.equal(
transaction.name,
'WebTransaction/Expressjs/GET/' + expectedName,
'transaction has expected name'
)
t.equal(transaction.name, `${txPrefix}/GET/${expectedName}`, 'transaction has expected name')
}
})
5 changes: 4 additions & 1 deletion test/versioned/express/app-use.tap.js
Original file line number Diff line number Diff line change
@@ -8,8 +8,11 @@
const test = require('tap').test
const helper = require('../../lib/agent_helper')
const http = require('http')
const { isExpress5 } = require('./utils')

test('app should be at top of stack when mounted', function (t) {
// This test is no longer applicable in express 5 as mounting a child router does not emit the same
// mount event
test('app should be at top of stack when mounted', { skip: isExpress5 }, function (t) {
const agent = helper.instrumentMockedAgent()
const express = require('express')

86 changes: 86 additions & 0 deletions test/versioned/express/async-handlers.tap.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/*
* Copyright 2024 New Relic Corporation. All rights reserved.
* SPDX-License-Identifier: Apache-2.0
*/

'use strict'

const { makeRequest, setup } = require('./utils')
const { test } = require('tap')

test('should properly track async handlers', (t) => {
setup(t)
const { app } = t.context
const mwTimeout = 20
const handlerTimeout = 25

app.use(async function (req, res, next) {
await new Promise((resolve) => {
setTimeout(() => {
resolve()
}, mwTimeout)
})
next()
})
app.use('/test', async function handler(req, res) {
await new Promise((resolve) => {
setTimeout(resolve, handlerTimeout)
})
res.send('ok')
})

runTest(t, '/test', (tx) => {
const [children] = tx.trace.root.children
const [mw, handler] = children.children
t.ok(
Math.ceil(mw.getDurationInMillis()) >= mwTimeout,
`should be at least ${mwTimeout} for middleware segment`
)
t.ok(
Math.ceil(handler.getDurationInMillis()) >= handlerTimeout,
`should be at least ${handlerTimeout} for handler segment`
)
t.end()
})
})

test('should properly handle errors in async handlers', (t) => {
setup(t)
const { app } = t.context

app.use(() => {
return Promise.reject(new Error('whoops i failed'))
})
app.use('/test', function handler(req, res) {
t.fail('should not call handler on error')
res.send('ok')
})
// eslint-disable-next-line no-unused-vars
app.use(function (error, req, res, next) {
res.status(400).end()
})

runTest(t, '/test', (tx) => {
const errors = tx.agent.errors.traceAggregator.errors
t.equal(errors.length, 1)
const [error] = errors
t.equal(error[2], 'HttpError 400', 'should return 400 from custom error handler')
t.end()
})
})

function runTest(t, endpoint, callback) {
const { agent, app } = t.context

agent.on('transactionFinished', callback)

const server = app.listen(function () {
makeRequest(this, endpoint, function (response) {
response.resume()
})
})

t.teardown(() => {
server.close()
})
}
2 changes: 1 addition & 1 deletion test/versioned/express/bare-router.tap.js
Original file line number Diff line number Diff line change
@@ -53,7 +53,7 @@ test('Express router introspection', function (t) {
const url = 'http://localhost:' + port + '/test'
helper.makeGetRequest(url, { json: true }, function (error, res, body) {
t.equal(res.statusCode, 200, 'nothing exploded')
t.deepEqual(body, { status: 'ok' }, 'got expected response')
t.same(body, { status: 'ok' }, 'got expected response')
})
})
})
4 changes: 1 addition & 3 deletions test/versioned/express/client-disconnect.tap.js
Original file line number Diff line number Diff line change
@@ -51,14 +51,12 @@ tap.test('Client Premature Disconnection', (t) => {
[
'WebTransaction/Expressjs/POST//test',
[
'Nodejs/Middleware/Expressjs/query',
'Nodejs/Middleware/Expressjs/expressInit',
'Nodejs/Middleware/Expressjs/jsonParser',
'Expressjs/Route Path: /test',
['Nodejs/Middleware/Expressjs/controller', ['timers.setTimeout']]
]
],
{ exact: true }
{ exact: false }
)

t.equal(agent.getTransaction(), null, 'should have ended the transaction')
396 changes: 189 additions & 207 deletions test/versioned/express/errors.tap.js

Large diffs are not rendered by default.

16 changes: 3 additions & 13 deletions test/versioned/express/middleware-name.tap.js
Original file line number Diff line number Diff line change
@@ -16,19 +16,9 @@ test('should name middleware correctly', function (t) {
app.use('/', testMiddleware)

const server = app.listen(0, function () {
t.equal(app._router.stack.length, 3, '3 middleware functions: query parser, Express, router')

let count = 0
for (let i = 0; i < app._router.stack.length; i++) {
const layer = app._router.stack[i]

// route middleware doesn't have a name, sentinel is our error handler,
// neither should be wrapped.
if (layer.handle.name && layer.handle.name === 'testMiddleware') {
count++
}
}
t.equal(count, 1, 'should find only one testMiddleware function')
const router = app._router || app.router
const mwLayer = router.stack.filter((layer) => layer.name === 'testMiddleware')
t.equal(mwLayer.length, 1, 'should only find one testMiddleware function')
t.end()
})

3 changes: 1 addition & 2 deletions test/versioned/express/newrelic.js
Original file line number Diff line number Diff line change
@@ -9,8 +9,7 @@ exports.config = {
app_name: ['My Application'],
license_key: 'license key here',
logging: {
level: 'trace',
filepath: '../../../newrelic_agent.log'
level: 'trace'
},
utilization: {
detect_aws: false,
43 changes: 39 additions & 4 deletions test/versioned/express/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
{
"name": "express-tests",
"targets": [{"name":"express","minAgentVersion":"2.6.0"}],
"targets": [
{
"name": "express",
"minAgentVersion": "2.6.0"
}
],
"version": "0.0.0",
"private": true,
"tests": [
@@ -9,7 +14,10 @@
"node": ">=18"
},
"dependencies": {
"express": ">=4.6.0",
"express": {
"versions": ">=4.6.0",
"samples": 5
},
"express-enrouten": "1.1",
"ejs": "2.5.9"
},
@@ -32,7 +40,34 @@
"segments.tap.js",
"transaction-naming.tap.js"
]
},
{
"engines": {
"node": ">=18"
},
"dependencies": {
"express": "5.0.0-beta.3",
"ejs": "2.5.9"
},
"files": [
"app-use.tap.js",
"async-handlers.tap.js",
"async-error.tap.js",
"bare-router.tap.js",
"captures-params.tap.js",
"client-disconnect.tap.js",
"errors.tap.js",
"ignoring.tap.js",
"issue171.tap.js",
"middleware-name.tap.js",
"render.tap.js",
"require.tap.js",
"route-iteration.tap.js",
"route-param.tap.js",
"router-params.tap.js",
"segments.tap.js",
"transaction-naming.tap.js"
]
}
],
"dependencies": {}
]
}
895 changes: 440 additions & 455 deletions test/versioned/express/render.tap.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion test/versioned/express/route-iteration.tap.js
Original file line number Diff line number Diff line change
@@ -35,7 +35,7 @@ test('new relic should not break route iteration', function (t) {
router.use(childA)
router.use(childB)

t.deepEqual(findAllRoutes(router, ''), ['/get', ['/test'], ['/hello']])
t.same(findAllRoutes(router, ''), ['/get', ['/test'], ['/hello']])
})

function findAllRoutes(router, path) {
259 changes: 85 additions & 174 deletions test/versioned/express/segments.tap.js

Large diffs are not rendered by default.

871 changes: 433 additions & 438 deletions test/versioned/express/transaction-naming.tap.js

Large diffs are not rendered by default.

37 changes: 37 additions & 0 deletions test/versioned/express/utils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* Copyright 2024 New Relic Corporation. All rights reserved.
* SPDX-License-Identifier: Apache-2.0
*/

'use strict'
const http = require('http')
const helper = require('../../lib/agent_helper')
const semver = require('semver')

function isExpress5() {
const { version } = require('express/package')
// TODO: change to 5.0.0 when officially released
return semver.gte(version, '5.0.0-beta.3')
}

function makeRequest(server, path, callback) {
const port = server.address().port
http.request({ port: port, path: path }, callback).end()
}

function setup(t, config = {}) {
t.context.agent = helper.instrumentMockedAgent(config)
t.context.isExpress5 = isExpress5()

t.context.express = require('express')
t.context.app = t.context.express()
t.teardown(() => {
helper.unloadAgent(t.context.agent)
})
}

module.exports = {
isExpress5,
makeRequest,
setup
}

0 comments on commit 06a4c2f

Please sign in to comment.