-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(cloudflare): Add option to opt out of capturing errors in wrapRequestHandler
#16852
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(cloudflare): Add option to opt out of capturing errors in wrapRequestHandler
#16852
Conversation
wrapRequesHandler
wrapRequestHandler
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.
Code LGTM, I was just wondering if higher-level SDKs can then capture these errors within the right scope, or if there could be any issue around that 😅.
Very good question because it made me double-check the implementation, thanks! Fortunately, in the SvelteKit case, the scope data is still kept because the order of scope forking is correct and we capture errors in an inner scope of the CF wrapper: // hooks.server.ts
export const handle = sequence(
initCloudflareSentryHandle({
dsn: '...',
}), // <-- inits SDK, creates isolation scope, sets CF data, but no longer captures error
sentryHandle(), // <-- sets SvelteKit data AND captures error (correctly)
redirectToSentryPage // <-- user-created middleware(s) that could throw
// other SvelteKit lifecycle APIs like `load` functions that could throw are executed now
// finally, we exit sentryHandle() and then initCloudflareSentryHandle() middlewares
); @s1gr1d, since your working on Nuxt on CF: Do you use |
Should be fine in Nuxt as the handler has its own isolation scope sentry-javascript/packages/nuxt/src/runtime/plugins/sentry-cloudflare.server.ts Lines 119 to 138 in b746c23
|
…flare (#16853) We want to avoid capturing errors too early and too generally via the `initCloudflareSentryHandle` request handler. Internally, this handler uses a wrapper from `@sentry/cloudflare` to capture errors but prior to #16852 it captured everything that was thrown. This included thrown `redirect()` objects from SvelteKit which serve as control flow mechanisms but should not be captured as errors. This patch opts out of capturing errors in the Cloudflare wrapper. Instead, we rely on our already existing error capturing mechanisms in SvelteKit, which already ignore `redirect()` and a few other error classes.
This PR adds a
captureErrors
option to thewrapRequestHandler
options from@sentry/cloudflare
. While by default, the wrapper should capture exceptions (for example, when it's used in pure CF worker functions), it shouldn't do that when it's used in higher-level SDKs, like SvelteKit. There, we want our other error capturing implementations to do the job.This came up because as reported in #16847, our CF wrapper caught thrown
redirect(...)
objects which we deliberately ignore in our SvelteKit SDK (they're a control flow mechanism in Kit and not an actual error).In a follow-up PR, I'll set this option in the SvelteKit SDK to
false
.If reviewers prefer, I can change the option to be a callback where we'd pass in the error and decide on capturing based on a returned
boolean
. I initially decided against it because it adds more complexity than necessary but happy to change.