feat(nextjs): Create transactions in getInitialProps and getServerSideProps

[lforst]

Ref: #5505

Creates transactions in the getInitialProps and getServerSideProps instrumentations. Additionally puts all the data fetching methods into their own domain so that any thrown errors get attributed to the right data fetching method spans instead of the overarching transaction.

Out of scope:

• The getStaticProps method has a very different signature than the other methods so this function is out of scope for this PR.
• Extracting trace data from req
• Adding trace data to props that were returned from data fetchers
• Proper error instrumentation (for now, simply introduced a wrapper that collects errors)
• Doing some sort of blocking until sending response (for serverless environments)

[lobsterkatie]

Looks good! 🙂

I know we talked about this a bit in slack, but IMHO a lot of what's in the individual wrappers could/should be brought into the main helper function, specifically the code splitting out the req and res (since they're already part of the args object you're passing), the error handling, and the tracing-enablement check, since all three are the same in both places.

Other than that, mostly just nits.

[lobsterkatie]

Hmmm... What happens if multiple data-fetching functions for the same request are running at the same time? (Not a current concern, but it will be one eventually, and I figure we should future-proof ourselves as much as we can.)

[lforst]

I think we're good in this situation. Since we have an individual domain for each of the data fetchers, they also have their own hub/cloned scope, so setting the span should "just work". Let me know if I misunderstand something.

[lobsterkatie]

Since we have an individual domain for each of the data fetchers

Yeah, okay, I see what you're saying. I think I'm stuck in the mindset which applies in our other framework integrations, where it's one domain/request. I'm trying to think if there's any reason we'd need to have all of a request's data fetchers live in the same domain. I guess the question is, is there anything which happens to the scope in one which we'd want to be persisted to another?

Thoughts:

• We don't know the order in which the data fetchers will run, so even if they were all running in the same domain, we can't count on one changing the scope in a way that effects the others, because it might not run first.
• In a way, we're treating the request like a persisted scope, in that we're attaching the transaction to it (the one thing we do need to be the same for all data fetchers).
• I don't think (?) there's anything else we automatically do to the scope which we wouldn't want to lose.

So I think the answer is no, because we don't know the order in which they'll run, so even if they were in the same domain, we couldn't count on one setting data that the next one could use. The one exception here is the event processor to add request data. That's gotta be attached to whatever scope is active when we call transaction.finish. Off-the-cuff idea: In addition to attaching the transaction to the request, we should also store a reference to the request in the transaction metadata. Then we can add an event processor at SDK startup which grabs the request out of sdkProcessingMetadata and uses it to populate request data into the event.

[lforst]

I spent the night thinking about if there is a really good reason for only having one domain across all data fetchers, since having only one domain has its drawbacks with error association. So far I've not come up with anything. I also believe users in any case already have the mental model that data fetchers are more or less isolated and do not share data between them. If bad comes to worst we can explain this behavior in the docs.

As for the request data processor, it turned out to be even simpler. transaction.finish() is still called within the individual data fetcher's scope, so adding the event processor inside callTracedServerSideDataFetcher just works: 3141759 👍

[lobsterkatie]

Oh, one other thought: We guard instrumentServer with an isBuild() check, so we don't check the phase when we're starting a transaction, but we probably should do so here, since this wrapping will be baked in.

[lforst]

Oh, one other thought: We guard instrumentServer with an isBuild() check, so we don't check the phase when we're starting a transaction, but we probably should do so here, since this wrapping will be baked in.

@lobsterkatie I'm not entirely sure what you mean by this. I added a check with isBuild in the wrappers and if it's during build it just returns the normal one. Is this what you meant? d7fec7e

[lforst]

I know we talked about this a bit in slack, but IMHO a lot of what's in the individual wrappers could/should be brought into the main helper function, specifically, the code splitting out the req and res (since they're already part of the args object you're passing), the error handling, and the tracing-enablement check, since all three are the same in both places.

Idk single responsibility of functions and all. A little bit of repeating doesn't hurt anyone and unifying things later on is way way easier than pulling them apart.

A perfect example of why we shouldn't prematurely DRY everything up is #5604. Since res and req are located in a nested object in _app's getInitialProps, we couldn't have extracted these objects with a generic wrapper (at least without some weird logic).

I really wanna urge us not to immediately create helpers for everything until after we explored a topic enough to identify that there is too much of the same code everywhere.

[github-actions]

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.4 KB (-0.02% 🔽)
@sentry/browser - ES5 CDN Bundle (minified)	60.06 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	17.98 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified)	52.92 KB (0%)
@sentry/browser - Webpack (gzipped + minified)	19.77 KB (0%)
@sentry/browser - Webpack (minified)	64.31 KB (0%)
@sentry/react - Webpack (gzipped + minified)	19.79 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified)	44.7 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	25.9 KB (+0.01% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	24.27 KB (-0.01% 🔽)

[lobsterkatie]

Oh, one other thought: We guard instrumentServer with an isBuild() check, so we don't check the phase when we're starting a transaction, but we probably should do so here, since this wrapping will be baked in.

@lobsterkatie I'm not entirely sure what you mean by this. I added a check with isBuild in the wrappers and if it's during build it just returns the normal one. Is this what you meant? d7fec7e

No. Sorry for being unclear. You're right, I didn't explain that well. What you did serves the same purpose, so if you prefer it to what I'm about to say, great, but I actually just meant that before starting a transaction the check should be not if (hasTracingEnabled()) but if (!isBuild() && hasTracingEnabled()).

[lobsterkatie]

unifying things later on is way way easier than pulling them apart.

All right - let's get everything working and then we can revisit.