Skip to content

Removing generic server instrumentation from @sentry/nextjs #6642

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

Closed
lforst opened this issue Jan 3, 2023 · 4 comments · Fixed by #6592
Closed

Removing generic server instrumentation from @sentry/nextjs #6642

lforst opened this issue Jan 3, 2023 · 4 comments · Fixed by #6592
Assignees
Labels
Package: nextjs Issues related to the Sentry Nextjs SDK Type: Improvement

Comments

@lforst
Copy link
Contributor

lforst commented Jan 3, 2023

The generic server instrumentation in the Next.js SDK contains a lot of code, complexity, and a spoooky side-effect. All of the performance and error instrumentation is already covered by our wrappers.

Upsides:

  • Way less code to maintain
  • Removes a side effect from the package
  • Allows us to simplify our isBuild function

Potential downsides:

  • We stop sending certain transactions which may trigger some things in the product (alerts, ...) (See discussion in this issue)
  • If people turn off auto-wrapping with instrumentServerFunctions: false they will not send any server-side transactions anymore
@lforst lforst added Type: Improvement Package: nextjs Issues related to the Sentry Nextjs SDK labels Jan 3, 2023
@mydea
Copy link
Member

mydea commented Jan 3, 2023

It would be great to have a (non-exhaustive) list of things that this would stop from being captured. As of now it is a bit fuzzy to me. Generally, I think this is great, and all for it, but it may be helpful for future reference to know e.g. A, B, and C are not being sent anymore. Do you have a rough idea of what these would be?

@lforst
Copy link
Contributor Author

lforst commented Jan 3, 2023

It would be great to have a (non-exhaustive) list of things that this would stop from being captured. As of now it is a bit fuzzy to me. Generally, I think this is great, and all for it, but it may be helpful for future reference to know e.g. A, B, and C are not being sent anymore. Do you have a rough idea of what these would be?

Ok after looking at it again, it doesn't seem like we are not sending any fewer transactions as long as our auto wrapping is turned on, which it is by default. I believe I confused the "unnecessary" transactions with some client spans. I updated the issue description to reflect this.

I guess the downside then is that we're not sending any server transactions in case people turn of the auto wrapping with instrumentServerFunctions: false. I think we should still go through with it and fully commit to the auto wrapping and make it work for everybody instead of maintaining these two approaches.

@Lms24
Copy link
Member

Lms24 commented Jan 4, 2023

I guess the downside then is that we're not sending any server transactions in case people turn of the auto wrapping with instrumentServerFunctions: false

Which, I guess one could argue, is what people would expect when setting this option to false, no?

@lforst
Copy link
Contributor Author

lforst commented Jan 4, 2023

I guess the downside then is that we're not sending any server transactions in case people turn off the auto wrapping with instrumentServerFunctions: false

Which, I guess one could argue, is what people would expect when setting this option to false, no?

I guess so!

As an additional note to give some more insight on in what cases users might have turned instrumentServerFunctions off: We sometimes recommended to turn it off when there is a bug on our side to unblock people. In other cases, we recommended turning it off if other libraries mess with our wrapping. And I believe early on, we recommended turning it off if you hosted your app on Vercel.

@lforst lforst self-assigned this Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: nextjs Issues related to the Sentry Nextjs SDK Type: Improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants