ref(nextjs): Use integration to add request data to transaction events

[lobsterkatie]

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 Clean up code relating to scope/event transaction value  #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.

• 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

TODO:

• Add note to changelog about new integration (will wait until I'm sure which package it's living in)
• Add docs (will wait until I'm sure which package it's living in)
• Respond in withSentry in nextjs can not be configured to scrub cookies (sensitive data) #4723 (this will have to wait until we use the integration for errors also)
• Add tests if not already covered

[github-actions]

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.51 KB (+0.01% 🔺)
@sentry/browser - ES5 CDN Bundle (minified)	60.26 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.1 KB (-0.02% 🔽)
@sentry/browser - ES6 CDN Bundle (minified)	53.18 KB (0%)
@sentry/browser - Webpack (gzipped + minified)	19.89 KB (0%)
@sentry/browser - Webpack (minified)	64.59 KB (0%)
@sentry/react - Webpack (gzipped + minified)	19.92 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified)	44.83 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	26.03 KB (-0.01% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	24.42 KB (0%)

[AbhiPrasad]

Why not make RequestData live in @sentry/node?

[lobsterkatie]

Why not make RequestData live in @sentry/node?

My thinking was that it should live in @sentry/integrations for the same reason that addReqeustDataToEvent lives in @sentry/utils rather than @sentry/node - isomorphic frameworks like nextjs have places where you have to import from a platform-neutral package (like the _error helper function here). That said, it may be that by switching it to an integration, I'm able to restrict its usage to node-specific locations (like index.server.ts), and therefore could move it to @sentry/node. For nextjs specifically, I actually do think that would be okay.

I wonder if there might not be other isomorphic situations in other frameworks (current or future) which would be more like the _error case, though. D'ya think? Or will it always be the case that we'll be setting up default integrations in the same spot where we're calling init, and therefore (given that init is always going to be platoform-specific) we'll always be safe with a platform-specific import for the integration? Now that I'm thinking about it, I think that might be true...

Any 🦆 thoughts here?

UPDATE: As of our DACI meeting earlier today (about recording failed http requests as errors), this question just got a bit more complicated, because we'll need request data there, too. Let's discuss this at the next standup.

FURTHER UPDATE: We decided to move it to @sentry/node for now. I've updated the PR description.

[lforst]

Rant alert. Just want to get the following out for my own sanity - no ill intentions:

I don't have any actionable feedback here but these lines of code make me question whether it is a good idea for us to internally use integrations at all. This whole addOrUpdateBlah api + passing down integrations and having to deal with them competing with eachother is so weird to me. I feel like we're more often fighting integrations than using them. In my perfect world, anything that is default should just be init options. Only extra non-default functionality should be done via integrations.

[lobsterkatie]

Yeah, this addOrUpdate business is a little weird, I'll admit, though it predates me. I actually have most of a solution to at least make that part cleaner, but it would mean making core code have more bytes just to accommodate something which only the nextjs SDK needs, so I gave up on it.

As for the rest, I wasn't there when the unified SDK API was specced out, but I think the reasoning is something along the lines of "keeping SDK-specific default behavior in integrations allows the init options to be more standardized across SDKs" (which is the ultimate goal of the the unified API).

[lforst]

Any reason why we can't define those variables for each test individually? That would keep side-effects between tests at a minimum.

[lobsterkatie]

Tests run serially within a given test file I believe, though, right? So resetting them before each test should in theory prevent any side effects. (And doing it this way saves having the boilerplate at the beginning of every test. On that topic, I actually tried doing this as a test.each but IIRC I couldn't get the types to work correctly.)

[lforst]

Yeah but why depend on jest behavior when you can simply make something be scoped to a single test? It is also easier to read and debug imo since you don't have to jump around to a globally defined variable and check whether it's overwritten at some point by some other test or if it is reset on some afterEach hook.

I don't want to turn these tests into test.each, I just want to remove the mutability of test inputs. I am not so worried about this particular file since it is very small but I'd like to avoid this pattern in other places. That's why I am bringing it up here.

[lforst]

I looked at this type and something didn't feel right to me about PolymorphicRequest being an intersection instead of a union so I opened an issue to discuss it: #5768

Of course that doesn't block this PR but just wanted to note it down here for posterity.

[lforst]

Wdyt about removing this non-null-assertion and making turning the type of _options into Required<RequestDataOptions>? The constructor options are still optional that way. Just want to get rid of as many places where we disable the type checker as we can.

[lobsterkatie]

I decided to go the other way (make everything required and have the constructor input be a partial) because it seems every-so-slightly more correct to me, but it still gets rid of the non-null assertion. That said, if we're worried about turning off the linter in this situation, maybe we should talk about whether we actually want to lint against ! at all. After all, we don't lint against as typecasts (because we assume that if you're doing it, you're doing it for a good reason), and they're just a different way of saying the same thing.

[lforst]

No need to change it now but I would have intentionally made it Required since it's pretty unusual to have user-facing APIs that are Optional. Doing it the other way would have kept the user-facing type simple to parse mentally.