fixes: 'error.transaction.*' capture; correct trans.name when error h…

…andling

- Set trans.name before a possible captureError handling a route. This
  is because `captureError(err)` will set `error.transaction.*` fields.
  We want the `trans.name` to be set by then.
- Ensure that when an *error* page component is being loaded, it isn't
  used for the transaction name.

lib/instrumentation/modules/next/dist/server/next-server.js

@@ -8,30 +8,40 @@
 
 // The main (but not only) module for instrumenting the Next.js server.
 //
-// XXX server hierarchy, mainly about the "prod" server, but mostly should
-//    work for the dev server `npm run dev`.
-//
-// Some notes on how the instrumentation works.
-//
-// - XXX NextNodeServer.handleRequest
-// - *API* routes ("pages/api/...") in a Next.js app are handled differently
-//   from other pages. The `catchAllRoute` calls `handleApiRequest`, which
-//   resolves the URL path to a possibly dynamic route name (e.g.
-//   `/api/widgets/[id]`, we instrument `ensureApiPage` to get that resolve
-//   route name), loads the webpack-compiled user module for that route, and
-//   calls `apiResolver` in "api-utils/node.ts" to execute. We instrument that
-//   `apiResolve()` function to capture any errors in the user's handler.
-// - For other routes ("pages/..."), XXX
+// Some notes on how the Next.js node server and the instrumentation works.
 //
+// - There are a number of ways to deploy (https://nextjs.org/docs/deployment)
+//   a Next.js app. This instrumentation works with "Self-Hosting", and using
+//   Next.js's built-in server (`class NextNodeServer`). This is the server
+//   that is used for `next build && next start` and a subclass of that server
+//   for `next dev`.
+// - The Next.js server is a vanilla Node.js `http.createServer` using
+//   `NextNodeServer.handleRequest` as the request handler, so every request
+//   to the server is a call to that method.
+// - Routes are defined by files under "pages/". An incoming request path is
+//   resolved a built-in Next.js route handler or one of those pages -- loaded
+//   by `NextNodeServer.findPageComponents`.
+// - An error in rendering a page results in `renderErrorToResponse(err)` being
+//   called to handle that error. (Limitation: There are some edge cases where
+//   this method is not used to handle an exception. This instrumentation isn't
+//   capturing those.)
+// - *API* routes ("pages/api/...") are handled differently from other pages.
+//   The `catchAllRoute` route handler calls `handleApiRequest`, which resolves
+//   the URL path to a possibly dynamic route name (e.g. `/api/widgets/[id]`,
+//   we instrument `ensureApiPage` to get that resolve route name), loads the
+//   webpack-compiled user module for that route, and calls `apiResolver` in
+//   "api-utils/node.ts" to execute. We instrument that `apiResolve()` function
+//   to capture any errors in the user's handler.
 // - There is open discussion here for other ways to support error capture
 //   for Next.js: https://github.com/vercel/next.js/discussions/32230
 
 const semver = require('semver')
 
 const shimmer = require('../../../../shimmer')
 
-const kRouteName = Symbol('nextJsRouteName')
-const kPossibleApiRouteName = Symbol('nextJsPossibleApiRouteName')
+const kInErrorHandling = Symbol('nextJsInErrorHandling')
+// const kRouteName = Symbol('nextJsRouteName')
+// const kPossibleApiRouteName = Symbol('nextJsPossibleApiRouteName')
 
 module.exports = function (mod, agent, { version, enabled }) {
   console.log('XXX enabled: ', enabled)
@@ -48,70 +58,73 @@ module.exports = function (mod, agent, { version, enabled }) {
   const log = agent.logger
 
   const NextNodeServer = mod.default
+  // XXX might still use generateRoutes to wrap route.fn functions to capture
+  //   the name of a route for the internal `_next`-y routes
   shimmer.wrap(NextNodeServer.prototype, 'generateRoutes', wrapGenerateRoutes)
-  shimmer.wrap(NextNodeServer.prototype, 'handleRequest', wrapHandleRequest)
-  shimmer.wrap(NextNodeServer.prototype, 'handleApiRequest', wrapHandleApiRequest)
+  // shimmer.wrap(NextNodeServer.prototype, 'handleRequest', wrapHandleRequest)
+  // shimmer.wrap(NextNodeServer.prototype, 'handleApiRequest', wrapHandleApiRequest)
   shimmer.wrap(NextNodeServer.prototype, 'ensureApiPage', wrapEnsureApiPage)
   shimmer.wrap(NextNodeServer.prototype, 'findPageComponents', wrapFindPageComponents)
   shimmer.wrap(NextNodeServer.prototype, 'renderErrorToResponse', wrapRenderErrorToResponse)
 
-  /*
-  XXX
-  - might still use generateRoutes to wrap route.fn functions to capture
-    the name of a route for the internal `_next`-y routes
-
-  */
-
   return mod
 
   function wrapGenerateRoutes (orig) {
     return function wrappedGenerateRoutes () {
       const routes = orig.apply(this, arguments)
+      console.log('XXX wrappedGenerateRoutes')
       // console.log('XXX routes: ', routes)
       return routes
     }
   }
 
-  function wrapHandleRequest (orig) {
-    return function wrappedHandleRequest (req, _res, parsedUrl) {
-      console.log('\nXXX wrappedHandleRequest(req "%s %s", res, parsedUrl=%s)', req.method, req.url, parsedUrl)
-      const promise = orig.apply(this, arguments)
-      promise.then(
-        () => {
-          const trans = ins.currTransaction()
-          if (trans && trans[kRouteName]) {
-            trans.setDefaultName(`${req.method} ${trans[kRouteName]}`)
-          }
-        }
-      )
-      return promise
-    }
-  }
+  // function wrapHandleRequest (orig) {
+  //   return function wrappedHandleRequest (req, _res, parsedUrl) {
+  //     console.log('\nXXX wrappedHandleRequest(req "%s %s", res, parsedUrl=%s)', req.method, req.url, parsedUrl)
+  //     const promise = orig.apply(this, arguments)
+  //     promise.then(
+  //       () => {
+  //         const trans = ins.currTransaction()
+  //         if (trans && trans[kRouteName]) {
+  //           trans.setDefaultName(`${req.method} ${trans[kRouteName]}`)
+  //           console.log('XXX setDefaultName: ', trans.name)
+  //         }
+  //       }
+  //     )
+  //     return promise
+  //   }
+  // }
 
-  function wrapHandleApiRequest (orig) {
-    return function wrappedHandleApiRequest () {
-      const promise = orig.apply(this, arguments)
-      promise.then(
-        handled => {
-          if (handled) {
-            // The API request was handled, therefore the route name found
-            // in the wrapped `ensureApiPage` is the route name.
-            const trans = ins.currTransaction()
-            if (trans && trans[kPossibleApiRouteName]) {
-              trans[kRouteName] = trans[kPossibleApiRouteName]
-            }
-          }
-        }
-      )
-      return promise
-    }
-  }
+  // function wrapHandleApiRequest (orig) {
+  //   return function wrappedHandleApiRequest () {
+  //     const promise = orig.apply(this, arguments)
+  //     promise.then(
+  //       handled => {
+  //         if (handled) {
+  //           console.log('XXX handled: ', handled)
+  //           // The API request was handled, therefore the route name found
+  //           // in the wrapped `ensureApiPage` is the route name.
+  //           const trans = ins.currTransaction()
+  //           if (trans && trans[kPossibleApiRouteName]) {
+  //             console.log('XXX could set trans name here')
+  //             trans[kRouteName] = trans[kPossibleApiRouteName]
+  //           }
+  //         }
+  //       }
+  //     )
+  //     return promise
+  //   }
+  // }
   function wrapEnsureApiPage (orig) {
     return function wrappedEnsureApiPage (pathname) {
       const trans = ins.currTransaction()
-      if (trans) {
-        log.trace({ pathname }, 'found possible API route name from ensureApiPage')
-        trans[kPossibleApiRouteName] = pathname
+      if (trans && trans.req) {
+        // XXX slight limitation on "handled" could *possibly* be false if
+        //     exception in `getPagePath`. Trade-off between getting that
+        //     wrong and setting the trans.name early enough for captureError
+        //     usage.
+        log.trace({ pathname }, 'set transaction name from ensureApiPage')
+        trans.setDefaultName(`${trans.req.method} ${pathname}`)
       }
       return orig.apply(this, arguments)
     }
@@ -123,22 +136,32 @@ module.exports = function (mod, agent, { version, enabled }) {
       promise.then(findComponentsResult => {
         if (findComponentsResult) {
           const trans = ins.currTransaction()
-          // Avoid overriding and already set kRouteName for the case when
-          // there is a page error and `findPageComponents()` is called
-          // independently to load the error page (e.g. "_error.js" or "500.js").
-          if (trans && !trans[kRouteName]) {
-            log.trace({ pathname }, 'found route from findPageComponents')
-            trans[kRouteName] = pathname
+          // If Next.js is doing error handling for this request, then it is
+          // loading an *error* page component (e.g. "_error.js"). We don't want
+          // to use that component's path for the transaction name.
+          if (trans && !trans[kInErrorHandling] && trans.req) {
+            log.trace({ pathname }, 'set transaction name from findPageComponents')
+            trans.setDefaultName(`${trans.req.method} ${pathname}`)
           }
         }
       })
       return promise
     }
   }
+
   function wrapRenderErrorToResponse (orig) {
     return function wrappedRenderErrorToResponse (ctx, err) {
       console.log('XXX wrappedRenderErrorToResponse(ctx, err=%s)', err)
-      agent.captureError(err)
+      const trans = ins.currTransaction()
+      if (trans) {
+        // Signal to subsequent instrumentation for this transaction that
+        // Next.js is now doing error handling for this request.
+        trans[kInErrorHandling] = true
+      }
+      // Next.js uses `err=null` to handle a 404.
+      if (err) {
+        agent.captureError(err)
+      }
       return orig.apply(this, arguments)
     }
   }

test/nextjs/a-nextjs-app/pages/api/an-api-endpoint-that-throws.js

@@ -2,6 +2,6 @@
 
 // Always executed server-side.
 export default function anApiEndpointThatThrows(req, res) {
-  throw new Error('boom')
+  throw new Error('An error thrown in anApiEndpointThatThrows handler')
 }