Skip to content
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 route handler instrumentation #8832

Merged
merged 17 commits into from
Aug 30, 2023

Conversation

lforst
Copy link
Member

@lforst lforst commented Aug 17, 2023

Resolves #7228

Adds support for route handlers by:

  • Creating a new wrapping function that instruments for errors and performance
  • Automatically wrapping route handlers

This does not yet properly implement RequestData metadata. We will do this in a follow-up.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 17, 2023

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 75.28 KB (0%)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 31.25 KB (0%)
@sentry/browser - Webpack (gzipped) 21.86 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 69.8 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 28.2 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped) 20.19 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 220.32 KB (0%)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 85.13 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 59.84 KB (0%)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 31.1 KB (0%)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 75.3 KB (0%)
@sentry/react - Webpack (gzipped) 21.89 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 93.16 KB (+0.01% 🔺)
@sentry/nextjs Client - Webpack (gzipped) 50.8 KB (+0.01% 🔺)

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So does this get react server components working as well?

I wonder if we can attach react server component payloads for debugging and having some fancy parsing for them in the Sentry UI

@lforst
Copy link
Member Author

lforst commented Aug 17, 2023

So does this get react server components working as well?

RSCs have been working since a very long time

@AbhiPrasad
Copy link
Member

RSCs have been working since a very long time

I was under the impression that automatic instrumentation didn't work - do you have example transactions to look at?

@lforst
Copy link
Member Author

lforst commented Aug 17, 2023

RSCs have been working since a very long time

I was under the impression that automatic instrumentation didn't work - do you have example transactions to look at?

@AbhiPrasad
Copy link
Member

I'm a dummy - of course I helped review these PRs as well

Then we should try to see if we can RSC payload into those transaction events.

@lforst
Copy link
Member Author

lforst commented Aug 17, 2023

Then we should try to see if we can RSC payload into those transaction events.

I don't understand what you mean by RSC payload. It's just a messed up version of HTML that is returned. Are we tracking response contents in other places too?

Also, this PR has nothing to do with RSCs. This is just adding instrumentation for the app router's version of API routes.

@AbhiPrasad
Copy link
Member

I don't understand what you mean by RSC payload. It's just a messed up version of HTML that is returned. Are we tracking response contents in other places too?

RSC stuff was a complete aside - I misunderstood the scope of this PR.

I'd imagine this working like how the new GraphQL stuff works as per getsentry/sentry#50230. There has been value in this, see https://rsc-parser.vercel.app/

@philwolstenholme
Copy link

philwolstenholme commented Aug 28, 2023

Thanks @lforst and colleagues – been looking forward to this for a while!

@lforst lforst marked this pull request as ready for review August 29, 2023 14:46
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

packages/nextjs/src/common/wrapRouteHandlerWithSentry.ts Outdated Show resolved Hide resolved
@lforst lforst merged commit 723285f into develop Aug 30, 2023
45 checks passed
@lforst lforst deleted the lforst-app-router-route-handler branch August 30, 2023 10:50
@edgarlr
Copy link

edgarlr commented Aug 30, 2023

hey @lforst ! Is this fix already included in v7.66 or still is yet to be included in an upcoming release?

@AbhiPrasad
Copy link
Member

@edgarlr it'll be out with the next release!

@ted-marozzi
Copy link

I tried the beta version and got this issue: #8934, but the capturing seemed to work.

@AbhiPrasad
Copy link
Member

Fixed for above issue was released in 7.67.0!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Next.js 13 app dir API route handlers
6 participants