Skip to content

Feature: Initial support for Flutter on Desktop #389

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

Merged
merged 41 commits into from
Apr 20, 2021
Merged

Conversation

ueman
Copy link
Collaborator

@ueman ueman commented Mar 29, 2021

📜 Description

Initial desktop support.
Currently only MacOS.

See https://sentry.io/organizations/sentry-sdks/issues/2149575550/?query=is%3Aunresolved for an example event.

What currently works:

  • Dart exceptions
  • Breadcrumbs for navigation
  • Release information
  • Native Integration just like iOS

What does not work:

  • Linux and Windows
  • SentryWidgetsBindingObserver.didChangeMetrics get called way too much. It could replace all breadcrumbs with just screen size changes.

💡 Motivation and Context

💚 How did you test it?

New tests and manual tests to make sure MacOS works.

📝 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

@codecov-io
Copy link

codecov-io commented Mar 29, 2021

Codecov Report

Merging #389 (d5e1123) into main (c1784cd) will decrease coverage by 0.76%.
The diff coverage is 54.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #389      +/-   ##
==========================================
- Coverage   90.74%   89.98%   -0.77%     
==========================================
  Files          52       54       +2     
  Lines        1654     1678      +24     
==========================================
+ Hits         1501     1510       +9     
- Misses        153      168      +15     
Impacted Files Coverage Δ
dart/lib/src/utils.dart 100.00% <ø> (ø)
dart/lib/src/platform/_io_platform.dart 14.28% <14.28%> (ø)
dart/lib/src/platform/platform.dart 14.28% <14.28%> (ø)
dart/lib/src/platform_checker.dart 62.50% <25.00%> (-37.50%) ⬇️
dart/lib/src/sentry.dart 86.56% <100.00%> (+0.20%) ⬆️
dart/lib/src/sentry_client.dart 93.24% <100.00%> (ø)
dart/lib/src/sentry_options.dart 87.87% <100.00%> (+0.37%) ⬆️
dart/lib/src/transport/http_transport.dart 92.30% <100.00%> (+0.15%) ⬆️
dart/lib/src/version.dart 100.00% <100.00%> (ø)
flutter/lib/src/sentry_flutter.dart 94.28% <100.00%> (+0.73%) ⬆️
... and 3 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 c1784cd...d5e1123. Read the comment docs.

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

Very cool!

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.

left a few small suggestions but overall looks good.
CI is broken though.
Approving it but let's address the open points before merging, LGTM.

@marandaneto marandaneto changed the title Feature: Initial support for Flutter on macOS Feature: Initial support for Flutter on Desktop Apr 14, 2021
@marandaneto
Copy link
Contributor

let's not squash & merge this branch, but instead just merging it, so we have the history of the other branches merged into this one.

* Support Linux

* Build Linux in CI

* Build linux in ci

* More linux foo

* Fix workflow file

* Update .github/workflows/flutter.yml

* Remove autogenerated files

* ignore autogenerated files

* --release is the default

* Fix Workflow file

Co-authored-by: Bruno Garcia <[email protected]>
@bruno-garcia
Copy link
Member

Merge commit is enabled on the repo

image

@marandaneto
Copy link
Contributor

@ueman can you test an uncaught exception for macOS? does it work or do we need to do this like for the Apple SDK? https://docs.sentry.io/platforms/apple/usage/#capturing-uncaught-exceptions-in-macos

@JigneshWorld
Copy link
Contributor

JigneshWorld commented Apr 16, 2021

Is this feature almost ready for next major release?

@ueman
Copy link
Collaborator Author

ueman commented Apr 19, 2021

@ueman can you test an uncaught exception for macOS? does it work or do we need to do this like for the Apple SDK? https://docs.sentry.io/platforms/apple/usage/#capturing-uncaught-exceptions-in-macos

If I click on Objective-C Throw unhandled exception it crashes the app with the following message.

