Improve Nextjs tracing

[smeubank]

Goal: Improve tracing in nextjs SDK - make parameterization more reliable, trace data fetchers, trace page requests for folks on Vercel

TL;DR Plan:

• get page path at build time
• use loader to inject into page as global variable
• use loader to wrap canonical functions
  • getStaticPaths - start transaction, add span
  • getStaticProps - start or continue transaction, add span
  • getServerSideProps - start transaction, add span
  • getInitialProps - needs investigation, can run on client
• use loader to wrap _app or _document
  • start or continue transaction, add span, finish transaction

Effects:

• Parameterized name is known when transaction is started
• Wrapping is at page level, not server level, so works on and off of Vercel (currently tracing for page requests only works off of Vercel)
• Spans for data-fetching functions (none now)
• Hopefully lets us eventually retire instrumentServer (brittle monkeypatching, only works off of vercel)

Open Questions:

• How to deal with domains when action happens in multiple functions at multiple times?
• What about background/pre-load/data-only requests?
• How to communicate why transactions may get marginally shorter?
• How to grab request data if no GSSP?

---

Tasks

⚠️ ... Required for making changes non-experimental

Prework

General prework necessary to improve the Next.js performance experience as a whole.

• [x] ⚠️ Add loader to inject prefix for RewriteFrames (https://github.com/getsentry/sentry-javascript/pull/5445)
• [x] ⚠️ Create MVP for experimental loader to wrap getServerSideProps, getStaticProps and getStaticPaths during build-time (https://github.com/getsentry/sentry-javascript/pull/5503)
• [x] ⚠️ Actually, don't wrap getStaticPaths (https://github.com/getsentry/sentry-javascript/pull/5561)
• [x] ⚠️ Turn loader MVP into sustainable solution (https://github.com/getsentry/sentry-javascript/pull/5602)

Transaction Name Parameterization

• [x] ⚠️ Have spans and proper transaction names for getServerSideProps and getStaticProps (https://github.com/getsentry/sentry-javascript/pull/5564)
• [x] ⚠️ Have spans and proper transaction names for getInitialProps for normal pages (not _app.js, _error.js, _document.js) (https://github.com/getsentry/sentry-javascript/pull/5587)
• [x] ⚠️ Have spans and proper transaction names for getInitialProps in _app.js, _error.js and _document.js (https://github.com/getsentry/sentry-javascript/pull/5604)

Connected Traces

• [x] ⚠️ Allow for connected traces when traces start on Next.js server (frontend changes) (https://github.com/getsentry/sentry-javascript/pull/5574)
• [x] ⚠️ Extract traceparent data from incoming requests in data functions and attach it to transactions and send trace parent data and baggage from data fetching methods (Make sure to handle redirect and notFound responses from getServerSideProps (also getStaticProps?)) (https://github.com/getsentry/sentry-javascript/pull/5655)
• [x] ⚠️ Find a solution for data fetchers + navigation transactions (https://github.com/getsentry/sentry-javascript/pull/5676)
• [ ] Somehow mark prefetch requests (add isPrefetchRequest: boolean tag to errors and do the one or more of the following to transactions 1. different name 2. data field 3. different op) (there is a purpose: prefetch header in those requests)
• [ ] Solve the case when navigating to a getServerSideProps page that throws: Transaction name is /_error but DSC contains route of getServerSideProps. (See TODO comment in src/performance/client.ts)

Serverside Transaction improvements

Currently, for mysterious reasons, in some circumstances no server-side non-API-route transactions are started for Next.js apps. (This is over and above the known limitation of non-API-route tracing not working on Vercel.) The following changes will enable serverside transactions in both those mysterious situations and on Vercel.

• [x] ⚠️ Have serverside transactions by starting transactions inside getInitialProps and getServerSideProps (https://github.com/getsentry/sentry-javascript/pull/5593)
• [x] ⚠️ Add request data to transactions started by data-fetching functions (https://github.com/getsentry/sentry-javascript/pull/5703)
• [x] ⚠️ Extend RequestData integration to work for error events (https://github.com/getsentry/sentry-javascript/pull/5729)
• [ ] Automatically wrap getStaticProps (Wrapper exists but we don't have access to the request. Is this solvable? Do we even care, given that this generally runs in the background?)
• [ ] Add proper error monitoring in data fetchers (by extracting logic from withSentry into a wrapper/helper)
• [ ] ⚠️ Figure out how to block res.send similar to how we do it in withSentry (WIP @lobsterkatie)
• [ ] Figure out how to propagate scope between data-fetching functions in the same transaction (or document scope-propagation limitations)
• [ ] Delete instrumentServer.ts?
• [x] ⚠️ Add "ok" status to transactions and spans and update to "internal_error" on error (https://github.com/getsentry/sentry-javascript/pull/5777)
• [ ] ⚠️ What happens for pages with no data fetchers? Should _app have a different helper? (no name for transaction)
• [ ] ⚠️ Automatically wrap API routes (https://github.com/getsentry/sentry-javascript/pull/5778)

Folow-up/Cleanup/Polishing

• [x] ⚠️ Remove MVP loader code and remove its dependencies from package.json (https://github.com/getsentry/sentry-javascript/pull/5713)
• [ ] Use RequestData integration everywhere
• [ ] Check if new model works with CJS, if no, support people that use CJS
• [x] ⚠️ Check Webpack 4 support of the loaders we created (our integration tests run under webpack 4 just fine, so I think we're good here)
• [ ] Bring withSentry into the fold (consolidate helpers)
• [ ] Nudge users to stop wrapping API routes if using auto-wrapping (once auto-wrapping includes API routes)
• [ ] Factor more common parts out of wrappers if possible

Documentation

• [ ] Document new RequestData integration and options
• [ ] Document auto-wrapping

Misc (stretch goals)

• [ ] Have spans for getInitialProps when run client-side
• [ ] Add instrumentation for how long server-side rendering takes
• [ ] Investigate how it would be possible to support custom servers
• [ ] Can common route-handling work be consolidated into helper functions across frameworks, so we're not always reinventing the wheel?
• [ ] Middleware support

Steps for Beta

• [ ] Rename option and make it an actual option (WIP @lforst)
• [ ] Add option to wizard (https://github.com/getsentry/sentry-wizard/pull/194)
• [ ] Update docs (WIP @lforst)
• [ ] Update Next.js examples (When there is time)

Steps for GA (Planned date: October 3, 2022)

• [ ] Remove option from wizard
• [ ] Flip default for option
• [ ] (Make withSentry a noop?)
• [ ] Update docs - removing the section where we tell people to wrap routes with withSentry

[lforst]

As for

⚠️ What happens for pages with no data fetchers? Should _app have a different helper? (no name for transaction)

Thinking about what was said in the last all-hands the performance product is something that should hint towards issues that are fixable via changing code. Since pages with no data fetchers are static, the only way to make serving them faster is via infrastructure (faster machines/network/CDN). Most of the time infrastructure isn't really a dev concern (I know that sometimes it is but I believe it's not the main persona we're targeting with the product).

Additionally, I believe the frontend-span we have for the request should be enough to give users a sense of what's going on. Since there isn't any fancy computation or fetching going on in the backend when a static page is requested, a backend transaction isn't going to be very useful here, since it's always going to consist of only a singular span.

Considering the above, I suggest we mark this as non-critical for now. Do you have a different opinion on this? @lobsterkatie

[lobsterkatie]

I'm fine to leave it for now. I figured out part of what I was worried about (yes, pages with dynamic paths but without data fetchers turn into JS rather than just static HTML, but it's JS which runs on the front end, not the back end, so don't have to be dealt with here. The rest of what made me add that TODO (I was seeing transactions show up with undefined names in my testing) isn't something I can now easily replicate. If it's an issue it'll come up again.

[kachkaev]

This feature might have caused a regression: #5998

[lforst]

Let's consider this resolved for now.