Skip to content
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

Improve Nextjs tracing #5505

Closed
26 of 43 tasks
smeubank opened this issue Aug 1, 2022 · 4 comments
Closed
26 of 43 tasks

Improve Nextjs tracing #5505

smeubank opened this issue Aug 1, 2022 · 4 comments
Assignees
Labels
Package: nextjs Issues related to the Sentry Nextjs SDK

Comments

@smeubank
Copy link
Member

smeubank commented Aug 1, 2022

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.

Transaction Name Parameterization

Connected Traces

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.

Folow-up/Cleanup/Polishing

  • ⚠️ Remove MVP loader code and remove its dependencies from package.json (chore(nextjs): Remove obsolete dataFetchers loader #5713)
  • Use RequestData integration everywhere
  • Check if new model works with CJS, if no, support people that use CJS
  • ⚠️ 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?

Steps for Beta

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


Follow ups

Undone tasks from above have been sorted into:

@AbhiPrasad AbhiPrasad added Status: In Progress Package: nextjs Issues related to the Sentry Nextjs SDK labels Aug 2, 2022
@lobsterkatie lobsterkatie changed the title Improve Nextjs parameterization Improve Nextjs tracing Aug 12, 2022
lobsterkatie added a commit that referenced this issue Aug 19, 2022
#5602)

This changes the experimental auto-wrapping feature to use a templated proxy module for all wrapping. (Previously, the wrapping was done using a different mix of code parsing, AST manipulation, proxying, templating, and string concatenation for each function.) Not only should this be easier to reason about and maintain (because it's one unified method of wrapping), it also solves a number of the disadvantages inherent in various parts of the previous approach. Specifically, using a template for everything rather than storing code as strings lets us take advantage of linting and syntax highlighting, and using a proxy loader rather than dealing with the AST directly puts the onus of handling syntax variations and edge cases on tools which are actually designed for that purpose.

At a high level, the proxy loader works like this:

- During the nextjs build phase, webpack loads all of the files in the `pages` directory, and feeds each one to our loader.
- The loader derives the parameterized route from the page file's path, and fills it and the path itself into the template.
- In the template, we load the page module, replace any data-fetching methods we find with wrapped versions of themselves, and then re-export everything.
- The contents of the template is returned by the loader in place of the original contents of the page module.

Previously, when working directly with the page module's AST, we had to account for the very many ways functions can be defined and exported. By contrast, doing the function wrapping in a separate module allows us to take advantage of the fact that imported modules have a single, known structure, which we can modify directly in the template code.

Notes:

- For some reason, nextjs won't accept data fetchers which are exported as part of an `export * from '...'` statement. Therefore, the "re-export everything" part of the third step above needs to be of the form `export { all, of, the, things, which, the, page, module, exports, listed, individually } from './pageModule'`. This in turn requires knowing the full list of each page module's exports, since, unfortunately, `export { ...importedPageModule }` isn't a thing. As it turns out, one of the noticeable differences between our published code before and after the build process revamp in the spring is that where `tsc` leaves `export *` statements untouched, Rollup splits them out into individual exports - exactly what's needed here! The loader therefore uses Rollup's JS API to process the proxy module code before returning it. Further, in order that Rollup be able to understand the page module code (which will be written in either `jsx` or `tsx`), we first use Sucrase to transpile the code to vanilla JS. Who knew the build process work would come in so handy?

- Given that we replace a page module's contents with the proxy code the first time webpack tries to load it, we need webpack to load the same module a second time, in order to be able to process and bundle the page module itself. We therefore attach a query string to the end of the page module's path wherever it's referenced in the template, because this makes Webpack think it is a different, as-yet-unloaded module, causing it to perform the second load. The query string also acts like a flag for us, so that the second time through we know we've already handled the file and can let it pass through the loader untouched.

- Rollup expects the entry point to be given as a path to a file on disk, not as raw code. We therefore create a temporary file for each page's proxy module, which we then delete as soon as rollup has read it. The easiest way to make sure that relative paths are preserved when things are re-exported is to put each temporary file alongside the page module it's wrapping, in the `pages/` directory. Fortunately, by the time our loader is running, Webpack has already decided what files it needs to load, so these temporary files don't get caught up in the bundling process.

- In order to satisfy the linter in the template file, the SDK itself has been added as a dev dependency. Fortunately this seems not to confuse yarn.

- Just to keep things manageable, this stops using but doesn't yet delete the previous loader (and associated files/code). Once this is merged, I'll do that in a separate PR.

Ref #5505
timfish pushed a commit to timfish/sentry-javascript that referenced this issue Aug 22, 2022
…rappers (getsentry#5564)

This adds span creation and route parameterization to the experimental `getStaticProps` and `getServerSideProps` wrappers. In order to do the parameterization, we store the parameterized route( which is the same as the filepath of the page file) as a global variable in the code we add using the `dataFetchersLoader` and then pass it to the wrappers, which then replace the name of the active transaction with the parameterized route we've stored. It also adds basic error-capturing to the wrappers.

Open issues/questions (not solved by this PR):

- Because of prefetching, revalidation of static pages, and the like, the data fetching functions can run in the background, during handling of a route different from their own. (For example, if page A has a nextjs `Link` component pointing to page B, next will prefetch the data for B (and therefore run its data fetching functions) while handling the request for A. We need to make sure that we don't accidentally overwrite the transaction name in that case to B.
- There's more we should do in terms of error handling. Specifically, we should port much of the logic in `withSentry` into these wrappers also.
- Porting the error-handling logic will mean we need to deal with the domain question - how to keep reactivating the correct domain as we go into and out of data fetching and rendering functions.
 
(There are other items listed in the meta issue linked below, but the above are the ones directly relevant to the work in this PR.)

Ref: getsentry#5505
lobsterkatie added a commit that referenced this issue Sep 19, 2022
#5703)

In most of our Node-based SDKs, we use domains to prevent scope bleed between requests, running the entire wrapped event-handling process inside a single domain. This works because in those cases, there is only one request-handling entry point for us to wrap, so we can stick the entire thing in a single `domain.run()` call and (more or less) rely on the fact that we've got a single, consistent, unique-to-that-request `Scope` object that we're dealing with. We've followed this pattern in the nextjs SDK as well, both in `instrumentServer` and `withSentry`. The new auto-wrapping of data-fetching functions presents a challenge, though, because a single request may involve a number of our wrapped functions running at different points in the request handling process, with no shared (little S) scope between them. While we still use domains when wrapping the data fetchers, a single request may pass through multiple domains during its lifecycle, preventing us from using any given domain as a universal `Scope`-carrier for a single `Scope` instance.

One place where this becomes is problem is our use of `addRequestDataToEvent`, which up until now we've just been calling inside a single-request event processor added to the current `Scope`. In this system, each event processor holds a reference to its particular `req` object. In the case of the data-fetchers, however, the `Scope` instance to which we might add such an event processor isn't the same as the one which will be active when the event is being processed, so the current way doesn't work. But given that the only way in which our current, single-request-specific event processors differ is by the request to which they hold a reference, they can be replaced by a single, and universal, event processor, as long as we can access `req` a different way besides keeping it in a closure as we do now.

This PR does just that (that is, switches to using a single event processor) for transaction events. First, a reference to `req` is stored in the transaction's metadata (which is then available to event processors as `sdkProcessingMetadata`). Then a new default integration, `RequestData`, pulls `req` out of the metadata and uses it to add a `request` property to the event. 

Notes:

- The options API for the new integration is inspired by, but different from, the options API for our Express request handler. (When we work on cleaning up the request data utility functions as part of fixing #5718, we might consider bringing those options in line with these.) The primary differences are:
  - The options have been (almost entirely) flattened. (It never made sense that inside of options for request data, you had a `request` key holding a subset of the options.) Now everything is at the same level and only takes a boolean. The one exception is `user`, which can still take a boolean or a list of attributes.
  - In the handler options, `transaction` can either be a boolean or a `TransactionNamingScheme`. In the integration, it can no longer be a boolean - events are going to have transactions, one way or another, so we shouldn't let people set it to `false`. Further, since it's now not about whether `transaction` is or isn't included, it's been moved out of `include` into its own `transactionNamingScheme` option. (Note that the new option is not yet used, but will be in future PRs.)
  - `method` has also been moved out of include, and has been hardcoded to `true`, since it's an integral part of naming the request. We currently include it in the transaction name, regardless of the setting, so again here, letting people set it to `false` makes no sense.

- Though `req` has been added to transaction metadata everywhere we start transactions, the existing domain/`Scope`-based event processors haven't yet been removed, because this new method only works for transactions, not errors. (Solving that will be the subject of a future PR.) The existing processors have been modified to not apply to transaction events, however.

- Though we may at some point use the `RequestData` integration added here in browser-based SDKs as well, both in this PR and in near-term future PRs it will only be used in a Node context. It's therefore been added to `@sentry/node`, to prevent a repeat of the dependency injection mess [we just undid](#5759).

- No integration tests were added specifically for this change, because the existing integration tests already test that transaction events include a `request` property, so as long as they continue to pass, we know that using the integration instead of an event processor is working.

Ref: #5505
@lforst
Copy link
Member

lforst commented Sep 20, 2022

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 added a commit that referenced this issue Sep 26, 2022
As part of #5505, this applies to API route handlers the same kind of auto-wrapping we've done with the data fetchers (`withServerSideProps` and the like). Though the general idea is the same, the one extra complicating factor here is that there's a good chance the handlers get to us already wrapped in `withSentry`, which we've up until now been telling users to use as a manual wrapper. This is handled by making `withSentry` idempotent - if it detects that it's already been run on the current request, it simply acts as a pass-through to the function it wraps.

Notes:

- A new template has been created to act as a proxy module for API routes, but the proxying work itself is done by the same `proxyLoader` as before - it just loads one template or the other depending on an individual page's path.

- Doing this auto-wrapping gives us a chance to do one thing manual `withSentry` wrapping isn't able to do, which is set the [route config](https://nextjs.org/docs/api-routes/request-helpers) to use an external resolver, which will prevent next's dev server from throwing warnings about API routes not sending responses. (In other words, it should solve #3852.)
@lobsterkatie
Copy link
Member

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
Copy link

This feature might have caused a regression: #5998

@lforst
Copy link
Member

lforst commented Nov 30, 2022

Let's consider this resolved for now.

@lforst lforst closed this as completed Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: nextjs Issues related to the Sentry Nextjs SDK
Projects
None yet
Development

No branches or pull requests

5 participants