-
Notifications
You must be signed in to change notification settings - Fork 6.5k
chore(open-next): add server-side sentry to open-next site #8830
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
base: main
Are you sure you want to change the base?
Changes from all commits
b7b2263
0f161be
3c40dba
16eb479
3d76a43
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| // Note: this custom worker-entrypoint is used so that the worker can include sentry support | ||
| // and it has been written by following: | ||
| // - the official open-next docs: https://opennext.js.org/cloudflare/howtos/custom-worker | ||
| // - the official sentry docs: https://docs.sentry.io/platforms/javascript/guides/cloudflare | ||
|
|
||
| import { withSentry } from '@sentry/cloudflare'; | ||
|
|
||
| import type { ExecutionContext } from '@cloudflare/workers-types'; | ||
|
|
||
| import { default as handler } from '../.open-next/worker.js'; | ||
|
|
||
| export default withSentry( | ||
| (env: { | ||
| /** | ||
| * Sentry DSN, used for error monitoring | ||
| * If missing, Sentry isn't used | ||
| */ | ||
| SENTRY_DSN?: string; | ||
| }) => ({ | ||
| dsn: env.SENTRY_DSN, | ||
|
flakey5 marked this conversation as resolved.
|
||
| // Adds request headers and IP for users, for more info visit: | ||
| // https://docs.sentry.io/platforms/javascript/guides/cloudflare/configuration/options/#sendDefaultPii | ||
| sendDefaultPii: true, | ||
|
ovflowd marked this conversation as resolved.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dario-piotrowicz disable this please
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mh... shall we disable it just to get sentry integrated and possibly re-enable it later when/if we want? 🤔 |
||
| // Enable logs to be sent to Sentry | ||
| enableLogs: true, | ||
| // Set tracesSampleRate to 0.05 to capture 5% of spans for tracing. | ||
| // Learn more at | ||
| // https://docs.sentry.io/platforms/javascript/guides/cloudflare/configuration/options/#tracesSampleRate | ||
| tracesSampleRate: 0.05, | ||
| }), | ||
| { | ||
| async fetch( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this fetch override, ooc? 👀
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This fetch calls the actual open-next worker |
||
| request: Request, | ||
| env: Record<string, unknown>, | ||
| ctx: ExecutionContext | ||
| ) { | ||
| return handler.fetch(request, env, ctx); | ||
| }, | ||
| } | ||
| ); | ||
|
|
||
| export { DOQueueHandler } from '../.open-next/worker.js'; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -107,7 +107,9 @@ | |
| "typescript": "catalog:", | ||
| "typescript-eslint": "~8.57.2", | ||
| "user-agent-data-types": "0.4.2", | ||
| "wrangler": "^4.77.0" | ||
| "wrangler": "^4.77.0", | ||
| "@cloudflare/workers-types": "^4.20260418.1", | ||
| "@sentry/cloudflare": "^10.49.0" | ||
|
Comment on lines
+110
to
+112
This comment was marked as low quality.
Sorry, something went wrong.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder how much this bloats our dependencies. I wonder if we should have a specific package for our OpenNext implementation on this monorepo. So that we move all the cloudflare specifics to a specific apps/site-cloudflare or something like that? This would reduce the need for local development and even other environments to install and pull and test all things unrelated to the default installation of the website? wdyt @dario-piotrowicz @nodejs/web-infra?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't imagine the extra deps being an issue for local development 🤔 Also even if they were in their specific package, as long as they were in the monorepo they'd still be pulled down/installed on |
||
| }, | ||
| "imports": { | ||
| "#site/*": [ | ||
|
|
||
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.
@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)
Uh oh!
There was an error while loading. Please reload this page.
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.
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.
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 don't see settings set for the release worker: https://github.com/nodejs/release-cloudflare-worker/blob/main/src/worker.ts 👀 am I looking in the wrong place? or do you mean those
Sentry.setTags?