From 6e709638184b0dbf37a54eb34f2c2b4f94b0c07c Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 20 May 2021 15:36:26 -0700 Subject: [PATCH 1/7] add request to transaction metadata type --- packages/types/src/transaction.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index f937c28bedc7..2b942af0763f 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -132,4 +132,10 @@ export interface TransactionMetadata { sentry?: string; thirdparty?: string; }; + + /** For transactions tracing server-side request handling, the request being tracked. */ + request?: { + [key: string]: any; + url: string; + }; } From eba181f367d0f0f45135da668b102c8d1dd1d155 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 20 May 2021 00:33:48 -0700 Subject: [PATCH 2/7] allow request.query_string to be an object --- packages/types/src/request.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/types/src/request.ts b/packages/types/src/request.ts index 1995919d11f9..e0b491b9a9b4 100644 --- a/packages/types/src/request.ts +++ b/packages/types/src/request.ts @@ -13,7 +13,7 @@ export interface Request { url?: string; method?: string; data?: any; - query_string?: string; + query_string?: string | { [key: string]: string }; cookies?: { [key: string]: string }; env?: { [key: string]: string }; headers?: { [key: string]: string }; From 45dccf073191dd1353f74e42442b5130cfcb29ad Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 20 May 2021 00:33:35 -0700 Subject: [PATCH 3/7] wrap methods to get parameterized names --- packages/nextjs/src/utils/instrumentServer.ts | 104 +++++++++++++----- 1 file changed, 78 insertions(+), 26 deletions(-) diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index ce097740e4b3..0950b3436814 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -1,10 +1,11 @@ import { deepReadDirSync } from '@sentry/node'; -import { hasTracingEnabled } from '@sentry/tracing'; import { Transaction } from '@sentry/types'; +import { getActiveTransaction, hasTracingEnabled } from '@sentry/tracing'; import { fill, logger } from '@sentry/utils'; import * as domain from 'domain'; import * as http from 'http'; import { default as createNextServer } from 'next'; +import * as querystring from 'querystring'; import * as url from 'url'; import * as Sentry from '../index.server'; @@ -40,11 +41,19 @@ interface NextResponse extends http.ServerResponse { type HandlerGetter = () => Promise; type ReqHandler = (req: NextRequest, res: NextResponse, parsedUrl?: url.UrlWithParsedQuery) => Promise; type ErrorLogger = (err: Error) => void; +type ApiPageEnsurer = (path: string) => Promise; +type PageComponentFinder = ( + pathname: string, + query: querystring.ParsedUrlQuery, + params: { [key: string]: any } | null, +) => Promise<{ [key: string]: any } | null>; // these aliases are purely to make the function signatures more easily understandable type WrappedHandlerGetter = HandlerGetter; type WrappedErrorLogger = ErrorLogger; type WrappedReqHandler = ReqHandler; +type WrappedApiPageEnsurer = ApiPageEnsurer; +type WrappedPageComponentFinder = PageComponentFinder; // TODO is it necessary for this to be an object? const closure: PlainObject = {}; @@ -125,6 +134,12 @@ function makeWrappedHandlerGetter(origHandlerGetter: HandlerGetter): WrappedHand // to the appropriate handlers) fill(serverPrototype, 'handleRequest', makeWrappedReqHandler); + // Wrap as a way to grab the parameterized request URL to use as the transaction name for API requests and page + // requests, respectively. These methods are chosen because they're the first spot in the request-handling process + // where the parameterized path is provided as an argument, so it's easy to grab. + fill(serverPrototype, 'ensureApiPage', makeWrappedMethodForGettingParameterizedPath); + fill(serverPrototype, 'findPageComponents', makeWrappedMethodForGettingParameterizedPath); + sdkSetupComplete = true; } @@ -182,40 +197,77 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { // local.on('error', Sentry.captureException); local.run(() => { - // We only want to record page and API requests - if (hasTracingEnabled() && shouldTraceRequest(req.url, publicDirFiles)) { - const transaction = Sentry.startTransaction({ - name: `${(req.method || 'GET').toUpperCase()} ${req.url}`, - op: 'http.server', - }); - Sentry.getCurrentHub() - .getScope() - ?.setSpan(transaction); - - res.__sentry__ = { transaction }; - - res.once('finish', () => { - const transaction = res.__sentry__?.transaction; - if (transaction) { - // Push `transaction.finish` to the next event loop so open spans have a chance to finish before the transaction - // closes - setImmediate(() => { - // TODO - // addExpressReqToTransaction(transaction, req); + const currentScope = Sentry.getCurrentHub().getScope(); + + if (currentScope) { + // We only want to record page and API requests + if (hasTracingEnabled() && shouldTraceRequest(req.url, publicDirFiles)) { + // pull off query string, if any + const reqPath = req.url.split('?')[0]; + + // 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 transaction = Sentry.startTransaction({ + name: `${namePrefix}${reqPath}`, + op: 'http.server', + metadata: { request: req }, + }); + + currentScope.setSpan(transaction); + + res.once('finish', () => { + const transaction = getActiveTransaction(); + if (transaction) { transaction.setHttpStatus(res.statusCode); - transaction.finish(); - }); - } - }); - return origReqHandler.call(this, req, res, parsedUrl); + delete transaction.metadata.request; + + // Push `transaction.finish` to the next event loop so open spans have a chance to finish before the + // transaction closes + setImmediate(() => { + transaction.finish(); + }); + } + }); + } } + + return origReqHandler.call(this, req, res, parsedUrl); }); }; return wrappedReqHandler; } +/** + * Wrap the given method in order to use the parameterized path passed to it in the transaction name. + * + * @param origMethod Either `ensureApiPage` (called for every API request) or `findPageComponents` (called for every + * page request), both from the `Server` class + * @returns A wrapped version of the given method + */ +function makeWrappedMethodForGettingParameterizedPath( + origMethod: ApiPageEnsurer | PageComponentFinder, +): WrappedApiPageEnsurer | WrappedPageComponentFinder { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const wrappedMethod = async function(this: Server, parameterizedPath: string, ...args: any[]): Promise { + const transaction = getActiveTransaction(); + + // replace specific URL with parameterized version + if (transaction && transaction.metadata.request) { + // strip query string, if any + const origPath = transaction.metadata.request.url.split('?')[0]; + transaction.name = transaction.name.replace(origPath, parameterizedPath); + } + + return origMethod.call(this, parameterizedPath, ...args); + }; + + return wrappedMethod; +} + /** * Determine if the request should be traced, by filtering out requests for internal next files and static resources. * From 01a397fa3f6c1331de99f7a9474ff01e25ced365 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 20 May 2021 15:43:52 -0700 Subject: [PATCH 4/7] add request data to event --- packages/nextjs/src/utils/instrumentServer.ts | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index 0950b3436814..38a4633b67f8 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -1,6 +1,6 @@ import { deepReadDirSync } from '@sentry/node'; -import { Transaction } from '@sentry/types'; import { getActiveTransaction, hasTracingEnabled } from '@sentry/tracing'; +import { Event as SentryEvent, Transaction } from '@sentry/types'; import { fill, logger } from '@sentry/utils'; import * as domain from 'domain'; import * as http from 'http'; @@ -30,6 +30,7 @@ interface Server { interface NextRequest extends http.IncomingMessage { cookies: Record; url: string; + query: { [key: string]: string }; } interface NextResponse extends http.ServerResponse { @@ -200,6 +201,8 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { const currentScope = Sentry.getCurrentHub().getScope(); if (currentScope) { + currentScope.addEventProcessor(event => addRequestDataToEvent(event, req)); + // We only want to record page and API requests if (hasTracingEnabled() && shouldTraceRequest(req.url, publicDirFiles)) { // pull off query string, if any @@ -222,6 +225,8 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { if (transaction) { transaction.setHttpStatus(res.statusCode); + // we'll collect this data in a more targeted way in the event processor we added above, + // `addRequestDataToEvent` delete transaction.metadata.request; // Push `transaction.finish` to the next event loop so open spans have a chance to finish before the @@ -280,3 +285,16 @@ function shouldTraceRequest(url: string, publicDirFiles: Set): boolean { // `static` is a deprecated but still-functional location for static resources return !url.startsWith('/_next/') && !url.startsWith('/static/') && !publicDirFiles.has(url); } + +function addRequestDataToEvent(event: SentryEvent, req: NextRequest): SentryEvent { + event.request = { + // TODO body/data + url: req.url.split('?')[0], + cookies: req.cookies, + headers: req.headers as { [key: string]: string }, + method: req.method, + query_string: req.query, + }; + + return event; +} From 23f9a9e4a51e5470435738e4f41845f40bcafa84 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 21 May 2021 20:19:51 -0700 Subject: [PATCH 5/7] store only request path rather than entire request object --- packages/nextjs/src/utils/instrumentServer.ts | 9 ++++----- packages/types/src/transaction.ts | 7 ++----- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index 38a4633b67f8..f1f6d687d370 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -215,7 +215,7 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { const transaction = Sentry.startTransaction({ name: `${namePrefix}${reqPath}`, op: 'http.server', - metadata: { request: req }, + metadata: { requestPath: req.url.split('?')[0] }, }); currentScope.setSpan(transaction); @@ -227,7 +227,7 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { // we'll collect this data in a more targeted way in the event processor we added above, // `addRequestDataToEvent` - delete transaction.metadata.request; + delete transaction.metadata.requestPath; // Push `transaction.finish` to the next event loop so open spans have a chance to finish before the // transaction closes @@ -261,9 +261,8 @@ function makeWrappedMethodForGettingParameterizedPath( const transaction = getActiveTransaction(); // replace specific URL with parameterized version - if (transaction && transaction.metadata.request) { - // strip query string, if any - const origPath = transaction.metadata.request.url.split('?')[0]; + if (transaction && transaction.metadata.requestPath) { + const origPath = transaction.metadata.requestPath; transaction.name = transaction.name.replace(origPath, parameterizedPath); } diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index 2b942af0763f..805d1350bff0 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -133,9 +133,6 @@ export interface TransactionMetadata { thirdparty?: string; }; - /** For transactions tracing server-side request handling, the request being tracked. */ - request?: { - [key: string]: any; - url: string; - }; + /** For transactions tracing server-side request handling, the path of the request being tracked. */ + requestPath?: string; } From c0565f8c4f69da72831bbbe36e657d37eaf0f60b Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 21 May 2021 20:20:13 -0700 Subject: [PATCH 6/7] tweak types --- packages/nextjs/src/utils/instrumentServer.ts | 3 ++- packages/types/src/index.ts | 2 +- packages/types/src/request.ts | 4 +++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index f1f6d687d370..9292e07030c5 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -31,6 +31,7 @@ interface NextRequest extends http.IncomingMessage { cookies: Record; url: string; query: { [key: string]: string }; + headers: { [key: string]: string }; } interface NextResponse extends http.ServerResponse { @@ -290,7 +291,7 @@ function addRequestDataToEvent(event: SentryEvent, req: NextRequest): SentryEven // TODO body/data url: req.url.split('?')[0], cookies: req.cookies, - headers: req.headers as { [key: string]: string }, + headers: req.headers, method: req.method, query_string: req.query, }; diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 0123fb42ec12..9674be34db9b 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -15,7 +15,7 @@ export { Mechanism } from './mechanism'; export { ExtractedNodeRequestData, Primitive, WorkerLocation } from './misc'; export { Options } from './options'; export { Package } from './package'; -export { Request, SentryRequest, SentryRequestType } from './request'; +export { QueryParams, Request, SentryRequest, SentryRequestType } from './request'; export { Response } from './response'; export { Runtime } from './runtime'; export { CaptureContext, Scope, ScopeContext } from './scope'; diff --git a/packages/types/src/request.ts b/packages/types/src/request.ts index e0b491b9a9b4..84b135fcfb3d 100644 --- a/packages/types/src/request.ts +++ b/packages/types/src/request.ts @@ -13,8 +13,10 @@ export interface Request { url?: string; method?: string; data?: any; - query_string?: string | { [key: string]: string }; + query_string?: QueryParams; cookies?: { [key: string]: string }; env?: { [key: string]: string }; headers?: { [key: string]: string }; } + +export type QueryParams = string | { [key: string]: string } | Array<[string, string]>; From f9601390c2c64671b62665c0e424efe2bacf868c Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 21 May 2021 20:20:52 -0700 Subject: [PATCH 7/7] stop potentially overwriting request data on event --- packages/nextjs/src/utils/instrumentServer.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index 9292e07030c5..127ce3e7b344 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -288,6 +288,7 @@ function shouldTraceRequest(url: string, publicDirFiles: Set): boolean { function addRequestDataToEvent(event: SentryEvent, req: NextRequest): SentryEvent { event.request = { + ...event.request, // TODO body/data url: req.url.split('?')[0], cookies: req.cookies,