-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(nextjs): Connect trace between data-fetching methods and pageload #5655
Changes from 4 commits
d54314f
b91ff50
31c51e6
5b310dd
faaba12
0f205ec
df1d3d9
3f192f7
5a76709
663f765
cee6dc3
5c86e9a
019751b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ import { captureException, getCurrentHub, startTransaction } from '@sentry/core' | |
import { addRequestDataToEvent } from '@sentry/node'; | ||
import { getActiveTransaction } from '@sentry/tracing'; | ||
import { Transaction } from '@sentry/types'; | ||
import { fill } from '@sentry/utils'; | ||
import { extractTraceparentData, fill, isString, parseBaggageSetMutability } from '@sentry/utils'; | ||
import * as domain from 'domain'; | ||
import { IncomingMessage, ServerResponse } from 'http'; | ||
|
||
|
@@ -12,7 +12,14 @@ declare module 'http' { | |
} | ||
} | ||
|
||
function getTransactionFromRequest(req: IncomingMessage): Transaction | undefined { | ||
/** | ||
* Grabs a transaction off a Next.js datafetcher request object, if it was previously put there via | ||
* `setTransactionOnRequest`. | ||
* | ||
* @param req The Next.js datafetcher request object | ||
* @returns the Transaction on the request object if there is one, or `undefined` if the request object didn't have one. | ||
*/ | ||
export function getTransactionFromRequest(req: IncomingMessage): Transaction | undefined { | ||
return req._sentryTransaction; | ||
} | ||
Comment on lines
+15
to
24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did you choose to wrap this line in a function? (Same goes for the setter.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know to be honest. It just made sense to me to put this behind a little bit of abstraction to provide more context why this field exists on the |
||
|
||
|
@@ -38,12 +45,15 @@ export function withErrorInstrumentation<F extends (...args: any[]) => any>( | |
return function (this: unknown, ...origFunctionArguments: Parameters<F>): ReturnType<F> { | ||
try { | ||
const potentialPromiseResult = origFunction.call(this, ...origFunctionArguments); | ||
|
||
// First of all, we need to capture promise rejections so we have the following check, as well as the try-catch block. | ||
// Additionally, we do the following instead of `await`-ing so we do not change the method signature of the passed function from `() => unknown` to `() => Promise<unknown>. | ||
Promise.resolve(potentialPromiseResult).catch(err => { | ||
// TODO: Extract error logic from `withSentry` in here or create a new wrapper with said logic or something like that. | ||
captureException(err); | ||
throw err; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Small bugfix: we need to rethrow the rejected promise here, otherwise it counts as "caught" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay so this turned out to be problematic. Throwing here caused an uncaught promise rejection to bubble up, which terminates the process in Node 16 and above. I think my approach here was overly clever from the beginning so I changed it to simply await the maybe-promise result. This makes the wrapped function async but as far as I can tell that is fine since everywhere we use it, we're already async. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking more about this, if we really want this wrapper function not to be changing the function signature in the future, we can go with that approach, but without the I think the behaviour stays as expected: |
||
}); | ||
|
||
return potentialPromiseResult; | ||
} catch (e) { | ||
// TODO: Extract error logic from `withSentry` in here or create a new wrapper with said logic or something like that. | ||
|
@@ -85,13 +95,20 @@ export function callTracedServerSideDataFetcher<F extends (...args: any[]) => Pr | |
let requestTransaction: Transaction | undefined = getTransactionFromRequest(req); | ||
|
||
if (requestTransaction === undefined) { | ||
// TODO: Extract trace data from `req` object (trace and baggage headers) and attach it to transaction | ||
const sentryTraceHeader = req.headers['sentry-trace']; | ||
const rawBaggageString = req.headers && isString(req.headers.baggage) && req.headers.baggage; | ||
const traceparentData = | ||
typeof sentryTraceHeader === 'string' ? extractTraceparentData(sentryTraceHeader) : undefined; | ||
|
||
const baggage = parseBaggageSetMutability(rawBaggageString, traceparentData); | ||
|
||
const newTransaction = startTransaction({ | ||
op: 'nextjs.data.server', | ||
name: options.requestedRouteName, | ||
...traceparentData, | ||
metadata: { | ||
source: 'route', | ||
baggage, | ||
}, | ||
}); | ||
|
||
|
@@ -121,7 +138,6 @@ export function callTracedServerSideDataFetcher<F extends (...args: any[]) => Pr | |
} | ||
|
||
try { | ||
// TODO: Inject trace data into returned props | ||
return await origFunction(...origFunctionArguments); | ||
} finally { | ||
dataFetcherSpan.finish(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,11 +22,10 @@ type StartTransactionCb = (context: TransactionContext) => Transaction | undefin | |
* Describes data located in the __NEXT_DATA__ script tag. This tag is present on every page of a Next.js app. | ||
*/ | ||
interface SentryEnhancedNextData extends NextData { | ||
// contains props returned by `getInitialProps` - except for `pageProps`, these are the props that got returned by `getServerSideProps` or `getStaticProps` | ||
props: { | ||
_sentryGetInitialPropsTraceData?: string; // trace parent info, if injected by server-side `getInitialProps` | ||
_sentryGetInitialPropsBaggage?: string; // baggage, if injected by server-side `getInitialProps` | ||
Comment on lines
-25
to
-28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I got this wrong in a previous PR |
||
pageProps?: { | ||
_sentryGetInitialPropsTraceData?: string; // trace parent info, if injected by server-side `getInitialProps` | ||
_sentryGetInitialPropsBaggage?: string; // baggage, if injected by server-side `getInitialProps` | ||
_sentryGetServerSidePropsTraceData?: string; // trace parent info, if injected by server-side `getServerSideProps` | ||
_sentryGetServerSidePropsBaggage?: string; // baggage, if injected by server-side `getServerSideProps` | ||
|
||
|
@@ -79,25 +78,27 @@ function extractNextDataTagInformation(): NextDataTagInfo { | |
|
||
const { page, query, props } = nextData; | ||
|
||
// `nextData.page` always contains the parameterized route | ||
// `nextData.page` always contains the parameterized route - except for when an error occurs in a data fetching | ||
// function, then it is "/_error", but that isn't a problem since users know which route threw by looking at the | ||
// parent transaction | ||
nextDataTagInfo.route = page; | ||
nextDataTagInfo.params = query; | ||
|
||
if (props) { | ||
const { pageProps } = props; | ||
if (props && props.pageProps) { | ||
const pageProps = props.pageProps; | ||
const getInitialPropsBaggage = pageProps._sentryGetInitialPropsBaggage; | ||
const getServerSidePropsBaggage = pageProps._sentryGetServerSidePropsBaggage; | ||
const getStaticPropsBaggage = pageProps._sentryGetStaticPropsBaggage; | ||
|
||
const getInitialPropsBaggage = props._sentryGetInitialPropsBaggage; | ||
const getServerSidePropsBaggage = pageProps && pageProps._sentryGetServerSidePropsBaggage; | ||
const getStaticPropsBaggage = pageProps && pageProps._sentryGetStaticPropsBaggage; | ||
// Ordering of the following shouldn't matter but `getInitialProps` generally runs before `getServerSideProps` or `getStaticProps` so we give it priority. | ||
const baggage = getInitialPropsBaggage || getServerSidePropsBaggage || getStaticPropsBaggage; | ||
if (baggage) { | ||
nextDataTagInfo.baggage = baggage; | ||
} | ||
|
||
const getInitialPropsTraceData = props._sentryGetInitialPropsTraceData; | ||
const getServerSidePropsTraceData = pageProps && pageProps._sentryGetServerSidePropsTraceData; | ||
const getStaticPropsTraceData = pageProps && pageProps._sentryGetStaticPropsTraceData; | ||
const getInitialPropsTraceData = pageProps._sentryGetInitialPropsTraceData; | ||
const getServerSidePropsTraceData = pageProps._sentryGetServerSidePropsTraceData; | ||
const getStaticPropsTraceData = pageProps._sentryGetStaticPropsTraceData; | ||
// Ordering of the following shouldn't matter but `getInitialProps` generally runs before `getServerSideProps` or `getStaticProps` so we give it priority. | ||
const traceData = getInitialPropsTraceData || getServerSidePropsTraceData || getStaticPropsTraceData; | ||
if (traceData) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the error handling here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I was just messing around and forgot to put it back. Good catch! Changed it back in faaba12