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

Support custom Sentry.runZoneGuarded zone creation #2088

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

buenaflor
Copy link
Contributor

@buenaflor buenaflor commented Jun 6, 2024

📜 Description

If the sentry init is already set up in a zone, don't create another one internally.

Provide a sentry wrapped Sentry.runZonedGuarded function which captures exceptions, adds breadcrumbs and propagate the user's onError on top of it so we can keep the original behaviour.

💡 Motivation and Context

Fixes #1943

💚 How did you test it?

Unit tests.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

Copy link

codecov bot commented Jun 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.37%. Comparing base (acc307e) to head (20261fb).

❗ There is a different number of reports uploaded between BASE (acc307e) and HEAD (20261fb). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (acc307e) HEAD (20261fb)
9 7
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2088      +/-   ##
==========================================
- Coverage   86.85%   78.37%   -8.48%     
==========================================
  Files         260       35     -225     
  Lines        9242     1489    -7753     
==========================================
- Hits         8027     1167    -6860     
+ Misses       1215      322     -893     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

github-actions bot commented Jun 6, 2024

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 444.80 ms 506.02 ms 61.22 ms
Size 6.49 MiB 7.56 MiB 1.07 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
d61cecf 339.69 ms 387.57 ms 47.88 ms
84c28cd 415.11 ms 549.44 ms 134.33 ms
bd75526 515.20 ms 568.79 ms 53.59 ms
90db9ff 334.86 ms 388.14 ms 53.28 ms
824df58 436.68 ms 548.80 ms 112.12 ms
c19bfb6 382.00 ms 454.62 ms 72.62 ms
af2d175 279.08 ms 312.37 ms 33.29 ms
0be962b 325.54 ms 382.83 ms 57.29 ms
e0f3acf 367.92 ms 402.89 ms 34.98 ms
78eeed5 298.35 ms 361.63 ms 63.28 ms

App size

Revision Plain With Sentry Diff
d61cecf 6.06 MiB 7.03 MiB 995.56 KiB
84c28cd 6.49 MiB 7.56 MiB 1.07 MiB
bd75526 6.49 MiB 7.56 MiB 1.07 MiB
90db9ff 6.06 MiB 7.10 MiB 1.04 MiB
824df58 6.35 MiB 7.35 MiB 1021.71 KiB
c19bfb6 6.35 MiB 7.41 MiB 1.05 MiB
af2d175 5.94 MiB 6.92 MiB 1001.83 KiB
0be962b 6.06 MiB 7.03 MiB 990.29 KiB
e0f3acf 6.49 MiB 7.56 MiB 1.08 MiB
78eeed5 6.16 MiB 7.14 MiB 1009.97 KiB

Previous results on branch: fix/runzoneguarded

Startup times

Revision Plain With Sentry Diff
b8b75a7 448.68 ms 490.80 ms 42.12 ms
ff275ca 503.47 ms 564.47 ms 61.00 ms
3f0f7d9 529.37 ms 585.55 ms 56.18 ms
4b77b41 478.64 ms 535.36 ms 56.72 ms

App size

Revision Plain With Sentry Diff
b8b75a7 6.49 MiB 7.56 MiB 1.08 MiB
ff275ca 6.49 MiB 7.56 MiB 1.07 MiB
3f0f7d9 6.49 MiB 7.56 MiB 1.07 MiB
4b77b41 6.49 MiB 7.56 MiB 1.07 MiB

Copy link
Contributor

github-actions bot commented Jun 6, 2024

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1251.88 ms 1279.57 ms 27.69 ms
Size 8.38 MiB 9.78 MiB 1.40 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
3f3ef0b 1223.73 ms 1237.67 ms 13.94 ms
3de8b9b 1234.22 ms 1251.94 ms 17.72 ms
0210372 1255.49 ms 1280.98 ms 25.49 ms
f6f3d25 1253.80 ms 1258.39 ms 4.59 ms
636cb61 1266.06 ms 1271.38 ms 5.31 ms
379d7a8 1267.65 ms 1288.39 ms 20.74 ms
0aaa46e 1256.39 ms 1276.41 ms 20.02 ms
72eeb80 1236.06 ms 1254.86 ms 18.80 ms
3d305b9 1230.29 ms 1247.39 ms 17.10 ms
78eeed5 1259.33 ms 1270.14 ms 10.82 ms

App size

Revision Plain With Sentry Diff
3f3ef0b 8.32 MiB 9.38 MiB 1.05 MiB
3de8b9b 8.28 MiB 9.34 MiB 1.06 MiB
0210372 8.38 MiB 9.73 MiB 1.35 MiB
f6f3d25 8.28 MiB 9.34 MiB 1.06 MiB
636cb61 8.28 MiB 9.34 MiB 1.06 MiB
379d7a8 8.16 MiB 9.16 MiB 1.00 MiB
0aaa46e 8.29 MiB 9.38 MiB 1.09 MiB
72eeb80 8.34 MiB 9.65 MiB 1.31 MiB
3d305b9 8.33 MiB 9.63 MiB 1.29 MiB
78eeed5 8.29 MiB 9.38 MiB 1.09 MiB

Previous results on branch: fix/runzoneguarded

Startup times

Revision Plain With Sentry Diff
3f0f7d9 1229.54 ms 1273.19 ms 43.65 ms
b8b75a7 1261.64 ms 1273.83 ms 12.19 ms
4b77b41 1249.47 ms 1278.40 ms 28.93 ms
ff275ca 1255.73 ms 1281.53 ms 25.80 ms

App size

Revision Plain With Sentry Diff
3f0f7d9 8.38 MiB 9.77 MiB 1.39 MiB
b8b75a7 8.38 MiB 9.78 MiB 1.40 MiB
4b77b41 8.38 MiB 9.77 MiB 1.39 MiB
ff275ca 8.38 MiB 9.78 MiB 1.40 MiB

@buenaflor buenaflor changed the title [draft] fix: don't create runZoneGuarded on web if it already exists [draft] fix(web): don't create runZoneGuarded on web if it already exists Jun 6, 2024
Copy link
Contributor Author

@buenaflor buenaflor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first look looks good, prolly also makes sense to test the case when apis are called before sentry is initialized e.g an error happens that triggers runZonedGuarded before SentryFlutter.init

@denrase denrase changed the title [draft] fix(web): don't create runZoneGuarded on web if it already exists Support custom Sentry.runZoneGuarded zone creation Nov 26, 2024
@denrase denrase marked this pull request as ready for review November 26, 2024 13:59
Comment on lines +76 to +82
final bool isRootZone = options.platformChecker.isRootZone;

// If onError is not supported and no custom zone exists, use runZonedGuarded to capture errors.
final bool useRunZonedGuarded = !isOnErrorSupported && isRootZone;

RunZonedGuardedOnError? runZonedGuardedOnError =
useRunZonedGuarded ? _createRunZonedGuardedOnError() : null;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo it would be great if we can identify if the user used our Sentry.runZonedGuarded vs a custom one and if they use a custom one we log a warning that we recommend switching to our implementation.

Comment on lines +370 to +376
/// With [runZonedGuarded] you can create a custom zone, and still let Sentry
/// report errors and breadcrumbs automatically.
///
/// It takes the same parameters as the dart function.
///
/// Please be aware that any errors in the zone which occur before the [init]
/// call cannot be handled by Sentry.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// With [runZonedGuarded] you can create a custom zone, and still let Sentry
/// report errors and breadcrumbs automatically.
///
/// It takes the same parameters as the dart function.
///
/// Please be aware that any errors in the zone which occur before the [init]
/// call cannot be handled by Sentry.
/// Creates a new error handling zone with Sentry integration using [runZonedGuarded].
///
/// This method provides automatic error reporting and breadcrumb tracking while
/// allowing you to define a custom error handling zone. It wraps Dart's native
/// [runZonedGuarded] function with Sentry-specific functionality.
///
/// This function automatically records calls to `print()` as Breadcrumbs and can be configured using [SentryOptions.enablePrintBreadcrumbs].

@@ -49,6 +49,27 @@
});
```
- Replace deprecated `BeforeScreenshotCallback` with new `BeforeCaptureCallback`.
- Support custom `Sentry.runZoneGuarded` zone creation ([#2088](https://github.com/getsentry/sentry-dart/pull/2088))
- Sentry will not create a custom zone anymore if it is started within a custom one.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe also makes sense to mention that this fixes Zone missmatch errors when trying to initialize WidgetsBinding before Sentry on Flutter Web

@buenaflor
Copy link
Contributor Author

@denrase also pls prepare docs for this

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.

Zone missmatch on flutter web
2 participants