2021-04-19 08:22:28.189 sentry_flutter_example[64302:2120223] *** Terminating app due to uncaught exception 'Raised from Objective-C.', reason: 'The value 42 is the answer'
*** First throw call stack:
(
	0   CoreFoundation                      0x00007fff207316af __exceptionPreprocess + 242
	1   libobjc.A.dylib                     0x00007fff204693c9 objc_exception_throw + 48
	2   CoreFoundation                      0x00007fff20731513 +[NSException raise:format:] + 189
	3   sentry_flutter_example              0x0000000108150280 +[Buggy throw] + 64
	4   sentry_flutter_example              0x00000001081515a8 $s22sentry_flutter_example17MainFlutterWindowC13handleMessage33_8C1567E4456C97301B55DAF33086A62BLL4call6resultySo0E10MethodCallC_yypSgXEtF + 1960
	5   sentry_flutter_example              0x0000000108150ad9 $s22sentry_flutter_example17MainFlutterWindowC12awakeFromNibyyFySo0E10MethodCallC_yypSgXEtcACcfu_yAF_yAGXEtcfu0_ + 57
	6   sentry_flutter_example              0x0000000108150b30 $sSo17FlutterMethodCallCypSgIgn_Ieggy_AbCIegn_Ieggg_TR + 64
	7   sentry_flutter_example              0x0000000108150c33 $sSo17FlutterMethodCallCypSgIegn_Ieggg_AByXlSgIeyBy_IeyByy_TR + 147
	8   FlutterMacOS                        0x0000000108a95367 __45-[FlutterMethodChannel setMethodCallHandler:]_block_invoke + 119
	9   FlutterMacOS                        0x00000001082fb9a3 -[FlutterEngine engineCallbackOnPlatformMessage:] + 275
	10  FlutterMacOS                        0x00000001082fa71c _ZL17OnPlatformMessagePK22FlutterPlatformMessageP13FlutterEngine + 44
	11  FlutterMacOS                        0x0000000108aa9a1d _ZNSt3__110__function6__funcIZ23FlutterEngineInitializeE4$_43NS_9allocatorIS2_EEFvN3fml6RefPtrIN7flutter15PlatformMessageEEEEEclEOS9_ + 125
	12  FlutterMacOS                        0x0000000108aba63a _ZN7flutter20PlatformViewEmbedder21HandlePlatformMessageEN3fml6RefPtrINS_15PlatformMessageEEE + 74
	13  FlutterMacOS                        0x0000000108823979 _ZNSt3__110__function6__funcIZN7flutter5Shell29OnEngineHandlePlatformMessageEN3fml6RefPtrINS2_15PlatformMessageEEEE4$_36NS_9allocatorIS8_EEFvvEEclEv + 137
	14  FlutterMacOS                        0x0000000108ab7b37 _ZN7flutter18EmbedderTaskRunner8PostTaskEy + 759
	15  FlutterMacOS                        0x0000000108aa10cc FlutterEngineRunTask + 44
	16  FlutterMacOS                        0x00000001082fc3d7 __60-[FlutterEngine postMainThreadTask:targetTimeInNanoseconds:]_block_invoke + 71
	17  libdispatch.dylib                   0x00007fff204135dd _dispatch_call_block_and_release + 12
	18  libdispatch.dylib                   0x00007fff204147c7 _dispatch_client_callout + 8
	19  libdispatch.dylib                   0x00007fff20420b86 _dispatch_main_queue_callback_4CF + 940
	20  CoreFoundation                      0x00007fff206f4970 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 9
	21  CoreFoundation                      0x00007fff206b6852 __CFRunLoopRun + 2731
	22  CoreFoundation                      0x00007fff206b56ce CFRunLoopRunSpecific + 563
	23  HIToolbox                           0x00007fff2893d630 RunCurrentEventLoopInMode + 292
	24  HIToolbox                           0x00007fff2893d42c ReceiveNextEventCommon + 709
	25  HIToolbox                           0x00007fff2893d14f _BlockUntilNextEventMatchingListInModeWithFilter + 64
	26  AppKit                              0x00007fff22ed59b1 _DPSNextEvent + 883
	27  AppKit                              0x00007fff22ed4177 -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 1366
	28  AppKit                              0x00007fff22ec668a -[NSApplication run] + 586
	29  AppKit                              0x00007fff22e9a96f NSApplicationMain + 816
	30  sentry_flutter_example              0x000000010815206d main + 13
	31  libdyld.dylib                       0x00007fff205da621 start + 1
	32  ???                                 0x0000000000000001 0x0 + 1
)
libc++abi.dylib: terminating with uncaught exception of type NSException
Lost connection to device.

When I restart the app, it gets send to Sentry: https://sentry.io/share/issue/4031d612e7504368ab4df8c19c4708bd/
I didn't need to follow the steps of the linked docs.

Is that what you wanted to know?

@ueman
Copy link
Collaborator Author

ueman commented Apr 19, 2021

Is this feature almost ready for next major release?

I just need to fix the CI build and it should be ready. @marandaneto knows more about the release timeline.

@marandaneto
Copy link
Contributor

@ueman can you test an uncaught exception for macOS? does it work or do we need to do this like for the Apple SDK? https://docs.sentry.io/platforms/apple/usage/#capturing-uncaught-exceptions-in-macos

If I click on Objective-C Throw unhandled exception it crashes the app with the following message.

2021-04-19 08:22:28.189 sentry_flutter_example[64302:2120223] *** Terminating app due to uncaught exception 'Raised from Objective-C.', reason: 'The value 42 is the answer'
*** First throw call stack:
(
	0   CoreFoundation                      0x00007fff207316af __exceptionPreprocess + 242
	1   libobjc.A.dylib                     0x00007fff204693c9 objc_exception_throw + 48
	2   CoreFoundation                      0x00007fff20731513 +[NSException raise:format:] + 189
	3   sentry_flutter_example              0x0000000108150280 +[Buggy throw] + 64
	4   sentry_flutter_example              0x00000001081515a8 $s22sentry_flutter_example17MainFlutterWindowC13handleMessage33_8C1567E4456C97301B55DAF33086A62BLL4call6resultySo0E10MethodCallC_yypSgXEtF + 1960
	5   sentry_flutter_example              0x0000000108150ad9 $s22sentry_flutter_example17MainFlutterWindowC12awakeFromNibyyFySo0E10MethodCallC_yypSgXEtcACcfu_yAF_yAGXEtcfu0_ + 57
	6   sentry_flutter_example              0x0000000108150b30 $sSo17FlutterMethodCallCypSgIgn_Ieggy_AbCIegn_Ieggg_TR + 64
	7   sentry_flutter_example              0x0000000108150c33 $sSo17FlutterMethodCallCypSgIegn_Ieggg_AByXlSgIeyBy_IeyByy_TR + 147
	8   FlutterMacOS                        0x0000000108a95367 __45-[FlutterMethodChannel setMethodCallHandler:]_block_invoke + 119
	9   FlutterMacOS                        0x00000001082fb9a3 -[FlutterEngine engineCallbackOnPlatformMessage:] + 275
	10  FlutterMacOS                        0x00000001082fa71c _ZL17OnPlatformMessagePK22FlutterPlatformMessageP13FlutterEngine + 44
	11  FlutterMacOS                        0x0000000108aa9a1d _ZNSt3__110__function6__funcIZ23FlutterEngineInitializeE4$_43NS_9allocatorIS2_EEFvN3fml6RefPtrIN7flutter15PlatformMessageEEEEEclEOS9_ + 125
	12  FlutterMacOS                        0x0000000108aba63a _ZN7flutter20PlatformViewEmbedder21HandlePlatformMessageEN3fml6RefPtrINS_15PlatformMessageEEE + 74
	13  FlutterMacOS                        0x0000000108823979 _ZNSt3__110__function6__funcIZN7flutter5Shell29OnEngineHandlePlatformMessageEN3fml6RefPtrINS2_15PlatformMessageEEEE4$_36NS_9allocatorIS8_EEFvvEEclEv + 137
	14  FlutterMacOS                        0x0000000108ab7b37 _ZN7flutter18EmbedderTaskRunner8PostTaskEy + 759
	15  FlutterMacOS                        0x0000000108aa10cc FlutterEngineRunTask + 44
	16  FlutterMacOS                        0x00000001082fc3d7 __60-[FlutterEngine postMainThreadTask:targetTimeInNanoseconds:]_block_invoke + 71
	17  libdispatch.dylib                   0x00007fff204135dd _dispatch_call_block_and_release + 12
	18  libdispatch.dylib                   0x00007fff204147c7 _dispatch_client_callout + 8
	19  libdispatch.dylib                   0x00007fff20420b86 _dispatch_main_queue_callback_4CF + 940
	20  CoreFoundation                      0x00007fff206f4970 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 9
	21  CoreFoundation                      0x00007fff206b6852 __CFRunLoopRun + 2731
	22  CoreFoundation                      0x00007fff206b56ce CFRunLoopRunSpecific + 563
	23  HIToolbox                           0x00007fff2893d630 RunCurrentEventLoopInMode + 292
	24  HIToolbox                           0x00007fff2893d42c ReceiveNextEventCommon + 709
	25  HIToolbox                           0x00007fff2893d14f _BlockUntilNextEventMatchingListInModeWithFilter + 64
	26  AppKit                              0x00007fff22ed59b1 _DPSNextEvent + 883
	27  AppKit                              0x00007fff22ed4177 -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 1366
	28  AppKit                              0x00007fff22ec668a -[NSApplication run] + 586
	29  AppKit                              0x00007fff22e9a96f NSApplicationMain + 816
	30  sentry_flutter_example              0x000000010815206d main + 13
	31  libdyld.dylib                       0x00007fff205da621 start + 1
	32  ???                                 0x0000000000000001 0x0 + 1
)
libc++abi.dylib: terminating with uncaught exception of type NSException
Lost connection to device.

