vercel · huozhi · Jul 17, 2024 · Jul 17, 2024 · Jul 17, 2024 · Jul 17, 2024
diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts
@@ -836,10 +836,7 @@ export default abstract class Server<
   ) {
     const [err, req, ctx] = args
 
-    if (
-      process.env.__NEXT_EXPERIMENTAL_INSTRUMENTATION &&
-      this.instrumentation
-    ) {
+    if (this.instrumentation) {
       try {
         await this.instrumentation.onRequestError?.(
           err,

diff --git a/packages/next/src/server/web/globals.ts b/packages/next/src/server/web/globals.ts
@@ -37,12 +37,7 @@ export async function edgeInstrumentationOnRequestError(
 ) {
   const instrumentation = await getEdgeInstrumentationModule()
   try {
-    if (
-      process.env.__NEXT_EXPERIMENTAL_INSTRUMENTATION &&
-      instrumentation?.onRequestError
-    ) {
-      await instrumentation.onRequestError(...args)
-    }
+    await instrumentation?.onRequestError?.(...args)
   } catch (err) {
     // Log the soft error and continue, since the original error has already been thrown
     console.error('Error in instrumentation.onRequestError:', err)

diff --git a/test/e2e/on-request-error/_testing/utils.js b/test/e2e/on-request-error/_testing/utils.js
@@ -0,0 +1,7 @@
+export async function getOutputLogJson(next, outputLogPath) {
+  if (!(await next.hasFile(outputLogPath))) {
+    return {}
+  }
+  const content = await next.readFile(outputLogPath)
+  return JSON.parse(content)
+}
diff --git a/test/e2e/on-request-error/basic/basic.test.ts b/test/e2e/on-request-error/basic/basic.test.ts
@@ -1,13 +1,11 @@
 import { nextTestSetup } from 'e2e-utils'
 import { retry } from 'next-test-utils'
+import { getOutputLogJson } from '../_testing/utils'
 
 describe('on-request-error - basic', () => {
   const { next, skipped } = nextTestSetup({
     files: __dirname,
     skipDeployment: true,
-    env: {
-      __NEXT_EXPERIMENTAL_INSTRUMENTATION: '1',
-    },
   })
 
   if (skipped) {
@@ -16,14 +14,6 @@ describe('on-request-error - basic', () => {
 
   const outputLogPath = 'output-log.json'
 
-  async function getOutputLogJson() {
-    if (!(await next.hasFile(outputLogPath))) {
-      return {}
-    }
-    const content = await next.readFile(outputLogPath)
-    return JSON.parse(content)
-  }
-
   async function validateErrorRecord({
     errorMessage,
     url,
@@ -37,14 +27,15 @@ describe('on-request-error - basic', () => {
   }) {
     // Assert the instrumentation is called
     await retry(async () => {
-      const recordLogs = next.cliOutput
+      const recordLogLines = next.cliOutput
         .split('\n')
         .filter((log) => log.includes('[instrumentation] write-log'))
-      const expectedLog = recordLogs.find((log) => log.includes(errorMessage))
-      expect(expectedLog).toBeDefined()
+      expect(recordLogLines).toEqual(
+        expect.arrayContaining([expect.stringContaining(errorMessage)])
+      )
     }, 5000)
 
-    const json = await getOutputLogJson()
+    const json = await getOutputLogJson(next, outputLogPath)
     const record = json[errorMessage]
 
     const { payload } = record

diff --git a/test/e2e/on-request-error/dynamic-routes/app/app-page/dynamic/[id]/page.js b/test/e2e/on-request-error/dynamic-routes/app/app-page/dynamic/[id]/page.js
@@ -0,0 +1,5 @@
+export default function Page() {
+  throw new Error('server-dynamic-page-node-error')
+}
+
+export const dynamic = 'force-dynamic'
diff --git a/test/e2e/on-request-error/dynamic-routes/app/app-page/suspense/page.js b/test/e2e/on-request-error/dynamic-routes/app/app-page/suspense/page.js
@@ -0,0 +1,18 @@
+import { Suspense } from 'react'
+
+export default function Page() {
+  return (
+    <Suspense>
+      <Inner />
+    </Suspense>
+  )
+}
+
+function Inner() {
+  if (typeof window === 'undefined') {
+    throw new Error('server-suspense-page-node-error')
+  }
+  return 'inner'
+}
+
+export const dynamic = 'force-dynamic'
diff --git a/test/e2e/on-request-error/dynamic-routes/app/app-route/dynamic/[id]/route.js b/test/e2e/on-request-error/dynamic-routes/app/app-route/dynamic/[id]/route.js
@@ -0,0 +1,5 @@
+export function GET() {
+  throw new Error('server-dynamic-route-node-error')
+}
+
+export const dynamic = 'force-dynamic'
diff --git a/test/e2e/on-request-error/dynamic-routes/app/layout.js b/test/e2e/on-request-error/dynamic-routes/app/layout.js
@@ -0,0 +1,12 @@
+export const metadata = {
+  title: 'Next.js',
+  description: 'Generated by Next.js',
+}
+
+export default function RootLayout({ children }) {
+  return (
+    <html lang="en">
+      <body>{children}</body>
+    </html>
+  )
+}
diff --git a/test/e2e/on-request-error/dynamic-routes/app/write-log/route.js b/test/e2e/on-request-error/dynamic-routes/app/write-log/route.js
@@ -0,0 +1,40 @@
+import fs from 'fs'
+import fsp from 'fs/promises'
+import path from 'path'
+
+const dir = path.join(path.dirname(new URL(import.meta.url).pathname), '../..')
+const logPath = path.join(dir, 'output-log.json')
+
+export async function POST(req) {
+  let payloadString = ''
+  const decoder = new TextDecoder()
+  const reader = req.clone().body.getReader()
+  while (true) {
+    const { done, value } = await reader.read()
+    if (done) {
+      break
+    }
+
+    payloadString += decoder.decode(value)
+  }
+
+  const payload = JSON.parse(payloadString)
+
+  const json = fs.existsSync(logPath)
+    ? JSON.parse(await fsp.readFile(logPath, 'utf8'))
+    : {}
+
+  if (!json[payload.message]) {
+    json[payload.message] = {
+      payload,
+      count: 1,
+    }
+  } else {
+    json[payload.message].count++
+  }
+
+  await fsp.writeFile(logPath, JSON.stringify(json, null, 2), 'utf8')
+
+  console.log(`[instrumentation] write-log:${payload.message}`)
+  return new Response(null, { status: 204 })
+}
diff --git a/test/e2e/on-request-error/dynamic-routes/dynamic-routes.test.ts b/test/e2e/on-request-error/dynamic-routes/dynamic-routes.test.ts
@@ -0,0 +1,134 @@
+import { nextTestSetup } from 'e2e-utils'
+import { retry } from 'next-test-utils'
+import { getOutputLogJson } from '../_testing/utils'
+
+describe('on-request-error - dynamic-routes', () => {
+  const { next, skipped } = nextTestSetup({
+    files: __dirname,
+    skipDeployment: true,
+  })
+
+  if (skipped) {
+    return
+  }
+
+  const outputLogPath = 'output-log.json'
+
+  async function getErrorRecord({ errorMessage }: { errorMessage: string }) {
+    // Assert the instrumentation is called
+    await retry(async () => {
+      const recordLogLines = next.cliOutput
+        .split('\n')
+        .filter((log) => log.includes('[instrumentation] write-log'))
+      expect(recordLogLines).toEqual(
+        expect.arrayContaining([expect.stringContaining(errorMessage)])
+      )
+    }, 5000)
+
+    const json = await getOutputLogJson(next, outputLogPath)
+    const record = json[errorMessage]
+
+    return record
+  }
+
+  beforeAll(async () => {
+    await next.patchFile(outputLogPath, '{}')
+  })
+
+  describe('app router', () => {
+    it('should catch app router dynamic page error with search params', async () => {
+      await next.fetch('/app-page/dynamic/123?apple=dope')
+      const record = await getErrorRecord({
+        errorMessage: 'server-dynamic-page-node-error',
+      })
+      expect(record).toMatchObject({
+        payload: {
+          message: 'server-dynamic-page-node-error',
+          request: {
+            url: '/app-page/dynamic/123?apple=dope',
+          },
+          context: {
+            routePath: '/app-page/dynamic/[id]',
+          },
+        },
+      })
+    })
+
+    it('should catch app router dynamic routes error with search params', async () => {
+      await next.fetch('/app-route/dynamic/123?apple=dope')
+      const record = await getErrorRecord({
+        errorMessage: 'server-dynamic-route-node-error',
+      })
+      expect(record).toMatchObject({
+        payload: {
+          message: 'server-dynamic-route-node-error',
+          request: {
+            url: '/app-route/dynamic/123?apple=dope',
+          },
+          context: {
+            routePath: '/app-route/dynamic/[id]',
+          },
+        },
+      })
+    })
+
+    it('should catch suspense rendering page error in node runtime', async () => {
+      await next.fetch('/app-page/suspense')
+      const record = await getErrorRecord({
+        errorMessage: 'server-suspense-page-node-error',
+      })
+
+      expect(record).toMatchObject({
+        payload: {
+          message: 'server-suspense-page-node-error',
+          request: {
+            url: '/app-page/suspense',
+          },
+          context: {
+            routePath: '/app-page/suspense',
+          },
+        },
+      })
+    })
+  })
+
+  describe('pages router', () => {
+    it('should catch pages router dynamic page error with search params', async () => {
+      await next.fetch('/pages-page/dynamic/123?apple=dope')
+      const record = await getErrorRecord({
+        errorMessage: 'pages-page-node-error',
+      })
+
+      expect(record).toMatchObject({
+        payload: {
+          message: 'pages-page-node-error',
+          request: {
+            url: '/pages-page/dynamic/123?apple=dope',
+          },
+          context: {
+            routePath: '/pages-page/dynamic/[id]',
+          },
+        },
+      })
+    })
+
+    it('should catch pages router dynamic API route error with search params', async () => {
+      await next.fetch('/api/dynamic/123?apple=dope')
+      const record = await getErrorRecord({
+        errorMessage: 'pages-api-node-error',
+      })
+
+      expect(record).toMatchObject({
+        payload: {
+          message: 'pages-api-node-error',
+          request: {
+            url: '/api/dynamic/123?apple=dope',
+          },
+          context: {
+            routePath: '/api/dynamic/[id]',
+          },
+        },
+      })
+    })
+  })
+})
diff --git a/test/e2e/on-request-error/dynamic-routes/instrumentation.js b/test/e2e/on-request-error/dynamic-routes/instrumentation.js
@@ -0,0 +1,13 @@
+export function onRequestError(err, request, context) {
+  fetch(`http://localhost:${process.env.PORT}/write-log`, {
+    method: 'POST',
+    body: JSON.stringify({
+      message: err.message,
+      request,
+      context,
+    }),
+    headers: {
+      'Content-Type': 'application/json',
+    },
+  })
+}
diff --git a/test/e2e/on-request-error/dynamic-routes/next.config.js b/test/e2e/on-request-error/dynamic-routes/next.config.js
@@ -0,0 +1,5 @@
+module.exports = {
+  experimental: {
+    instrumentationHook: true,
+  },
+}
diff --git a/test/e2e/on-request-error/dynamic-routes/pages/api/dynamic/[id].js b/test/e2e/on-request-error/dynamic-routes/pages/api/dynamic/[id].js
@@ -0,0 +1,3 @@
+export default function handler() {
+  throw new Error('pages-api-node-error')
+}
diff --git a/test/e2e/on-request-error/dynamic-routes/pages/pages-page/dynamic/[id].js b/test/e2e/on-request-error/dynamic-routes/pages/pages-page/dynamic/[id].js
@@ -0,0 +1,9 @@
+export default function Page() {
+  throw new Error('pages-page-node-error')
+}
+
+export function getServerSideProps() {
+  return {
+    props: {},
+  }
+}
diff --git a/test/e2e/on-request-error/server-action-error/server-action-error.test.ts b/test/e2e/on-request-error/server-action-error/server-action-error.test.ts
@@ -1,13 +1,11 @@
 import { nextTestSetup } from 'e2e-utils'
 import { retry } from 'next-test-utils'
+import { getOutputLogJson } from '../_testing/utils'
 
 describe('on-request-error - server-action-error', () => {
   const { next, skipped } = nextTestSetup({
     files: __dirname,
     skipDeployment: true,
-    env: {
-      __NEXT_EXPERIMENTAL_INSTRUMENTATION: '1',
-    },
   })
 
   if (skipped) {
@@ -16,14 +14,6 @@ describe('on-request-error - server-action-error', () => {
 
   const outputLogPath = 'output-log.json'
 
-  async function getOutputLogJson() {
-    if (!(await next.hasFile(outputLogPath))) {
-      return {}
-    }
-    const content = await next.readFile(outputLogPath)
-    return JSON.parse(content)
-  }
-
   async function validateErrorRecord({
     errorMessage,
     url,
@@ -42,7 +32,7 @@ describe('on-request-error - server-action-error', () => {
       )
     }, 5000)
 
-    const json = await getOutputLogJson()
+    const json = await getOutputLogJson(next, outputLogPath)
     const record = json[errorMessage]
 
     // Assert error is recorded in the output log

diff --git a/test/e2e/on-request-error/skip-next-internal-error/skip-next-internal-error.test.ts b/test/e2e/on-request-error/skip-next-internal-error/skip-next-internal-error.test.ts
@@ -4,9 +4,6 @@ describe('on-request-error - skip-next-internal-error', () => {
   const { next, skipped } = nextTestSetup({
     files: __dirname,
     skipDeployment: true,
-    env: {
-      __NEXT_EXPERIMENTAL_INSTRUMENTATION: '1',
-    },
   })
 
   if (skipped) {