chore(open-next): add server-side sentry to open-next site#8830
chore(open-next): add server-side sentry to open-next site#8830dario-piotrowicz wants to merge 3 commits intonodejs:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview Updates Wrangler to use this entrypoint as Reviewed by Cursor Bugbot for commit 3c40dba. Bugbot is set up for automated code reviews on this repo. Configure here. |
👋 Codeowner Review RequestThe following codeowners have been identified for the changed files: Team reviewers: @nodejs/nodejs-website @nodejs/web-infra Please review the changes when you have a chance. Thank you! 🙏 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8830 +/- ##
=======================================
Coverage 73.88% 73.88%
=======================================
Files 105 105
Lines 8889 8889
Branches 326 326
=======================================
Hits 6568 6568
Misses 2320 2320
Partials 1 1 ☔ View full report in Codecov by Sentry. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| export default Sentry.withSentry( | ||
| () => ({ | ||
| dsn: 'https://examplePublicKey@o0.ingest.sentry.io/0', |
There was a problem hiding this comment.
Placeholder Sentry DSN makes monitoring non-functional
High Severity
The dsn is set to the example placeholder 'https://examplePublicKey@o0.ingest.sentry.io/0' from the Sentry docs, which won't deliver events to any real Sentry project. Additionally, the withSentry config callback uses () => ({...}) instead of (env) => ({...}) — the env parameter is how the Sentry Cloudflare SDK provides access to worker environment bindings, which is the standard way to read env.SENTRY_DSN at runtime. Without both a valid DSN and the env parameter, the entire Sentry integration this PR adds is non-functional.
Reviewed by Cursor Bugbot for commit b7b2263. Configure here.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Dario Piotrowicz <dario.piotrowicz@gmail.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0f161be. Configure here.
| // Set tracesSampleRate to 1.0 to capture 100% of spans for tracing. | ||
| // Learn more at | ||
| // https://docs.sentry.io/platforms/javascript/guides/cloudflare/configuration/options/#tracesSampleRate | ||
| tracesSampleRate: 1.0, |
There was a problem hiding this comment.
Full trace sampling rate unsuitable for production traffic
Medium Severity
tracesSampleRate: 1.0 instruments 100% of requests. For a high-traffic production site like nodejs.org, this adds latency to every request and can generate massive Sentry billing costs. Sentry's own docs recommend a lower value (e.g., 0.2) for production. This appears to be copy-pasted boilerplate from Sentry's getting-started guide and is likely to be overlooked when the placeholder DSN is replaced with a real one.
Reviewed by Cursor Bugbot for commit 0f161be. Configure here.
ovflowd
left a comment
ovflowd
left a comment
There was a problem hiding this comment.
Why are we doing this? Where the need of adding Sentry comes from?
|
@dario-piotrowicz please don't use Copilot. We're using Cursor on this repository for now :) |
oh... I actually didn't do anything (at least I don't think so!)... Copilot just automatically reviewed the PR... 😓 |
No? You explicitly requested a review from it 
|
|
If that's not the case, I'd recommend you to review your Copilot settings, IDK 😅 |
|
no, I didn't... 😕 😓 🤷 |
|
Maybe this was the problem 🤷 I've disabled it now 😅👍 |
| dsn: 'https://examplePublicKey@o0.ingest.sentry.io/0', | ||
| // Adds request headers and IP for users, for more info visit: | ||
| // https://docs.sentry.io/platforms/javascript/guides/cloudflare/configuration/options/#sendDefaultPii | ||
| sendDefaultPii: true, |
There was a problem hiding this comment.
Why do we need to disable this? This is pretty important for allowing Sentry to properly tell us the scope of an issue in terms of user impact etc.
There was a problem hiding this comment.
@dario-piotrowicz we had Sentry in the past with good chunk of customization, could you please go through GitHub history and check it out (just to compare what we had at the time)
There was a problem hiding this comment.
We should replicate the settings that we use on the release worker, as we know those settings work for us. I don't think historical settings from the Next.js site make much sense as a reference point, given this is Worker instrumentation.
| tracesSampleRate: 1.0, | ||
| }), | ||
| { | ||
| async fetch( |
There was a problem hiding this comment.
What is this fetch override, ooc? 👀
There was a problem hiding this comment.
This fetch calls the actual open-next worker
Description
This PR adds server-side sentry to the open-next site.
Validation
To be done.
Check List
pnpm formatto ensure the code follows the style guide.pnpm testto check if all tests are passing.pnpm buildto check if the website builds without errors.