From a230be4867deee8d2041397707c6921c79e27687 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Thu, 18 Apr 2024 12:51:34 -0700 Subject: [PATCH] Clean-up fetch metrics tracking (#64746) This ensures we only track fetch metrics in development mode as that's the only time we report them currently, this also adds an upper limit on how many metrics we store per-request as previously this was unbounded, on top of that this ensures we don't keep tracking fetch metrics after the request has ended as we only report on request end, and finally this adds a clean-up to the reference we attach to the request object containing the fetch metrics once we have used them. Closes: https://github.com/vercel/next.js/issues/64212 Closes NEXT-3159 --- ...tatic-generation-async-storage.external.ts | 2 ++ .../next/src/server/app-render/app-render.tsx | 23 +++++++++++----- ...static-generation-async-storage-wrapper.ts | 4 ++- packages/next/src/server/lib/patch-fetch.ts | 27 ++++++++++++++++++- packages/next/src/server/next-server.ts | 2 ++ 5 files changed, 50 insertions(+), 8 deletions(-) diff --git a/packages/next/src/client/components/static-generation-async-storage.external.ts b/packages/next/src/client/components/static-generation-async-storage.external.ts index 2a21c77357dfe..62a7b2c04490e 100644 --- a/packages/next/src/client/components/static-generation-async-storage.external.ts +++ b/packages/next/src/client/components/static-generation-async-storage.external.ts @@ -51,6 +51,8 @@ export interface StaticGenerationStore { isDraftMode?: boolean isUnstableNoStore?: boolean + + requestEndedState?: { ended?: boolean } } export type StaticGenerationAsyncStorage = diff --git a/packages/next/src/server/app-render/app-render.tsx b/packages/next/src/server/app-render/app-render.tsx index e6598a5a189a8..b46a5ec3881c5 100644 --- a/packages/next/src/server/app-render/app-render.tsx +++ b/packages/next/src/server/app-render/app-render.tsx @@ -585,7 +585,8 @@ async function renderToHTMLOrFlightImpl( pagePath: string, query: NextParsedUrlQuery, renderOpts: RenderOpts, - baseCtx: AppRenderBaseContext + baseCtx: AppRenderBaseContext, + requestEndedState: { ended?: boolean } ) { const isNotFoundPath = pagePath === '/404' @@ -621,6 +622,7 @@ async function renderToHTMLOrFlightImpl( if (typeof req.on === 'function') { req.on('end', () => { + requestEndedState.ended = true if ('performance' in globalThis) { const metrics = getClientComponentLoaderMetrics({ reset: true }) if (metrics) { @@ -1438,14 +1440,23 @@ export const renderToHTMLOrFlight: AppPageRender = ( { urlPathname: pathname, renderOpts, + requestEndedState: { ended: false }, }, (staticGenerationStore) => - renderToHTMLOrFlightImpl(req, res, pagePath, query, renderOpts, { - requestStore, - staticGenerationStore, - componentMod: renderOpts.ComponentMod, + renderToHTMLOrFlightImpl( + req, + res, + pagePath, + query, renderOpts, - }) + { + requestStore, + staticGenerationStore, + componentMod: renderOpts.ComponentMod, + renderOpts, + }, + staticGenerationStore.requestEndedState || {} + ) ) ) } diff --git a/packages/next/src/server/async-storage/static-generation-async-storage-wrapper.ts b/packages/next/src/server/async-storage/static-generation-async-storage-wrapper.ts index 2f11e7cf85097..bbe9d42e1bb86 100644 --- a/packages/next/src/server/async-storage/static-generation-async-storage-wrapper.ts +++ b/packages/next/src/server/async-storage/static-generation-async-storage-wrapper.ts @@ -9,6 +9,7 @@ import type { FetchMetric } from '../base-http' export type StaticGenerationContext = { urlPathname: string + requestEndedState?: { ended?: boolean } renderOpts: { incrementalCache?: IncrementalCache isOnDemandRevalidate?: boolean @@ -50,7 +51,7 @@ export const StaticGenerationAsyncStorageWrapper: AsyncStorageWrapper< > = { wrap( storage: AsyncLocalStorage, - { urlPathname, renderOpts }: StaticGenerationContext, + { urlPathname, renderOpts, requestEndedState }: StaticGenerationContext, callback: (store: StaticGenerationStore) => Result ): Result { /** @@ -96,6 +97,7 @@ export const StaticGenerationAsyncStorageWrapper: AsyncStorageWrapper< isDraftMode: renderOpts.isDraftMode, prerenderState, + requestEndedState, } // TODO: remove this when we resolve accessing the store outside the execution context diff --git a/packages/next/src/server/lib/patch-fetch.ts b/packages/next/src/server/lib/patch-fetch.ts index ca61c1b534d02..5e1beaf79a8d8 100644 --- a/packages/next/src/server/lib/patch-fetch.ts +++ b/packages/next/src/server/lib/patch-fetch.ts @@ -163,7 +163,13 @@ function trackFetchMetric( staticGenerationStore: StaticGenerationStore, ctx: Omit ) { - if (!staticGenerationStore) return + if ( + !staticGenerationStore || + staticGenerationStore.requestEndedState?.ended || + process.env.NODE_ENV !== 'development' + ) { + return + } staticGenerationStore.fetchMetrics ??= [] const dedupeFields = ['url', 'status', 'method'] as const @@ -181,6 +187,25 @@ function trackFetchMetric( end: Date.now(), idx: staticGenerationStore.nextFetchId || 0, }) + + // only store top 10 metrics to avoid storing too many + if (staticGenerationStore.fetchMetrics.length > 10) { + // sort slowest first as these should be highlighted + staticGenerationStore.fetchMetrics.sort((a, b) => { + const aDur = a.end - a.start + const bDur = b.end - b.start + + if (aDur < bDur) { + return 1 + } else if (aDur > bDur) { + return -1 + } + return 0 + }) + // now grab top 10 + staticGenerationStore.fetchMetrics = + staticGenerationStore.fetchMetrics.slice(0, 10) + } } interface PatchableModule { diff --git a/packages/next/src/server/next-server.ts b/packages/next/src/server/next-server.ts index 66ac2be127388..96f1669efdea8 100644 --- a/packages/next/src/server/next-server.ts +++ b/packages/next/src/server/next-server.ts @@ -1127,6 +1127,7 @@ export default class NextNodeServer extends BaseServer { if (this.renderOpts.dev) { const { blue, green, yellow, red, gray, white } = require('../lib/picocolors') as typeof import('../lib/picocolors') + const _res = res as NodeNextResponse | ServerResponse const origRes = 'originalResponse' in _res ? _res.originalResponse : _res @@ -1247,6 +1248,7 @@ export default class NextNodeServer extends BaseServer { } } } + delete normalizedReq.fetchMetrics origRes.off('close', reqCallback) } origRes.on('close', reqCallback)