feat(nextjs): Connect trace between data-fetching methods and pageload #5655
Conversation
| // 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.
Small bugfix: we need to rethrow the rejected promise here, otherwise it counts as "caught"
There was a problem hiding this comment.
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.
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 throw.
I think the behaviour stays as expected: awaiting a rejected Promise throws, and you can also catch it via .catch(). The only drawback is, that it will be marked as "caught", not causing an unhandled promise rejection to bubble up. We can maybe circumvent this via queueMicrotask: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Using_promises#when_promises_and_tasks_collide. But that again sounds a bit overly clever and is complicated to understand after the fact.
| // 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` |
There was a problem hiding this comment.
I got this wrong in a previous PR
There was a problem hiding this comment.
Looks good, but I have one question: Does it matter which data-fetching function provides the sentry-trace and baggage data? If not, is there a reason to differentiate between _sentryGetServerSidePropsBaggage and _sentryGetInitialPropsBaggage (same for senty-trace)? Could we even just do something like:
pageProps._sentryTracingHeaders = {"sentry-trace": xxxxx, bagage: yyyyy}?
| } else { | ||
| return errorWrappedGetServerSideProps(...getServerSidePropsArguments); | ||
| return origGetServerSideProps(...getServerSidePropsArguments); |
There was a problem hiding this comment.
Why remove the error handling here?
There was a problem hiding this comment.
I think I was just messing around and forgot to put it back. Good catch! Changed it back in faaba12
size-limit report 📦
|
@lobsterkatie No, it doesn't matter at all, so this is excellent feedback. Thank you! I changed it so we're only setting |
@AbhiPrasad I added some. |
lforst
force-pushed
the
lforst-next-connected-traces
branch
from
b8c734e
to
cee6dc3
Compare
| /** | ||
| * 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; | ||
| } |
There was a problem hiding this comment.
Why did you choose to wrap this line in a function? (Same goes for the setter.)
There was a problem hiding this comment.
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 req object. Feel free to remove it in the future though! I think the ambient type (declare module 'http' { at the top of the file) provides enough context.
Ref: #5505
Connects traces between data fetching methods and the client by returning trace parent data and baggage from the data fetchers in a way that the client understands.
Additionally:
We don't return baggage + trace parent info from
_document'sgetInitialPropsbecause those props are not put into the__NEXT_DATA__tag.