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

Feature: PlatformDispatcher.onError integration #915

Merged
merged 14 commits into from
Jul 26, 2022

Conversation

ueman
Copy link
Collaborator

@ueman ueman commented Jul 1, 2022

📜 Description

This adds an integration for PlatformDispatcher.onError which was introduced in the Flutter 3.1 beta.

The interesting thing about the addition of this error handler is that the runZonedGuarded integration isn't needed anymore. This needs some more testing, though.
Ideally, the runZonedGuarded integration shouldn't be applied anymore if the PlatformDispatcher.onError is available. The removal of runZonedGuarded also removes the ability to log print() statements. In Flutter that's not that bad, as Flutter uses debugPrint() which is still logged as breadcrumbs.
This could also replace the IsolateErrorIntegration (at least on Flutter), but that needs to be tested, too.

The removal of the runZonedGuarded integration leads to much less complicated manual initialization code, because Sentry can now be initialized anywhere in the application lifecycle. There's no need anymore for Sentry to be the first call the user does in his code.

This is especially important for the use case, where crash reporting should only be enabled after user consent.

Example event: https://sentry.io/organizations/sentry-sdks/issues/3229407573/events/c912a632f2b147099657544fa4f22c60/?project=5428562

The error handler was introduced in flutter/engine#32078

💡 Motivation and Context

Closes #833

💚 How did you test it?

New test + example application

Test scenarios

  • Passing error directly to the error handler
  • Remove the runZoneGuarded integration and throw and uncaught exception

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

  • Release it.
  • Add functionality to remove the need for the runZoneGuarded integration if this error handler is available.

@codecov-commenter
Copy link

codecov-commenter commented Jul 1, 2022

Codecov Report

Merging #915 (58cb46b) into main (507f4df) will increase coverage by 1.65%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #915      +/-   ##
==========================================
+ Coverage   89.49%   91.14%   +1.65%     
==========================================
  Files         105        9      -96     
  Lines        3303      192    -3111     
==========================================
- Hits         2956      175    -2781     
+ Misses        347       17     -330     
Impacted Files Coverage Δ
dart/lib/src/http_client/breadcrumb_client.dart
dart/lib/src/protocol/span_status.dart
dart/lib/src/transport/http_transport.dart
dart/lib/src/protocol/sentry_device.dart
dart/lib/src/isolate_error_integration.dart
dart/lib/src/protocol/span_id.dart
.../lib/src/enricher/io_enricher_event_processor.dart
dart/lib/src/sentry_span_context.dart
dart/lib/src/protocol/dsn.dart
dart/lib/src/protocol/sentry_stack_frame.dart
... and 86 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 507f4df...58cb46b. Read the comment docs.

@ueman
Copy link
Collaborator Author

ueman commented Jul 1, 2022

Not sure why the tests fail on the beta channel. The type errors which are shown are just wrong: https://github.com/flutter/engine/blob/28a5d3c05d61b25f82080260bae4b6c3b77d03b5/lib/ui/platform_dispatcher.dart#L64

Edit: Web has a different signature: https://github.com/flutter/engine/blob/28a5d3c05d61b25f82080260bae4b6c3b77d03b5/lib/web_ui/lib/platform_dispatcher.dart#L17

Edit 2: I guess the tests have to be disabled until flutter/engine#34428 hits the beta/stable channel or this should only be merged when the fixed error handler hits the beta/stable channel.

Edit 3: Fix has landed

Edit 4: Latest Flutter beta includes the fix, so this is ready

@ueman ueman marked this pull request as ready for review July 20, 2022 22:14
@marandaneto
Copy link
Contributor

@ueman Right now the OnErrorIntegration is installed automatically.
We also have the IsolateErrorIntegration, RunZonedGuardedIntegration, FlutterErrorIntegration.
I'm wondering how all of these integrations play together, maybe not adding the OnErrorIntegration by default right now is the right way to do so? We could add code snippets on readme to actually remove such integrations on SDK init and add the OnErrorIntegration manually until this is GA? what do you think?

@ueman
Copy link
Collaborator Author

ueman commented Jul 21, 2022

@ueman Right now the OnErrorIntegration is installed automatically. We also have the IsolateErrorIntegration, RunZonedGuardedIntegration, FlutterErrorIntegration. I'm wondering how all of these integrations play together, maybe not adding the OnErrorIntegration by default right now is the right way to do so? We could add code snippets on readme to actually remove such integrations on SDK init and add the OnErrorIntegration manually until this is GA? what do you think?

Sounds reasonable, since it's only available in the beta anyway.

The FlutterErrorIntegration and this one play nicely together. The IsolateIntegration probably too. Just the RunZoneGuardedIntegration will become obsolete.

That's however quite nice, because it enables us (speaking as a user) to be much more flexible in enabling and disabling Sentry. That's important for GDPR, for example.


The goal is to use this integration when available and the RunZoneGuardedIntegration on older Flutter versions.

flutter/test/integrations/on_error_integration_test.dart Outdated Show resolved Hide resolved
flutter/lib/src/integrations/on_error_integration.dart Outdated Show resolved Hide resolved
flutter/example/lib/main.dart Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

Thanks @ueman

@marandaneto marandaneto enabled auto-merge (squash) July 26, 2022 06:42
@marandaneto marandaneto merged commit 53b6921 into getsentry:main Jul 26, 2022
@ueman ueman deleted the feature/platformDispatcher_onError branch August 11, 2022 18:38
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.

Support new PlatformDispatcher.onError
3 participants