When I restart the app, it gets send to Sentry: https://sentry.io/share/issue/4031d612e7504368ab4df8c19c4708bd/
I didn't need to follow the steps of the linked docs.

Is that what you wanted to know?

yes, thanks.

@marandaneto
Copy link
Contributor

Is this feature almost ready for next major release?

I just need to fix the CI build and it should be ready. @marandaneto knows more about the release timeline.

still #410 pending and getsentry/sentry-docs#3399

@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2021

Codecov Report

Merging #389 (50ab07a) into main (f9e674d) will increase coverage by 2.74%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #389      +/-   ##
==========================================
+ Coverage   90.78%   93.52%   +2.74%     
==========================================
  Files          52        5      -47     
  Lines        1660      170    -1490     
==========================================
- Hits         1507      159    -1348     
+ Misses        153       11     -142     
Impacted Files Coverage Δ
flutter/lib/src/sentry_flutter.dart 94.28% <100.00%> (+0.73%) ⬆️
flutter/lib/src/sentry_flutter_options.dart 79.48% <100.00%> (ø)
dart/lib/src/noop_hub.dart
dart/lib/src/diagnostic_logger.dart
dart/lib/src/scope.dart
dart/lib/src/protocol/sentry_gpu.dart
dart/lib/src/protocol/sentry_runtime.dart
dart/lib/src/throwable_mechanism.dart
dart/lib/src/protocol/debug_image.dart
dart/lib/src/protocol/sentry_id.dart
... and 32 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 f9e674d...50ab07a. Read the comment docs.

@ueman
Copy link
Collaborator Author

ueman commented Apr 19, 2021

@marandaneto This builds now. Do you want to take another look or can I merge this now?

@marandaneto
Copy link
Contributor

@marandaneto This builds now. Do you want to take another look or can I merge this now?

can we merge without #420 or should we wait for it?
let's not do squash & merge btw, but PR is approved from my side.

@ueman
Copy link
Collaborator Author

ueman commented Apr 19, 2021

can we merge without #420 or should we wait for it?

We could merge without it, but it's probably better to wait for it.

let's not do squash & merge btw

Yep, sure.

@marandaneto
Copy link
Contributor

@ueman do we need to run tests also for macos/win/linux? See https://pub.dev/packages/test#platform-selectors
right now we run for chrome and vm, which should be enough.
flutter module we just do flutter test, I don't see to find the -p argument available

@ueman
Copy link
Collaborator Author

ueman commented Apr 19, 2021

right now we run for chrome and vm, which should be enough.

I think so too. We can mock the platform and all of the Dart and Flutter code does not have any implementation which work on just one platform because we can also mock the platform channels.

Also Sentry is platform agnostic, so it doesn't matter.

* Fix: Trim Unicode Character 'NULL' (U+0000) for Windows

* Add PR ID

* Rename method and add some explanation

* Update flutter/lib/src/default_integrations.dart

Co-authored-by: Manoel Aranda Neto <[email protected]>

* Add another test

Co-authored-by: Manoel Aranda Neto <[email protected]>
@ueman ueman merged commit a6fb132 into main Apr 20, 2021
@ueman ueman deleted the feature/support-desktop branch April 20, 2021 08:32
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.

7 participants