Improve Nextjs tracing #5505
Assignees
Labels
Comments
10 tasks
This was referenced Aug 13, 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
This was referenced Sep 8, 2022
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
|
As for
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 |
This was referenced Sep 21, 2022
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.)
|
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. |
This was referenced Oct 10, 2022
|
This feature might have caused a regression: #5998 |
|
Let's consider this resolved for now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Goal: Improve tracing in nextjs SDK - make parameterization more reliable, trace data fetchers, trace page requests for folks on Vercel
TL;DR Plan:
Effects:
instrumentServer(brittle monkeypatching, only works off of vercel)Open Questions:
Tasks
Prework
General prework necessary to improve the Next.js performance experience as a whole.
RewriteFrames(ref(nextjs): Use loader to setRewriteFrameshelper value #5445)getServerSideProps,getStaticPropsandgetStaticPathsduring build-time (ref(nextjs): Wrap server-side data-fetching methods during build #5503)getStaticPaths(fix(nextjs): Remove experimental wrapping ofgetStaticPaths#5561)Transaction Name Parameterization
getServerSidePropsandgetStaticProps(feat(nextjs): Add spans and route parameterization in data fetching wrappers #5564)getInitialPropsfor normal pages (not_app.js,_error.js,_document.js) (feat(nextjs): Create spans and route parameterization in server-sidegetInitialProps#5587)getInitialPropsin_app.js,_error.jsand_document.js(feat(nextjs): Instrument server-sidegetInitialPropsof_app,_documentand_error#5604)Connected Traces
redirectandnotFoundresponses fromgetServerSideProps(alsogetStaticProps?)) (feat(nextjs): Connect trace between data-fetching methods and pageload #5655)isPrefetchRequest: booleantag to errors and do the one or more of the following to transactions 1. different name 2. data field 3. different op) (there is apurpose: prefetchheader in those requests)getServerSidePropspage that throws: Transaction name is/_errorbut DSC contains route ofgetServerSideProps. (SeeTODOcomment in src/performance/client.ts) ([nextjs] Fix transaction name getting lost when hitting_errorpage #5826)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.
getInitialPropsandgetServerSideProps(feat(nextjs): Create transactions ingetInitialPropsandgetServerSideProps#5593)RequestDataintegration to work for error events (ref(nextjs): UseRequestDataintegration for errors #5729)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?)withSentryinto a wrapper/helper)res.sendsimilar to how we do it inwithSentry(ref(nextjs): Use flush code fromwithSentryin all backend wrappers #5814)Figure out how to propagate scope between data-fetching functions in the same transaction (ordocument scope-propagation limitations)instrumentServer.ts?statusto transactions and spans and update to "internal_error" on error (feat(nextjs): Add status to data-fetcher spans #5777)What happens for pages with no data fetchers? Should_apphave a different helper? (no name for transaction)Folow-up/Cleanup/Polishing
package.json(chore(nextjs): Remove obsoletedataFetchersloader #5713)RequestDataintegration everywhereCheck if new model works with CJS, if no, support people that use CJSwithSentryinto the fold (consolidate helpers)Documentation
RequestDataintegration and optionsMisc (stretch goals)
getInitialPropswhen run client-sideSteps for Beta
autoInstrumentServerFunctionssentry-docs#5542)withSentryfunction inwith-sentryexample vercel/next.js#41326)Steps for GA (Planned date: October 5, 2022)
autoInstrumentServerFunctionsper default #5919)withSentry(Remove references to manually usewithSentryfrom Next.js docs sentry-docs#5543)Follow ups
Undone tasks from above have been sorted into:
withSentryand possiblyinstrumentServer(JS SDK v8 Deprecations List #5194)_errorbetter ([nextjs] Fix transaction name getting lost when hitting_errorpage #5826)RequestDataintegration (RequestData integration prework, work, and postwork #5756)The text was updated successfully, but these errors were encountered: