-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(nextjs): Add spans and route parameterization in data fetching wrappers #5564
feat(nextjs): Add spans and route parameterization in data fetching wrappers #5564
Conversation
984e74e
to
be3a2fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, and we can always change it if product requires us to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
bb57072
to
01300bd
Compare
01300bd
to
84cd837
Compare
This adds span creation and route parameterization to the experimental
getStaticProps
andgetServerSideProps
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 thedataFetchersLoader
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):
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.withSentry
into 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