feat(nextjs): Add spans and route parameterization in data fetching wrappers#5564
Conversation
984e74e to
be3a2fa
Compare
lforst
left a comment
There was a problem hiding this comment.
Great work! Just left some questions for me to understand better what we're doing.
|
Can we make sure we align the operation names with https://develop.sentry.dev/sdk/performance/span-operations? If need be we can amend that spec to add new categories |
6ac67a8 to
99b1f78
Compare
size-limit report 📦
|
| origFunction: T['fn']; | ||
| context: T['context']; | ||
| route: string; | ||
| op: string; |
There was a problem hiding this comment.
Ok as for the thing Abhi mentioned: I took a look at https://develop.sentry.dev/sdk/event-payloads/span/#attributes, and https://develop.sentry.dev/sdk/performance/span-operations/.
From what's already there, it IMO makes the most sense to have something like op: "render", description: "[dataFetchingFunction]: [route]". (fwiw, "render" isn't all that wrong since the file that calls the data fetching methods in the next.js repository is literally called "render.tsx")
We could also expand the JS framework operations by something nextjs.data, or nextjs.data.getinitialprops. I don't know if we would have to involve other teams in this.
Right now I would just go for the first approach so we don't block this PR and fix stuff up later if necessary.
There was a problem hiding this comment.
Hmmm... I see your point, but given that I'm thinking we'll have spans for the actual rendering bit, I kinda feel like we should reserve that title for them. Of the examples already on our list, these data fetching methods feel most like a cross between django.middleware and django.view to me. I think your proposed names make a lot of sense, and IMHO I think we should just go for it because a) there is already precedent for individual frameworks having their own span names, and b) this is still behind a flag, so the stakes are low.
There was a problem hiding this comment.
yup, and we can always change it if product requires us to.
bb57072 to
01300bd
Compare
01300bd to
84cd837
Compare
This adds span creation and route parameterization to the experimental
getStaticPropsandgetServerSidePropswrappers. 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 thedataFetchersLoaderand 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):
Linkcomponent 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.withSentryinto these wrappers also.(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: #5505