From 66cc65e425fda63428591857614dcbd4111560e9 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 13 Jul 2021 09:19:24 -0700 Subject: [PATCH 01/11] add logging when flushing events --- packages/nextjs/src/utils/handlers.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/nextjs/src/utils/handlers.ts b/packages/nextjs/src/utils/handlers.ts index af0114258fad..c9ab16a3f45b 100644 --- a/packages/nextjs/src/utils/handlers.ts +++ b/packages/nextjs/src/utils/handlers.ts @@ -84,9 +84,12 @@ export const withSentry = (handler: NextApiHandler): WrappedNextApiHandler => { transaction.finish(); } try { + logger.log('Flushing events...'); await flush(2000); } catch (e) { - // no-empty + logger.log(`Error while flushing events:\n${e}`); + } finally { + logger.log('Done flushing events'); } } }); From 62189efca1e3fe371ae158acd4d8e6b7bf99d59a Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 13 Jul 2021 15:57:46 -0700 Subject: [PATCH 02/11] wrap res.end --- packages/nextjs/src/utils/handlers.ts | 42 +++++++++++++++++---------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/packages/nextjs/src/utils/handlers.ts b/packages/nextjs/src/utils/handlers.ts index c9ab16a3f45b..88ef2c023593 100644 --- a/packages/nextjs/src/utils/handlers.ts +++ b/packages/nextjs/src/utils/handlers.ts @@ -2,7 +2,7 @@ import { captureException, flush, getCurrentHub, Handlers, startTransaction, wit import { extractTraceparentData, getActiveTransaction, hasTracingEnabled } from '@sentry/tracing'; import { addExceptionMechanism, isString, logger, stripUrlQueryAndFragment } from '@sentry/utils'; import * as domain from 'domain'; -import { NextApiHandler } from 'next'; +import { NextApiHandler, NextApiResponse } from 'next'; import { addRequestDataToEvent, NextRequest } from './instrumentServer'; @@ -15,6 +15,31 @@ type WrappedNextApiHandler = NextApiHandler; export const withSentry = (handler: NextApiHandler): WrappedNextApiHandler => { // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types return async (req, res) => { + const origEnd = res.end; + + async function newEnd(this: NextApiResponse, ...args: any[]) { + const transaction = getActiveTransaction(); + + if (transaction) { + transaction.setHttpStatus(res.statusCode); + + transaction.finish(); + } + try { + logger.log('Flushing events...'); + await flush(2000); + } catch (e) { + logger.log(`Error while flushing events:\n${e}`); + } finally { + logger.log('Done flushing events'); + // res.end(); + } + + return origEnd.call(this, ...args); + } + + res.end = newEnd; + // wrap everything in a domain in order to prevent scope bleed between requests const local = domain.create(); local.add(req); @@ -76,21 +101,6 @@ export const withSentry = (handler: NextApiHandler): WrappedNextApiHandler => { captureException(e); }); throw e; - } finally { - const transaction = getActiveTransaction(); - if (transaction) { - transaction.setHttpStatus(res.statusCode); - - transaction.finish(); - } - try { - logger.log('Flushing events...'); - await flush(2000); - } catch (e) { - logger.log(`Error while flushing events:\n${e}`); - } finally { - logger.log('Done flushing events'); - } } }); From 344c5f31d30b3d8b55598104779974712c19908c Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 13 Jul 2021 22:47:56 -0700 Subject: [PATCH 03/11] move newEnd into a factory function --- packages/nextjs/src/utils/handlers.ts | 77 ++++++++++++++++++--------- 1 file changed, 51 insertions(+), 26 deletions(-) diff --git a/packages/nextjs/src/utils/handlers.ts b/packages/nextjs/src/utils/handlers.ts index 88ef2c023593..0d145101d797 100644 --- a/packages/nextjs/src/utils/handlers.ts +++ b/packages/nextjs/src/utils/handlers.ts @@ -1,5 +1,6 @@ import { captureException, flush, getCurrentHub, Handlers, startTransaction, withScope } from '@sentry/node'; -import { extractTraceparentData, getActiveTransaction, hasTracingEnabled } from '@sentry/tracing'; +import { extractTraceparentData, hasTracingEnabled } from '@sentry/tracing'; +import { Transaction } from '@sentry/types'; import { addExceptionMechanism, isString, logger, stripUrlQueryAndFragment } from '@sentry/utils'; import * as domain from 'domain'; import { NextApiHandler, NextApiResponse } from 'next'; @@ -11,36 +12,18 @@ const { parseRequest } = Handlers; // purely for clarity type WrappedNextApiHandler = NextApiHandler; +type AugmentedResponse = NextApiResponse & { __sentryTransaction?: Transaction }; + // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types export const withSentry = (handler: NextApiHandler): WrappedNextApiHandler => { // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types return async (req, res) => { - const origEnd = res.end; - - async function newEnd(this: NextApiResponse, ...args: any[]) { - const transaction = getActiveTransaction(); - - if (transaction) { - transaction.setHttpStatus(res.statusCode); - - transaction.finish(); - } - try { - logger.log('Flushing events...'); - await flush(2000); - } catch (e) { - logger.log(`Error while flushing events:\n${e}`); - } finally { - logger.log('Done flushing events'); - // res.end(); - } - - return origEnd.call(this, ...args); - } - - res.end = newEnd; + // first order of business: monkeypatch `res.end()` so that it will wait for us to send events to sentry before it + // fires (if we don't do this, the lambda will close too early and events will be either delayed or lost) + // eslint-disable-next-line @typescript-eslint/unbound-method + res.end = wrapEndMethod(res.end); - // wrap everything in a domain in order to prevent scope bleed between requests + // use a domain in order to prevent scope bleed between requests const local = domain.create(); local.add(req); local.add(res); @@ -86,6 +69,10 @@ export const withSentry = (handler: NextApiHandler): WrappedNextApiHandler => { { request: req }, ); currentScope.setSpan(transaction); + + // save a link to the transaction on the response, so that even if there's an error (landing us outside of + // the domain), we can still finish it (albeit possibly missing some scope data) + (res as AugmentedResponse).__sentryTransaction = transaction; } } @@ -107,3 +94,41 @@ export const withSentry = (handler: NextApiHandler): WrappedNextApiHandler => { return await boundHandler(); }; }; + +type ResponseEndMethod = AugmentedResponse['end']; +type WrappedResponseEndMethod = AugmentedResponse['end']; + +function wrapEndMethod(origEnd: ResponseEndMethod): WrappedResponseEndMethod { + return async function newEnd(this: AugmentedResponse, ...args: unknown[]) { + // TODO: if the handler errored, it will have popped us out of the domain, so all of our scope data will be missing + + const transaction = this.__sentryTransaction; + + if (transaction) { + transaction.setHttpStatus(this.statusCode); + + // Push `transaction.finish` to the next event loop so open spans have a better chance of finishing before the + // transaction closes, and make sure to wait until that's done before flushing events + const transactionFinished: Promise = new Promise(resolve => { + setImmediate(() => { + transaction.finish(); + resolve(); + }); + }); + await transactionFinished; + } + + // flush the event queue to ensure that events get sent to Sentry before the response is finished and the lambda + // ends + try { + logger.log('Flushing events...'); + await flush(2000); + } catch (e) { + logger.log(`Error while flushing events:\n${e}`); + } finally { + logger.log('Done flushing events'); + } + + return origEnd.call(this, ...args); + }; +} From 9e7e9c02ccd0a98a1afc2626d2228f3d1e3bf442 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 15 Jul 2021 19:35:37 -0700 Subject: [PATCH 04/11] restrict try to original route handler --- packages/nextjs/src/utils/handlers.ts | 74 +++++++++++++-------------- 1 file changed, 37 insertions(+), 37 deletions(-) diff --git a/packages/nextjs/src/utils/handlers.ts b/packages/nextjs/src/utils/handlers.ts index 0d145101d797..e1986be7f060 100644 --- a/packages/nextjs/src/utils/handlers.ts +++ b/packages/nextjs/src/utils/handlers.ts @@ -32,50 +32,50 @@ export const withSentry = (handler: NextApiHandler): WrappedNextApiHandler => { // return a value. In our case, all any of the codepaths return is a promise of `void`, but nextjs still counts on // getting that before it will finish the response. const boundHandler = local.bind(async () => { - try { - const currentScope = getCurrentHub().getScope(); + const currentScope = getCurrentHub().getScope(); - if (currentScope) { - currentScope.addEventProcessor(event => addRequestDataToEvent(event, req as NextRequest)); + if (currentScope) { + currentScope.addEventProcessor(event => addRequestDataToEvent(event, req as NextRequest)); - if (hasTracingEnabled()) { - // 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); - logger.log(`[Tracing] Continuing trace ${traceparentData?.traceId}.`); - } + if (hasTracingEnabled()) { + // 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); + logger.log(`[Tracing] Continuing trace ${traceparentData?.traceId}.`); + } - const url = `${req.url}`; - // pull off query string, if any - let reqPath = stripUrlQueryAndFragment(url); - // Replace with placeholder - if (req.query) { - // TODO get this from next if possible, to avoid accidentally replacing non-dynamic parts of the path if - // they match dynamic parts - for (const [key, value] of Object.entries(req.query)) { - reqPath = reqPath.replace(`${value}`, `[${key}]`); - } + const url = `${req.url}`; + // pull off query string, if any + let reqPath = stripUrlQueryAndFragment(url); + // Replace with placeholder + if (req.query) { + // TODO get this from next if possible, to avoid accidentally replacing non-dynamic parts of the path if + // they match dynamic parts + for (const [key, value] of Object.entries(req.query)) { + reqPath = reqPath.replace(`${value}`, `[${key}]`); } - const reqMethod = `${(req.method || 'GET').toUpperCase()} `; - - const transaction = startTransaction( - { - name: `${reqMethod}${reqPath}`, - op: 'http.server', - ...traceparentData, - }, - // extra context passed to the `tracesSampler` - { request: req }, - ); - currentScope.setSpan(transaction); - - // save a link to the transaction on the response, so that even if there's an error (landing us outside of - // the domain), we can still finish it (albeit possibly missing some scope data) - (res as AugmentedResponse).__sentryTransaction = transaction; } + const reqMethod = `${(req.method || 'GET').toUpperCase()} `; + + const transaction = startTransaction( + { + name: `${reqMethod}${reqPath}`, + op: 'http.server', + ...traceparentData, + }, + // extra context passed to the `tracesSampler` + { request: req }, + ); + currentScope.setSpan(transaction); + + // save a link to the transaction on the response, so that even if there's an error (landing us outside of + // the domain), we can still finish it (albeit possibly missing some scope data) + (res as AugmentedResponse).__sentryTransaction = transaction; } + } + try { return await handler(req, res); // Call original handler } catch (e) { withScope(scope => { From 6bb2487b2326c882aa79b289037044114292beec Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Wed, 14 Jul 2021 18:15:14 -0700 Subject: [PATCH 05/11] use current scope rather than pushing a new one on the stack --- packages/nextjs/src/utils/handlers.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/nextjs/src/utils/handlers.ts b/packages/nextjs/src/utils/handlers.ts index e1986be7f060..3c0b92728167 100644 --- a/packages/nextjs/src/utils/handlers.ts +++ b/packages/nextjs/src/utils/handlers.ts @@ -1,4 +1,4 @@ -import { captureException, flush, getCurrentHub, Handlers, startTransaction, withScope } from '@sentry/node'; +import { captureException, flush, getCurrentHub, Handlers, startTransaction } from '@sentry/node'; import { extractTraceparentData, hasTracingEnabled } from '@sentry/tracing'; import { Transaction } from '@sentry/types'; import { addExceptionMechanism, isString, logger, stripUrlQueryAndFragment } from '@sentry/utils'; @@ -78,15 +78,15 @@ export const withSentry = (handler: NextApiHandler): WrappedNextApiHandler => { try { return await handler(req, res); // Call original handler } catch (e) { - withScope(scope => { - scope.addEventProcessor(event => { + if (currentScope) { + currentScope.addEventProcessor(event => { addExceptionMechanism(event, { handled: false, }); return parseRequest(event, req); }); captureException(e); - }); + } throw e; } }); From 34bec18aad8879089c9b797592094352065c3e9e Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 15 Jul 2021 19:39:25 -0700 Subject: [PATCH 06/11] stop adding request data twice when there's an error, use parseRequest --- packages/nextjs/src/utils/handlers.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/nextjs/src/utils/handlers.ts b/packages/nextjs/src/utils/handlers.ts index 3c0b92728167..3be1e4b8662b 100644 --- a/packages/nextjs/src/utils/handlers.ts +++ b/packages/nextjs/src/utils/handlers.ts @@ -5,8 +5,6 @@ import { addExceptionMechanism, isString, logger, stripUrlQueryAndFragment } fro import * as domain from 'domain'; import { NextApiHandler, NextApiResponse } from 'next'; -import { addRequestDataToEvent, NextRequest } from './instrumentServer'; - const { parseRequest } = Handlers; // purely for clarity @@ -35,7 +33,7 @@ export const withSentry = (handler: NextApiHandler): WrappedNextApiHandler => { const currentScope = getCurrentHub().getScope(); if (currentScope) { - currentScope.addEventProcessor(event => addRequestDataToEvent(event, req as NextRequest)); + currentScope.addEventProcessor(event => parseRequest(event, req)); if (hasTracingEnabled()) { // If there is a trace header set, extract the data from it (parentSpanId, traceId, and sampling decision) @@ -83,7 +81,7 @@ export const withSentry = (handler: NextApiHandler): WrappedNextApiHandler => { addExceptionMechanism(event, { handled: false, }); - return parseRequest(event, req); + return event; }); captureException(e); } From eaa9987259ed0b281c97a53f7b16391261a19dfe Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 16 Jul 2021 10:08:50 -0700 Subject: [PATCH 07/11] make code review changes --- packages/nextjs/src/utils/handlers.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/nextjs/src/utils/handlers.ts b/packages/nextjs/src/utils/handlers.ts index 3be1e4b8662b..8a134e89600c 100644 --- a/packages/nextjs/src/utils/handlers.ts +++ b/packages/nextjs/src/utils/handlers.ts @@ -121,10 +121,9 @@ function wrapEndMethod(origEnd: ResponseEndMethod): WrappedResponseEndMethod { try { logger.log('Flushing events...'); await flush(2000); + logger.log('Done flushing events'); } catch (e) { logger.log(`Error while flushing events:\n${e}`); - } finally { - logger.log('Done flushing events'); } return origEnd.call(this, ...args); From d8c11818abd9f62f556770f8b7efcccb5042175f Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 16 Jul 2021 14:50:28 -0700 Subject: [PATCH 08/11] linting fixes --- packages/nextjs/test/integration/test/runner.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/nextjs/test/integration/test/runner.js b/packages/nextjs/test/integration/test/runner.js index 23eba29292c2..582396e1caf4 100644 --- a/packages/nextjs/test/integration/test/runner.js +++ b/packages/nextjs/test/integration/test/runner.js @@ -52,7 +52,11 @@ const runScenario = async (scenario, execute, env) => { }; const runScenarios = async (scenarios, execute, env) => { - return Promise.all(scenarios.map(scenario => runScenario(scenario, execute, env))); + return Promise.all( + scenarios.map(scenario => { + return runScenario(scenario, execute, env); + }), + ); }; module.exports.run = async ({ From e7945c1876b4e8e33d24620b27902539cf01ad20 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 16 Jul 2021 14:33:24 -0700 Subject: [PATCH 09/11] match on full url --- .../test/integration/test/server/errorApiEndpoint.js | 8 +++++--- .../test/integration/test/server/errorServerSideProps.js | 8 +++++--- .../nextjs/test/integration/test/server/tracing200.js | 8 +++++--- .../nextjs/test/integration/test/server/tracing500.js | 7 ++++--- .../nextjs/test/integration/test/server/tracingHttp.js | 8 +++++--- 5 files changed, 24 insertions(+), 15 deletions(-) diff --git a/packages/nextjs/test/integration/test/server/errorApiEndpoint.js b/packages/nextjs/test/integration/test/server/errorApiEndpoint.js index e622cac1ba0c..c48cc229a8f6 100644 --- a/packages/nextjs/test/integration/test/server/errorApiEndpoint.js +++ b/packages/nextjs/test/integration/test/server/errorApiEndpoint.js @@ -3,7 +3,9 @@ const assert = require('assert'); const { sleep } = require('../utils/common'); const { getAsync, interceptEventRequest } = require('../utils/server'); -module.exports = async ({ url, argv }) => { +module.exports = async ({ url: urlBase, argv }) => { + const url = `${urlBase}/api/error`; + const capturedRequest = interceptEventRequest( { exception: { @@ -18,7 +20,7 @@ module.exports = async ({ url, argv }) => { runtime: 'node', }, request: { - url: `${url}/api/error`, + url, method: 'GET', }, transaction: 'GET /api/error', @@ -27,7 +29,7 @@ module.exports = async ({ url, argv }) => { 'errorApiEndpoint', ); - await getAsync(`${url}/api/error`); + await getAsync(url); await sleep(100); assert.ok(capturedRequest.isDone(), 'Did not intercept expected request'); diff --git a/packages/nextjs/test/integration/test/server/errorServerSideProps.js b/packages/nextjs/test/integration/test/server/errorServerSideProps.js index d486daab43d6..b18b099f8af0 100644 --- a/packages/nextjs/test/integration/test/server/errorServerSideProps.js +++ b/packages/nextjs/test/integration/test/server/errorServerSideProps.js @@ -3,7 +3,9 @@ const assert = require('assert'); const { sleep } = require('../utils/common'); const { getAsync, interceptEventRequest } = require('../utils/server'); -module.exports = async ({ url, argv }) => { +module.exports = async ({ url: urlBase, argv }) => { + const url = `${urlBase}/withServerSideProps`; + const capturedRequest = interceptEventRequest( { exception: { @@ -18,7 +20,7 @@ module.exports = async ({ url, argv }) => { runtime: 'node', }, request: { - url: '/withServerSideProps', + url, method: 'GET', }, }, @@ -26,7 +28,7 @@ module.exports = async ({ url, argv }) => { 'errorServerSideProps', ); - await getAsync(`${url}/withServerSideProps`); + await getAsync(url); await sleep(100); assert.ok(capturedRequest.isDone(), 'Did not intercept expected request'); diff --git a/packages/nextjs/test/integration/test/server/tracing200.js b/packages/nextjs/test/integration/test/server/tracing200.js index 31701c9941ca..fa8ae1d69abb 100644 --- a/packages/nextjs/test/integration/test/server/tracing200.js +++ b/packages/nextjs/test/integration/test/server/tracing200.js @@ -3,7 +3,9 @@ const assert = require('assert'); const { sleep } = require('../utils/common'); const { getAsync, interceptTracingRequest } = require('../utils/server'); -module.exports = async ({ url, argv }) => { +module.exports = async ({ url: urlBase, argv }) => { + const url = `${urlBase}/api/users`; + const capturedRequest = interceptTracingRequest( { contexts: { @@ -16,14 +18,14 @@ module.exports = async ({ url, argv }) => { transaction: 'GET /api/users', type: 'transaction', request: { - url: '/api/users', + url, }, }, argv, 'tracing200', ); - await getAsync(`${url}/api/users`); + await getAsync(url); await sleep(100); assert.ok(capturedRequest.isDone(), 'Did not intercept expected request'); diff --git a/packages/nextjs/test/integration/test/server/tracing500.js b/packages/nextjs/test/integration/test/server/tracing500.js index fc8f51cf8d27..1958478a598f 100644 --- a/packages/nextjs/test/integration/test/server/tracing500.js +++ b/packages/nextjs/test/integration/test/server/tracing500.js @@ -3,7 +3,8 @@ const assert = require('assert'); const { sleep } = require('../utils/common'); const { getAsync, interceptTracingRequest } = require('../utils/server'); -module.exports = async ({ url, argv }) => { +module.exports = async ({ url: urlBase, argv }) => { + const url = `${urlBase}/api/broken`; const capturedRequest = interceptTracingRequest( { contexts: { @@ -16,14 +17,14 @@ module.exports = async ({ url, argv }) => { transaction: 'GET /api/broken', type: 'transaction', request: { - url: '/api/broken', + url, }, }, argv, 'tracing500', ); - await getAsync(`${url}/api/broken`); + await getAsync(url); await sleep(100); assert.ok(capturedRequest.isDone(), 'Did not intercept expected request'); diff --git a/packages/nextjs/test/integration/test/server/tracingHttp.js b/packages/nextjs/test/integration/test/server/tracingHttp.js index de48c3468f7b..27d989c4395a 100644 --- a/packages/nextjs/test/integration/test/server/tracingHttp.js +++ b/packages/nextjs/test/integration/test/server/tracingHttp.js @@ -5,7 +5,9 @@ const nock = require('nock'); const { sleep } = require('../utils/common'); const { getAsync, interceptTracingRequest } = require('../utils/server'); -module.exports = async ({ url, argv }) => { +module.exports = async ({ url: urlBase, argv }) => { + const url = `${urlBase}/api/http`; + nock('http://example.com') .get('/') .reply(200, 'ok'); @@ -30,14 +32,14 @@ module.exports = async ({ url, argv }) => { transaction: 'GET /api/http', type: 'transaction', request: { - url: '/api/http', + url, }, }, argv, 'tracingHttp', ); - await getAsync(`${url}/api/http`); + await getAsync(url); await sleep(100); assert.ok(capturedRequest.isDone(), 'Did not intercept expected request'); From 88ab3c7556f9872ed552c5d13a5bc1afb39d03b1 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 19 Jul 2021 22:57:34 -0700 Subject: [PATCH 10/11] use parseRequest in instrumentServer --- packages/nextjs/src/utils/instrumentServer.ts | 31 +++---------------- 1 file changed, 4 insertions(+), 27 deletions(-) diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index d55ec62851ac..eee338cde538 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -1,6 +1,5 @@ -import { captureException, deepReadDirSync, getCurrentHub, startTransaction } from '@sentry/node'; +import { captureException, deepReadDirSync, getCurrentHub, Handlers, startTransaction } from '@sentry/node'; import { extractTraceparentData, getActiveTransaction, hasTracingEnabled } from '@sentry/tracing'; -import { Event as SentryEvent } from '@sentry/types'; import { fill, isString, logger, stripUrlQueryAndFragment } from '@sentry/utils'; import * as domain from 'domain'; import * as http from 'http'; @@ -8,6 +7,8 @@ import { default as createNextServer } from 'next'; import * as querystring from 'querystring'; import * as url from 'url'; +const { parseRequest } = Handlers; + // eslint-disable-next-line @typescript-eslint/no-explicit-any type PlainObject = { [key: string]: T }; @@ -193,7 +194,7 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { const currentScope = getCurrentHub().getScope(); if (currentScope) { - currentScope.addEventProcessor(event => addRequestDataToEvent(event, req)); + currentScope.addEventProcessor(event => parseRequest(event, req)); // We only want to record page and API requests if (hasTracingEnabled() && shouldTraceRequest(req.url, publicDirFiles)) { @@ -288,27 +289,3 @@ 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); } - -/** - * Harvest specific data from the request, and add it to the event. - * - * @param event The event to which to add request data - * @param req The request whose data is being added - * @returns The modified event - */ -export function addRequestDataToEvent(event: SentryEvent, req: NextRequest): SentryEvent { - // TODO (breaking change): Replace all calls to this function with `parseRequest(event, req, { transaction: false })` - // (this is breaking because doing so will change `event.request.url` from a path into an absolute URL, which might - // mess up various automations in the Sentry web app - alert rules, auto assignment, saved searches, etc) - event.request = { - ...event.request, - data: req.body, - url: req.url.split('?')[0], - cookies: req.cookies, - headers: req.headers, - method: req.method, - query_string: req.query, - }; - - return event; -} From a4763f54f94ed00d480b4b33ee506ffe1211bf79 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 19 Jul 2021 23:00:49 -0700 Subject: [PATCH 11/11] capture transaction as well as error in errorApiEndpoint --- .../test/server/errorApiEndpoint.js | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/packages/nextjs/test/integration/test/server/errorApiEndpoint.js b/packages/nextjs/test/integration/test/server/errorApiEndpoint.js index c48cc229a8f6..f0d9985a1871 100644 --- a/packages/nextjs/test/integration/test/server/errorApiEndpoint.js +++ b/packages/nextjs/test/integration/test/server/errorApiEndpoint.js @@ -1,12 +1,12 @@ const assert = require('assert'); const { sleep } = require('../utils/common'); -const { getAsync, interceptEventRequest } = require('../utils/server'); +const { getAsync, interceptEventRequest, interceptTracingRequest } = require('../utils/server'); module.exports = async ({ url: urlBase, argv }) => { const url = `${urlBase}/api/error`; - const capturedRequest = interceptEventRequest( + const capturedErrorRequest = interceptEventRequest( { exception: { values: [ @@ -29,8 +29,28 @@ module.exports = async ({ url: urlBase, argv }) => { 'errorApiEndpoint', ); + const capturedTransactionRequest = interceptTracingRequest( + { + contexts: { + trace: { + op: 'http.server', + status: 'internal_error', + tags: { 'http.status_code': '500' }, + }, + }, + transaction: 'GET /api/error', + type: 'transaction', + request: { + url, + }, + }, + argv, + 'errorApiEndpoint', + ); + await getAsync(url); await sleep(100); - assert.ok(capturedRequest.isDone(), 'Did not intercept expected request'); + assert.ok(capturedErrorRequest.isDone(), 'Did not intercept expected error request'); + assert.ok(capturedTransactionRequest.isDone(), 'Did not intercept expected transaction request'); };