From b3b5c7df7b53c589e3dfffff1128ebac2dc8f453 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 27 Jan 2022 21:01:31 -0800 Subject: [PATCH 1/5] fix types --- packages/nextjs/src/utils/instrumentServer.ts | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index 1b9484612528..879fbc98911c 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -34,14 +34,27 @@ interface Server { publicDir: string; } -export interface NextRequest extends http.IncomingMessage { +export type NextRequest = ( + | http.IncomingMessage // in nextjs versions < 12.0.9, `NextRequest` extends `http.IncomingMessage` + | { + _req: http.IncomingMessage; // in nextjs versions >= 12.0.9, `NextRequest` wraps `http.IncomingMessage` + } +) & { cookies: Record; url: string; query: { [key: string]: string }; headers: { [key: string]: string }; body: string | { [key: string]: unknown }; -} -type NextResponse = http.ServerResponse; + method: string; +}; + +type NextResponse = + // in nextjs versions < 12.0.9, `NextResponse` extends `http.ServerResponse` + | http.ServerResponse + // in nextjs versions >= 12.0.9, `NextResponse` wraps `http.ServerResponse` + | { + _res: http.ServerResponse; + }; // the methods we'll wrap type HandlerGetter = () => Promise; From 719c013bd444c78efc14398f0c52b0df18bbe900 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 27 Jan 2022 21:06:48 -0800 Subject: [PATCH 2/5] s/req/nextReq and s/res/nextRes --- packages/nextjs/src/utils/instrumentServer.ts | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index 879fbc98911c..8521b296d506 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -58,7 +58,7 @@ type NextResponse = // the methods we'll wrap type HandlerGetter = () => Promise; -type ReqHandler = (req: NextRequest, res: NextResponse, parsedUrl?: url.UrlWithParsedQuery) => Promise; +type ReqHandler = (nextReq: NextRequest, nextRes: NextResponse, parsedUrl?: url.UrlWithParsedQuery) => Promise; type ErrorLogger = (err: Error) => void; type ApiPageEnsurer = (path: string) => Promise; type PageComponentFinder = ( @@ -218,8 +218,8 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { // add transaction start and stop to the normal request handling const wrappedReqHandler = async function ( this: Server, - req: NextRequest, - res: NextResponse, + nextReq: NextRequest, + nextRes: NextResponse, parsedUrl?: url.UrlWithParsedQuery, ): Promise { // wrap everything in a domain in order to prevent scope bleed between requests @@ -233,23 +233,23 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { const currentScope = getCurrentHub().getScope(); if (currentScope) { - currentScope.addEventProcessor(event => parseRequest(event, req)); + currentScope.addEventProcessor(event => parseRequest(event, nextReq)); // We only want to record page and API requests - if (hasTracingEnabled() && shouldTraceRequest(req.url, publicDirFiles)) { + if (hasTracingEnabled() && shouldTraceRequest(nextReq.url, publicDirFiles)) { // If there is a trace header set, extract the data from it (parentSpanId, traceId, and sampling decision) let traceparentData; - if (req.headers && isString(req.headers['sentry-trace'])) { - traceparentData = extractTraceparentData(req.headers['sentry-trace'] as string); + if (nextReq.headers && isString(nextReq.headers['sentry-trace'])) { + traceparentData = extractTraceparentData(nextReq.headers['sentry-trace'] as string); logger.log(`[Tracing] Continuing trace ${traceparentData?.traceId}.`); } // pull off query string, if any - const reqPath = stripUrlQueryAndFragment(req.url); + const reqPath = stripUrlQueryAndFragment(nextReq.url); // requests for pages will only ever be GET requests, so don't bother to include the method in the transaction // name; requests to API routes could be GET, POST, PUT, etc, so do include it there - const namePrefix = req.url.startsWith('/api') ? `${(req.method || 'GET').toUpperCase()} ` : ''; + const namePrefix = nextReq.url.startsWith('/api') ? `${nextReq.method.toUpperCase()} ` : ''; const transaction = startTransaction( { @@ -259,7 +259,7 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { ...traceparentData, }, // extra context passed to the `tracesSampler` - { request: req }, + { request: nextReq }, ); currentScope.setSpan(transaction); @@ -283,7 +283,7 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { } } - return origReqHandler.call(this, req, res, parsedUrl); + return origReqHandler.call(this, nextReq, nextRes, parsedUrl); }); }; From ec7544c5db00215f1f002f30906840997c818d74 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 27 Jan 2022 21:07:16 -0800 Subject: [PATCH 3/5] unpack request and response if necessary --- packages/nextjs/src/utils/instrumentServer.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index 8521b296d506..d5fd351e68fd 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -222,6 +222,12 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { nextRes: NextResponse, parsedUrl?: url.UrlWithParsedQuery, ): Promise { + // Starting with version 12.0.9, nextjs wraps the incoming request in a `NodeNextRequest` object and the outgoing + // response in a `NodeNextResponse` object before passing them to the handler. (This is necessary here but not in + // `withSentry` because by the time nextjs passes them to an API handler, it's unwrapped them again.) + const req = '_req' in nextReq ? nextReq._req : nextReq; + const res = '_res' in nextRes ? nextRes._res : nextRes; + // wrap everything in a domain in order to prevent scope bleed between requests const local = domain.create(); local.add(req); From 129acd398e818eb932918b6f87c9d5e1602a2143 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 28 Jan 2022 00:24:40 -0800 Subject: [PATCH 4/5] prevent breaking changes to sampling context data --- packages/nextjs/src/utils/instrumentServer.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index d5fd351e68fd..9be2721336f7 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -264,8 +264,11 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { metadata: { requestPath: reqPath }, ...traceparentData, }, - // extra context passed to the `tracesSampler` - { request: nextReq }, + // Extra context passed to the `tracesSampler` (Note: this weird format is in order to not break people's + // `tracesSampler` functions, even though the format of `nextReq` has changed (see note above re: nextjs + // 12.0.9). If `nextReq === req` (the old format), then we'll just spread the same stuff twice. If `nextReq` + // contains `req` (the new format), then this mimics the old format by flattening the data.) + { request: { ...nextReq, ...req } }, ); currentScope.setSpan(transaction); From 5d0b4a9adcf0147bb4fe72420f2dfe327572de0f Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 28 Jan 2022 06:32:29 -0800 Subject: [PATCH 5/5] clarify comment --- packages/nextjs/src/utils/instrumentServer.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index 9be2721336f7..f85089048454 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -264,10 +264,11 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { metadata: { requestPath: reqPath }, ...traceparentData, }, - // Extra context passed to the `tracesSampler` (Note: this weird format is in order to not break people's - // `tracesSampler` functions, even though the format of `nextReq` has changed (see note above re: nextjs - // 12.0.9). If `nextReq === req` (the old format), then we'll just spread the same stuff twice. If `nextReq` - // contains `req` (the new format), then this mimics the old format by flattening the data.) + // Extra context passed to the `tracesSampler` (Note: We're combining `nextReq` and `req` this way in order + // to not break people's `tracesSampler` functions, even though the format of `nextReq` has changed (see + // note above re: nextjs 12.0.9). If `nextReq === req` (pre 12.0.9), then spreading `req` is a no-op - we're + // just spreading the same stuff twice. If `nextReq` contains `req` (12.0.9 and later), then spreading `req` + // mimics the old format by flattening the data.) { request: { ...nextReq, ...req } }, );