-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 information about enable next instrumentation #11140
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
Bundle ReportChanges will decrease total bundle size by 15 bytes ⬇️
|
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.
Added some suggestions
To set up this file, enable the Next.js instrumentation hook by setting the [`experimental.instrumentationHook`](https://nextjs.org/docs/app/api-reference/next-config-js/instrumentationHook) to `true` in your `next.config.js`. | ||
Then add a `instrumentation.ts` file to the root directory of your Next.js application (or inside the `src` folder if you're using one) and add the following content: |
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.
To set up this file, enable the Next.js instrumentation hook by setting the [`experimental.instrumentationHook`](https://nextjs.org/docs/app/api-reference/next-config-js/instrumentationHook) to `true` in your `next.config.js`. | |
Then add a `instrumentation.ts` file to the root directory of your Next.js application (or inside the `src` folder if you're using one) and add the following content: | |
To use this instrumentation, you need to explicitly enable the Next.js instrumentation hook in your `next.config.js` file by setting [`experimental.instrumentationHook`](https://nextjs.org/docs/app/api-reference/next-config-js/instrumentationHook) to `true`. | |
Then, create an `instrumentation.ts` file in the root directory of your Next.js application (or inside the `src` folder, if you're using one) and add the following content: |
@@ -144,7 +144,8 @@ We recommend you include your DSN directly in these three files. Alternatively y | |||
|
|||
While the client initialization code will be injected into your application's client bundle by `withSentryConfig` which we set up earlier, | |||
the configuration for the server and edge runtime needs to be imported from a [Next.js Instrumentation file](https://nextjs.org/docs/app/building-your-application/optimizing/instrumentation). | |||
To set up this file, add a `instrumentation.ts` file to the root directory of your Next.js application (or inside the `src` folder if you're using one) and add the following content: | |||
To set up this file, enable the Next.js instrumentation hook by setting the [`experimental.instrumentationHook`](https://nextjs.org/docs/app/api-reference/next-config-js/instrumentationHook) to `true` in your `next.config.js`. |
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.
Since this is required, can you also update the code snippet for next.config.js
to include this? Otherwise I feel like it's still easy to miss for a user just skimming & copy-pasting the code samples
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.
It is technically not required, which is why I am also a bit on the fence about this change. Something is fishy with the user report.
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.
Should we add this then? I think it is a little bit more transparent if the users set this option themselves. But if the SDK enables instrumentationHook
we could also document that this is enabled for them? Just for some context on what's going on in the background.
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.
I say we just merge this as is and all is fine.
DESCRIBE YOUR PR
Adding more information about enabling
instrumentationHook
first. Based on this comment: getsentry/sentry-javascript#12044 (comment)IS YOUR CHANGE URGENT?
Help us prioritize incoming PRs by letting us know when the change needs to go live.
SLA
Thanks in advance for your help!
PRE-MERGE CHECKLIST
Make sure you've checked the following before merging your changes: