fix: Updated express instrumentation to ignore error when the next handler passes route or router (#3513)

Author: bizob2828

lib/subscribers/express/base.js

@@ -0,0 +1,26 @@
+/*
+ * Copyright 2025 New Relic Corporation. All rights reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+const MiddlewareSubscriber = require('../middleware')
+
+/**
+ * Special error handler to ignore if the error passed to next handler
+ * is 'route' or 'router', which are used by Express internally to
+ * skip out of a route or router.
+ *
+ * @param {*} err - The error passed to the next handler
+ * @returns {boolean} - returns true if error exists and not equal to a string or `route` or `router`
+ */
+function errorHandler(err) {
+  return err && err !== 'route' && err !== 'router'
+}
+
+class ExpressSubscriber extends MiddlewareSubscriber {
+  constructor({ agent, logger, packageName = 'express', channelName }) {
+    super({ agent, logger, packageName, channelName, system: 'Expressjs', errorHandler })
+  }
+}
+
+module.exports = ExpressSubscriber

lib/subscribers/express/param.js

@@ -5,11 +5,11 @@
 
 'use strict'
 
-const MiddlewareSubscriber = require('../middleware')
+const ExpressSubscriber = require('./base')
 
-class ExpressParamSubscriber extends MiddlewareSubscriber {
-  constructor({ agent, logger, packageName = 'express' }) {
-    super({ agent, logger, packageName, channelName: 'nr_param', system: 'Expressjs' })
+class ExpressParamSubscriber extends ExpressSubscriber {
+  constructor({ agent, logger, packageName }) {
+    super({ agent, logger, packageName, channelName: 'nr_param' })
   }
 
   handler(data) {

lib/subscribers/express/route.js

@@ -5,12 +5,12 @@
 
 'use strict'
 
-const MiddlewareSubscriber = require('../middleware')
 const methods = ['all', 'delete', 'get', 'head', 'post', 'put', 'patch']
+const ExpressSubscriber = require('./base')
 
-class ExpressRouteSubscriber extends MiddlewareSubscriber {
-  constructor({ agent, logger, packageName = 'express' }) {
-    super({ agent, logger, packageName, channelName: 'nr_route', system: 'Expressjs' })
+class ExpressRouteSubscriber extends ExpressSubscriber {
+  constructor({ agent, logger, packageName }) {
+    super({ agent, logger, packageName, channelName: 'nr_route' })
     this.events = ['end']
   }
 

lib/subscribers/express/use.js

@@ -5,11 +5,11 @@
 
 'use strict'
 
-const MiddlewareSubscriber = require('../middleware')
+const ExpressSubscriber = require('./base')
 
-class ExpressUseSubscriber extends MiddlewareSubscriber {
-  constructor({ agent, logger, packageName = 'express' }) {
-    super({ agent, logger, packageName, channelName: 'nr_use', system: 'Expressjs' })
+class ExpressUseSubscriber extends ExpressSubscriber {
+  constructor({ agent, logger, packageName }) {
+    super({ agent, logger, packageName, channelName: 'nr_use' })
   }
 
   handler(data) {

lib/subscribers/middleware-wrapper.js

@@ -19,13 +19,25 @@ function getFunctionName(handler) {
   return handler.name === '' ? '<anonymous>' : handler.name
 }
 
+/**
+ * Default error handler for determining if error should stored with transaction.
+ * Note: Based on previous shim instrumentation only Express, Restify, and Hapi have
+ * a different handler.
+ * @param {Error} err error passed to done handler
+ * @returns {boolean} returns true if error exists
+ */
+function defaultErrorHandler(err) {
+  return err
+}
+
 /**
  * The baseline parameters available to the middleware wrapper
  *
  * @typedef {object} WrapperParams
  * @property {object} agent A New Relic Node.js agent instance.
  * @property {object} logger An agent logger instance.
  * @property {string} system handling the instrumentation(i.e - Fastify, Expressjs, Hapi, etc)
+ * @property {Function} [errorHandler] optional function to determine if error should be recorded
  */
 
 /**
@@ -36,13 +48,14 @@ function getFunctionName(handler) {
  * @property {string} prefix formatted prefix to name segments/timeslice metrics
  */
 class MiddlewareWrapper {
-  constructor({ agent, logger, system }) {
+  constructor({ agent, logger, system, errorHandler }) {
     this.agent = agent
     this.logger = logger
     this.system = system
     this.config = agent.config
     this.prefix = `Nodejs/Middleware/${this.system}`
     this.agent.environment.setFramework(system)
+    this.isError = errorHandler ?? defaultErrorHandler
   }
 
   /**
@@ -180,7 +193,7 @@ class MiddlewareWrapper {
     if (isSync) {
       function wrappedDone(...doneArgs) {
         const [err] = doneArgs
-        if (err) {
+        if (self.isError(err)) {
           self.storeError(txInfo, err)
         // route has been completed, pop from path
         // to allow other handlers to name it more accurately

lib/subscribers/middleware.js

@@ -8,12 +8,12 @@ const Subscriber = require('./base')
 const MiddlewareWrapper = require('./middleware-wrapper')
 
 class MiddlewareSubscriber extends Subscriber {
-  constructor({ agent, logger, packageName, channelName, system }) {
+  constructor({ agent, logger, packageName, channelName, system, errorHandler }) {
     super({ agent, logger, packageName, channelName })
     // this is because the handler simply wraps a function
     // that is executed later when a request is made
     this.requireActiveTx = false
-    this.wrapper = new MiddlewareWrapper({ agent, logger, system })
+    this.wrapper = new MiddlewareWrapper({ agent, logger, system, errorHandler })
   }
 }
 

test/unit/lib/subscribers/middleare-wrapper.test.js

@@ -17,10 +17,16 @@ test.beforeEach((ctx) => {
   function handler (arg1, arg2, arg3) {
     return `${arg1}, ${arg2}, ${arg3}`
   }
+  const txInfo = {}
+  wrapper.extractTxInfo = function() {
+    const request = {}
+    return { txInfo, request, errorWare: false }
+  }
   ctx.nr = {
     agent,
     handler,
     logger,
+    txInfo,
     wrapper
   }
 })
@@ -69,3 +75,62 @@ test('should run wrapped handler in context if transaction present, and properly
     end()
   })
 })
+
+test('should handle error when passed in to done handler', function (t, end) {
+  const { agent, txInfo, wrapper } = t.nr
+  const error = new Error('test error')
+  function handler (req, res, next) {
+    next(error)
+  }
+  const route = '/test/url'
+  const wrapped = wrapper.wrap({ handler, route })
+  helper.runInTransaction(agent, function (tx) {
+    tx.type = 'web'
+    tx.url = route
+    wrapped('one', 'two', function() {})
+    assert.deepEqual(txInfo.error, error)
+    assert.equal(txInfo.errorHandled, false)
+
+    tx.end()
+    end()
+  })
+})
+
+test('should not handle error when no error is passed to done handler', function (t, end) {
+  const { agent, txInfo, wrapper } = t.nr
+  function handler (req, res, next) {
+    next()
+  }
+  const route = '/test/url'
+  const wrapped = wrapper.wrap({ handler, route })
+  helper.runInTransaction(agent, function (tx) {
+    tx.type = 'web'
+    tx.url = route
+    wrapped('one', 'two', function() {})
+    assert.deepEqual(txInfo, {})
+
+    tx.end()
+    end()
+  })
+})
+
+test('should not handle error when isError is not using default handler', function (t, end) {
+  const { agent, txInfo, wrapper } = t.nr
+  const error = new Error('test error')
+  function handler (req, res, next) {
+    next(error)
+  }
+  const route = '/test/url'
+  const wrapped = wrapper.wrap({ handler, route })
+  wrapper.isError = function(err) {
+    return err.message !== 'test error'
+  }
+  helper.runInTransaction(agent, function (tx) {
+    tx.type = 'web'
+    tx.url = route
+    wrapped('one', 'two', function() {})
+    assert.deepEqual(txInfo, {})
+    tx.end()
+    end()
+  })
+})

test/versioned/express/errors.test.js

@@ -224,6 +224,52 @@ test('Error handling tests', async (t) => {
     })
     await plan.completed
   })
+
+  await t.test('does not report error when the argument to next is `route`', function (t, end) {
+    const { app, express } = t.nr
+
+    const router1 = express.Router()
+    function bail(req, res, next) {
+      next('route')
+    }
+    router1.get('/test', bail, function (req, res, next) {
+      res.send('fail')
+    })
+
+    app.use(router1)
+    app.get('/test', function (req, res) {
+      res.send('done')
+    })
+
+    runTest(t, function (errors, statuscode) {
+      assert.equal(errors.length, 0)
+      assert.equal(statuscode, 200)
+      end()
+    })
+  })
+
+  await t.test('does not report error when the argument to next is `router`', function (t, end) {
+    const { app, express } = t.nr
+
+    const router1 = express.Router()
+    function bail(req, res, next) {
+      next('router')
+    }
+    router1.get('/test', bail, function (req, res, next) {
+      res.end('fail')
+    })
+
+    app.use(router1)
+    app.get('/test', function (req, res) {
+      res.end('done')
+    })
+
+    runTest(t, function (errors, statuscode) {
+      assert.equal(errors.length, 0)
+      assert.equal(statuscode, 200)
+      end()
+    })
+  })
 })
 
 function runTest(t, callback) {