ref(nextjs): UseRequestData integration for errors

[lobsterkatie]

In #5703, a new integration, RequestData, was added to take the place of the request-specific event processors we've been using to add request data to transaction events in the nextjs SDK. This builds on that work by making the same switch for error events.

Notes:

• Because of Clean up code relating to scope/event transaction value  #5718, it's hard to reason about the potential side effects of making major changes to the logic in @sentry/utils/requestData. Therefore, the majority of the new logic in this PR has been added to the integration, and just overwrites the transaction value added by the functions in requestData. Once we've cleaned up the request data code, we can consolidate the logic.

[lforst]

It just took me 20 minutes to figure out why it is safe to remove the transaction: false logic here. I believe what confused me since we don't have the include.transaction defaulted to false in the requestdata integration. I think we should add that default just so it's more explicit.

Also (for a lack of better place to comment) there are still some TODO comments in addRequestDataToEvent in requestdata.ts referencing Next.js. Can we remove those now?

[lobsterkatie]

It just took me 20 minutes to figure out why it is safe to remove the transaction: false logic here. I believe what confused me since we don't have the include.transaction defaulted to false in the requestdata integration. I think we should add that default just so it's more explicit.

include.transaction isn't a thing anymore, at least not for the integration. From the description of #5703:

• The options API for the new integration is inspired by, but different from, the options API for our Express request handler.
  • 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.

Regardless, we didn't actually need that setting in the first place, at least not for transaction events. Even if include.transaction is set to true, transaction events will never fall into the if because they always already have a transaction value:

sentry-javascript/packages/node/src/requestdata.ts

Line 286 in 17e4cb8

if (include.transaction && !event.transaction) {

So that handles transaction events. As for error events, as of this PR it doesn't matter what transaction value addRequestDataToTransaction sets, because we're overwriting it in the integration.

(And sorry - I don't mean that to sound dismissive. It's a valid concern, just not one that happens to apply here.)

Also (for a lack of better place to comment) there are still some TODO comments in addRequestDataToEvent in requestdata.ts referencing Next.js. Can we remove those now?

Probably. I'll give them a look.

[lforst]

Ah right. I now remember why I set transaction: false in the first place - it was because the errors originating from data fetchers had the unparameterized route in a tag - which now seems to be the case again and I'm not sure this is a good thing, since the transaction tag now mismatches the actual transaction (Do we even need to be exact here though? I guess it might be confusing to users):

Example - here the transaction tag is GET /parameterized-page-with-getServerSideProps/two while it should be /parameterized-page-with-getServerSideProps/[pageNum]

(event (after), event (before))

I guess both are sort of correct, I am just wondering which one is better. Do you have an opinion here?

[lforst]

There also seem to be events where the tag is the request url. Not sure if this is related to this change though:

event

[lobsterkatie]

Huh. In my testing, the tag was always overwritten with the transaction value, even when I specifically set it to be something different in beforeSend. The "after" event is from this branch?

(Also, there's no reason we can't just fix the tag, too, just to be certain.)

UPDATE: In your event(after), event(before) links, one thing I notice is that the before event doesn't have a transaction value at all (only a tag). Lemme play around with this a little more.

UPDATE2: Damn. Rebase fail. 🤦🏻‍♀️ An old bug crept back in - wrong naming scheme for the link to the transaction on the request. Now I want to go back and make sure nothing else regressed...

On the transaction score, I think we should be good now, though:

[lforst]

Yeah before is some event before this PR and after is after checking out the PR.

Are we sure we want to have the GET in the transaction tag though? Since there will be a mismatch between actual transaction and transaction tag.

[lobsterkatie]

Yup, discovered that as well. I think I managed to drop a fixup commit somewhere in there, which both fixed how the transaction is retrieved and the check for whether or not to include the method for nextjs events.

For nextjs, to make it match current behavior, the answer is yes for API routes, no for page routes. I'm proposing that in v8 we should just standardize on always including the method. (We already include it in our express transaction names, for example.)

there will be a mismatch between actual transaction and transaction tag.

From my testing, I believe the only way this can happen (in the UI, at least) is for an event to come in with a transaction tag but no top-level transaction value (like your "before" event). If an outgoing event has both a top-level transaction value and a transaction tag, and they don't match, ingest seems to overwrite the tag value with the top-level value. For example, this beforeSend

beforeSend: (event) => {
  event.transaction = "aardvark";
  event.tags.transaction = "zebra";
  return event;
}

produced an event with this JSON:

[lforst]

Gut feeling wise I like the current behavior (yes for API routes, no for page routes), because that way the pageload/pagenavigation transaction names and data-fetcher transactions line up. But let's discuss this in-depth when we get to it!

[lobsterkatie]

Yeah, I'm not wedded to either outcome, I just think we should standardize it across SDKs if possible. The tricky part when it comes to something like Express is that it's not as clear-cut what's an API request and what's a page load request. But as you say, we can talk about it closer to the time.