Skip to content

feat(nextjs): Add route handler instrumentation#8832

Merged
lforst merged 17 commits intodevelopfrom
lforst-app-router-route-handler
Aug 30, 2023
Merged

feat(nextjs): Add route handler instrumentation#8832
lforst merged 17 commits intodevelopfrom
lforst-app-router-route-handler

Conversation

@lforst
Copy link
Copy Markdown
Contributor

@lforst lforst commented Aug 17, 2023 •
edited
Loading

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
Copy Markdown
Contributor

github-actions bot commented Aug 17, 2023 •
edited
Loading

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% 🔺)

AbhiPrasad
AbhiPrasad reviewed Aug 17, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@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
Copy Markdown
Contributor 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
Copy Markdown
Contributor

AbhiPrasad 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?

@lforst
Copy link
Copy Markdown
Contributor 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
Copy Markdown
Contributor

AbhiPrasad commented Aug 17, 2023

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
Copy Markdown
Contributor 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
Copy Markdown
Contributor

AbhiPrasad commented Aug 17, 2023

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/

This was referenced Aug 21, 2023
Closed
Closed
@philwolstenholme
Copy link
Copy Markdown

philwolstenholme commented Aug 28, 2023 •
edited
Loading

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

@lforst lforst requested review from AbhiPrasad, Lms24 and mydea August 29, 2023 14:46
@lforst lforst marked this pull request as ready for review August 29, 2023 14:46
AbhiPrasad
AbhiPrasad approved these changes Aug 29, 2023
View reviewed changes
Lms24
Lms24 approved these changes Aug 30, 2023
View reviewed changes
Copy link
Copy Markdown
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:

@lforst lforst merged commit 723285f into develop Aug 30, 2023
@lforst lforst deleted the lforst-app-router-route-handler branch August 30, 2023 10:50
@edgarlr
Copy link
Copy Markdown

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
Copy Markdown
Contributor

AbhiPrasad commented Aug 30, 2023

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

@ted-marozzi ted-marozzi mentioned this pull request Sep 2, 2023
Closed
3 tasks
@ted-marozzi
Copy link
Copy Markdown

ted-marozzi commented Sep 2, 2023

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

@AbhiPrasad
Copy link
Copy Markdown
Contributor

AbhiPrasad commented Sep 5, 2023

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

Reviewers

@Lms24 Lms24 Lms24 approved these changes

@mydea mydea Awaiting requested review from mydea

+1 more reviewer

@AbhiPrasad AbhiPrasad AbhiPrasad approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Next.js 13 app dir API route handlers

6 participants

@lforst @AbhiPrasad @philwolstenholme @edgarlr @ted-marozzi @Lms24