Skip to content

Commit

Permalink
Add render source to onRequestError context
Browse files Browse the repository at this point in the history
  • Loading branch information
huozhi committed Jul 12, 2024
1 parent 48f62e0 commit 0620f62
Show file tree
Hide file tree
Showing 4 changed files with 135 additions and 74 deletions.
61 changes: 43 additions & 18 deletions packages/next/src/server/app-render/app-render.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,7 @@ import { addImplicitTags } from '../lib/patch-fetch'
import { AppRenderSpan, NextNodeServerSpan } from '../lib/trace/constants'
import { getTracer } from '../lib/trace/tracer'
import { FlightRenderResult } from './flight-render-result'
import {
createErrorHandler,
ErrorHandlerSource,
type ErrorHandler,
} from './create-error-handler'
import { createErrorHandler, type ErrorHandler } from './create-error-handler'
import {
getShortDynamicParamType,
dynamicParamTypes,
Expand Down Expand Up @@ -727,7 +723,7 @@ async function renderToHTMLOrFlightImpl(
serverModuleMap,
})

const digestErrorsMap: Map<string, Error> = new Map()
const digestErrorsMap: Map<string, Error & { digest: string }> = new Map()
const allCapturedErrors: Error[] = []
const isNextExport = !!renderOpts.nextExport
const { staticGenerationStore, requestStore } = baseCtx
Expand Down Expand Up @@ -762,32 +758,61 @@ async function renderToHTMLOrFlightImpl(
routeType: 'render',
} as const

const onReactStreamRenderError = onInstrumentationRequestError
? (err: Error) => onInstrumentationRequestError(err, req, errorContext)
: undefined
// Including RSC rendering and flight data rendering
function getRSCError(err: Error & { digest: string }) {
const digest = err.digest
if (!digestErrorsMap.has(digest)) {
digestErrorsMap.set(digest, err)
}
return err
}

function getSSRError(err: Error & { digest: string }) {
// For SSR errors, if we have the existing digest in errors map,
// we should use the existing error object to avoid duplicate error logs.
if (digestErrorsMap.has(err.digest)) {
return digestErrorsMap.get(err.digest)!
}
return err
}

function onFlightDataRenderError(err: Error & { digest: string }) {
return onInstrumentationRequestError?.(err, req, {
...errorContext,
renderSource: 'react-server-components-payload',
})
}

function onServerRenderError(err: Error & { digest: string }) {
const renderSource = digestErrorsMap.has(err.digest)
? 'react-server-components'
: 'server-rendering'
return onInstrumentationRequestError?.(err, req, {
...errorContext,
renderSource,
})
}

const serverComponentsErrorHandler = createErrorHandler({
source: ErrorHandlerSource.serverComponents,
dev,
isNextExport,
onReactStreamRenderError,
digestErrorsMap,
// RSC rendering error will report as SSR error
onReactStreamRenderError: undefined,
getErrorByRenderSource: getRSCError,
silenceLogger: silenceStaticGenerationErrors,
})
const flightDataRendererErrorHandler = createErrorHandler({
source: ErrorHandlerSource.flightData,
dev,
isNextExport,
onReactStreamRenderError,
digestErrorsMap,
onReactStreamRenderError: onFlightDataRenderError,
getErrorByRenderSource: getRSCError,
silenceLogger: silenceStaticGenerationErrors,
})
const htmlRendererErrorHandler = createErrorHandler({
source: ErrorHandlerSource.html,
dev,
isNextExport,
onReactStreamRenderError,
digestErrorsMap,
onReactStreamRenderError: onServerRenderError,
getErrorByRenderSource: getSSRError,
allCapturedErrors,
silenceLogger: silenceStaticGenerationErrors,
})
Expand Down
39 changes: 5 additions & 34 deletions packages/next/src/server/app-render/create-error-handler.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,34 +15,23 @@ export type ErrorHandler = (
errorInfo: unknown
) => string | undefined

export const ErrorHandlerSource = {
serverComponents: 'serverComponents',
flightData: 'flightData',
html: 'html',
} as const

/**
* Create error handler for renderers.
* Tolerate dynamic server errors during prerendering so console
* isn't spammed with unactionable errors
*/
export function createErrorHandler({
/**
* Used for debugging
*/
source,
dev,
isNextExport,
onReactStreamRenderError,
digestErrorsMap,
getErrorByRenderSource,
allCapturedErrors,
silenceLogger,
}: {
source: (typeof ErrorHandlerSource)[keyof typeof ErrorHandlerSource]
dev?: boolean
isNextExport?: boolean
onReactStreamRenderError?: (err: any) => void
digestErrorsMap: Map<string, Error>
getErrorByRenderSource: (err: Error & { digest: string }) => Error
allCapturedErrors?: Error[]
silenceLogger?: boolean
}): ErrorHandler {
Expand All @@ -55,7 +44,6 @@ export function createErrorHandler({
err.message + (errorInfo?.stack || err.stack || '')
).toString()
}
const digest = err.digest

if (allCapturedErrors) allCapturedErrors.push(err)

Expand All @@ -68,13 +56,7 @@ export function createErrorHandler({
// If this is a navigation error, we don't need to log the error.
if (isNavigationSignalError(err)) return err.digest

if (!digestErrorsMap.has(digest)) {
digestErrorsMap.set(digest, err)
} else if (source === ErrorHandlerSource.html) {
// For SSR errors, if we have the existing digest in errors map,
// we should use the existing error object to avoid duplicate error logs.
err = digestErrorsMap.get(digest)
}
err = getErrorByRenderSource(err)

// If this error occurs, we know that we should be stopping the static
// render. This is only thrown in static generation when PPR is not enabled,
Expand All @@ -86,8 +68,7 @@ export function createErrorHandler({
if (dev) {
formatServerError(err)
}
// Used for debugging error source
// console.error(source, err)

// Don't log the suppressed error during export
if (
!(
Expand All @@ -107,17 +88,7 @@ export function createErrorHandler({
})
}

if (
(!silenceLogger &&
// Only log the error from SSR rendering errors and flight data render errors,
// as RSC renderer error will still be pipped into SSR renderer as well.
source === 'html') ||
source === 'flightData'
) {
// The error logger is currently not provided in the edge runtime.
// Use the exposed `__next_log_error__` instead.
// This will trace error traces to the original source code.

if (!silenceLogger) {
onReactStreamRenderError?.(err)
}
}
Expand Down
4 changes: 4 additions & 0 deletions packages/next/src/server/instrumentation/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ type RequestErrorContext = {
routerKind: 'Pages Router' | 'App Router'
routePath: string // the route file path, e.g. /app/blog/[dynamic]
routeType: 'render' | 'route' | 'action' | 'middleware'
renderSource?:
| 'react-server-components'
| 'react-server-components-payload'
| 'server-rendering'
// TODO: other future instrumentation context
}

Expand Down
105 changes: 83 additions & 22 deletions test/e2e/on-request-error/basic/basic.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,17 @@ describe('on-request-error - basic', () => {
return JSON.parse(content)
}

async function validateErrorRecord(
name: string,
url: string,
isMiddleware: boolean = false
) {
async function validateErrorRecord({
name,
url,
renderSource,
isMiddleware = false,
}: {
name: string
url: string
renderSource?: string
isMiddleware?: boolean
}) {
await retry(async () => {
const recordLogs = next.cliOutput
.split('\n')
Expand All @@ -40,21 +46,38 @@ describe('on-request-error - basic', () => {
expect(payload.message).toBe(name)
expect(count).toBe(1)

validateRequestByName(payload.request, url, isMiddleware)
validateRequestByName({
payload: payload,
url,
isMiddleware,
renderSource,
})
}

function validateRequestByName(
request: any,
url: string,
isMiddleware: boolean = false
) {
function validateRequestByName({
payload,
url,
renderSource,
isMiddleware = false,
}: {
payload: any
url: string
renderSource?: string
isMiddleware: boolean
}) {
const { request } = payload
if (isMiddleware) {
// For middleware, the URL is absolute url with host
expect(request.url).toMatch(/^http:\/\//)
expect(request.url).toMatch(url)
} else {
expect(request.url).toBe(url)
}

if (renderSource) {
expect(payload.context.renderSource).toBe(renderSource)
}

expect(request.method).toBe('GET')
expect(request.headers['accept']).toBe('*/*')
}
Expand All @@ -66,61 +89,99 @@ describe('on-request-error - basic', () => {
describe('app router', () => {
it('should catch server component page error in node runtime', async () => {
await next.fetch('/server-page')
await validateErrorRecord('server-page-node-error', '/server-page')
await validateErrorRecord({
name: 'server-page-node-error',
url: '/server-page',
renderSource: 'react-server-components',
})
})

it('should catch server component page error in edge runtime', async () => {
await next.fetch('/server-page/edge')
await validateErrorRecord('server-page-edge-error', '/server-page/edge')
await validateErrorRecord({
name: 'server-page-edge-error',
url: '/server-page/edge',
renderSource: 'react-server-components',
})
})

it('should catch client component page error in node runtime', async () => {
await next.fetch('/client-page')
await validateErrorRecord('client-page-node-error', '/client-page')
await validateErrorRecord({
name: 'client-page-node-error',
url: '/client-page',
renderSource: 'server-rendering',
})
})

it('should catch client component page error in edge runtime', async () => {
await next.fetch('/client-page/edge')
await validateErrorRecord('client-page-edge-error', '/client-page/edge')
await validateErrorRecord({
name: 'client-page-edge-error',
url: '/client-page/edge',
renderSource: 'server-rendering',
})
})

it('should catch app routes error in node runtime', async () => {
await next.fetch('/app-route')
await validateErrorRecord('route-node-error', '/app-route')
await validateErrorRecord({
name: 'route-node-error',
url: '/app-route',
})
})

it('should catch app routes error in edge runtime', async () => {
await next.fetch('/app-route/edge')
await validateErrorRecord('route-edge-error', '/app-route/edge')
await validateErrorRecord({
name: 'route-edge-error',
url: '/app-route/edge',
})
})
})

describe('pages router', () => {
it('should catch pages router page error in node runtime', async () => {
await next.fetch('/page')
await validateErrorRecord('pages-page-node-error', '/page')
await validateErrorRecord({
name: 'pages-page-node-error',
url: '/page',
})
})

it('should catch pages router page error in edge runtime', async () => {
await next.fetch('/page/edge')
await validateErrorRecord('pages-page-edge-error', '/page/edge')
await validateErrorRecord({
name: 'pages-page-edge-error',
url: '/page/edge',
})
})

it('should catch pages router api error in node runtime', async () => {
await next.fetch('/api/pages-route')
await validateErrorRecord('api-node-error', '/api/pages-route')
await validateErrorRecord({
name: 'api-node-error',
url: '/api/pages-route',
})
})

it('should catch pages router api error in edge runtime', async () => {
await next.fetch('/api/pages-route/edge')
await validateErrorRecord('api-edge-error', '/api/pages-route/edge')
await validateErrorRecord({
name: 'api-edge-error',
url: '/api/pages-route/edge',
})
})
})

describe('middleware', () => {
it('should catch middleware error', async () => {
await next.fetch('/middleware-error')
await validateErrorRecord('middleware-error', '/middleware-error', true)
await validateErrorRecord({
name: 'middleware-error',
url: '/middleware-error',
isMiddleware: true,
})
})
})
})

0 comments on commit 0620f62

Please sign in to comment.