Skip to content
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(browser): Mark errors caught from TryCatch as unhandled #8890

Merged
merged 3 commits into from
Sep 4, 2023

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Aug 29, 2023

This PR is the first of a series of PRs which adjust the exception mechanism to be set to handled: false whenever our SDK-internal instrumentation catches an error. I decided to split these changes up by SDK package to revert specific changes more easily, and more clearly show which change has which consequences (e.g. on tests).

Right now, we sometimes set handled: true which often incorrectly classifies actually unhandled errors (from the perspective of a user) as handled. Consequently, users who triage Sentry issue based on the exception mechanism might triage an actually unhandled error as handled.

The goal of fixing handled/unhandled is that all errors caught by us are marked as unhandled while user-invoked calls (e.g. Sentry.captureException are continued to be marked as handled).

Changed Instrumentation

This PR addresses the mechanisms set in the browser package. Most of our instrumentation in the browser SDK already sets the mechanism correctly. Only the TryCatch integration required adjustments.

Consequences of this change

While the change itself is simple, there are SDK- and product-wide consequences to it:

  • Sessions in which errors occurred that were incorrectly given handled: true, previously finished with status 'ok'. These sessions now finish with status: 'crashed'.
  • The increase in "crashed" sessions potentially leads to a decreased release health in users' releases.
  • We cannot say for sure that users didn't handle errors that we now mark as unhandled. So while before we produced false positives (in the sense of classifying an exception as handled), we now create false negatives. To be clear, we had false positives and negatives before and will continue to have both with this change. It might just shift a little into the other direction.

The concept of a "crashed session" doesn't make much sense in browser world, given that the browser very rarely actually crashes. This is very well explained in #6073 (comment). As we all know, release health therefore doesn't make too much sense for browser-based applications.

In a nutshell, these PRs address #6073 with a pragmatic approach of at least uniformly setting the "correct" handled status. These PRs do not completely address the linked RFC or gathering more data around decisions first.

ref #6073

@github-actions
Copy link
Contributor

github-actions bot commented Aug 29, 2023

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 75.22 KB (-0.01% 🔽)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 31.19 KB (-0.04% 🔽)
@sentry/browser - Webpack (gzipped) 21.85 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 69.76 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 28.22 KB (+0.03% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped) 20.18 KB (-0.04% 🔽)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 220.07 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 84.89 KB (+0.02% 🔺)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 59.88 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 31.12 KB (+0.06% 🔺)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 75.24 KB (-0.01% 🔽)
@sentry/react - Webpack (gzipped) 21.88 KB (+0.02% 🔺)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 93.04 KB (0%)
@sentry/nextjs Client - Webpack (gzipped) 50.75 KB (+0.03% 🔺)

@Lms24 Lms24 self-assigned this Aug 29, 2023
@Lms24 Lms24 marked this pull request as ready for review August 29, 2023 12:50
@Lms24 Lms24 requested review from mydea and AbhiPrasad August 29, 2023 12:50
@Lms24
Copy link
Member Author

Lms24 commented Aug 29, 2023

Discussed in Daily: We'll label these PRs as feat to highlight the importance of this change

@Lms24 Lms24 changed the title fix(browser): Mark errors caught from TryCatch integration as unhandled feat(browser): Mark errors caught from TryCatch integration as unhandled Aug 29, 2023
@Lms24 Lms24 changed the title feat(browser): Mark errors caught from TryCatch integration as unhandled feat(browser): Mark errors caught from Browser SDK as unhandled Aug 29, 2023
@Lms24 Lms24 changed the title feat(browser): Mark errors caught from Browser SDK as unhandled feat(browser): Mark errors caught from TryCatch as unhandled Aug 29, 2023
@Lms24 Lms24 merged commit 726a6ad into develop Sep 4, 2023
@Lms24 Lms24 deleted the lms/fix-handled-unhandled-browser branch September 4, 2023 08:55
Lms24 added a commit that referenced this pull request Sep 4, 2023
Report exceptions caught by our Vue error handler and routing instrumentation as unhandled.

Detailed write up in #8890
Lms24 added a commit that referenced this pull request Sep 4, 2023
…led (#8907)

Mark errors caught in

* AWS Lambda handler
* GCP http, function, cloudEvent handlers

as unhandled

For more details see #8890
Lms24 added a commit that referenced this pull request Sep 4, 2023
Lms24 added a commit that referenced this pull request Sep 4, 2023
…8893)

Mark errors caught in

* `captureUnderscoreException` 
* `wrapApiHandlerWithSentry`
* `callDataFetcherTraced`
* `withErrorInstrumentation`
* `wrapServerComponentWithSentry`

as unhandled.

For more details, see #8890, #6073

Co-authored-by: Luca Forstner <luca.forstner@sentry.io>
Lms24 added a commit that referenced this pull request Sep 4, 2023
…ed (#8894)

Mark errors caught from remix instrumentation as unhandled:
* `captureRemixErrorBoundaryError` 
* `captureRemixServerException`

For more details see #8890, #6073
Lms24 added a commit that referenced this pull request Sep 5, 2023
…Console` integrations as unhandled (#8891)

Mark errors caught from optional integrations

* `HttpClient`
* `CaptureConsole`

as unhandled. 

For more details see #8890, #6073
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants