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

Fix: Use PlatformDispatcher.onError in Flutter 3.3 #1039

Merged
merged 26 commits into from
Oct 13, 2022

Conversation

denrase
Copy link
Collaborator

@denrase denrase commented Oct 3, 2022

📜 Description

Closes #988

💚 How did you test it?

Added unit tests. Some expectations that were run in a closure would always evaluate as passed. Moved those out fo we don't have false positives anymore.

📝 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

Should we update the documentation to reflect the change?

@marandaneto
Copy link
Contributor

@denrase based on what I understood from #988
We'd do a try catch with NoSuchMethodError in order to detect if PlatformDispatcher.onError is available and use it automatically instead of having a boolean flag that has to be specified to the user, didn't work?

@marandaneto
Copy link
Contributor

@denrase based on what I understood from #988 We'd do a try catch with NoSuchMethodError in order to detect if PlatformDispatcher.onError is available and use it automatically instead of having a boolean flag that has to be specified to the user, didn't work?

nvm, the method isOnErrorSupported was already builtin.

@codecov-commenter
Copy link

codecov-commenter commented Oct 3, 2022

Codecov Report

Base: 89.97% // Head: 91.37% // Increases project coverage by +1.39% 🎉

Coverage data is based on head (485b071) compared to base (21845e2).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1039      +/-   ##
==========================================
+ Coverage   89.97%   91.37%   +1.39%     
==========================================
  Files         115        9     -106     
  Lines        3582      197    -3385     
==========================================
- Hits         3223      180    -3043     
+ Misses        359       17     -342     
Impacted Files Coverage Δ
dart/lib/src/sentry.dart
dart/lib/src/protocol/sentry_stack_trace.dart
dart/lib/src/protocol/debug_image.dart
dart/lib/src/integration.dart
dart/lib/src/transport/rate_limit.dart
dart/lib/src/sentry_exception_factory.dart
dart/lib/src/protocol/sentry_span.dart
dart/lib/src/client_reports/client_report.dart
dart/lib/src/protocol/sentry_trace_header.dart
...lib/src/invalid_sentry_trace_header_exception.dart
... and 96 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

CHANGELOG.md Outdated Show resolved Hide resolved
@denrase denrase requested a review from marandaneto October 10, 2022 09:12
@marandaneto
Copy link
Contributor

@denrase there are lint issues:

info • Unused import: 'binding_utils.dart' • lib/src/sentry_flutter.dart:9:8 • unused_import
info • The member 'callAppRunnerInRunZonedGuarded' can only be used within its package • lib/src/sentry_flutter.dart:68:9 • invalid_use_of_internal_member

@github-actions
Copy link
Contributor

github-actions bot commented Oct 11, 2022

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 298.33 ms 346.69 ms 48.36 ms
Size 5.94 MiB 6.92 MiB 1004.83 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
633cf2e 289.36 ms 340.38 ms 51.02 ms
56810ff 309.72 ms 352.26 ms 42.54 ms
9c5aec6 287.33 ms 320.94 ms 33.61 ms
4efee31 308.92 ms 368.68 ms 59.76 ms
72dfc83 298.62 ms 340.14 ms 41.52 ms
559d28f 302.35 ms 339.53 ms 37.18 ms
1c6eb5b 350.69 ms 393.86 ms 43.17 ms
3e5ee37 317.56 ms 366.84 ms 49.28 ms
af2d175 279.08 ms 312.37 ms 33.29 ms
21845e2 345.08 ms 382.82 ms 37.74 ms

App size

Revision Plain With Sentry Diff
633cf2e 5.94 MiB 6.92 MiB 1001.53 KiB
56810ff 5.94 MiB 6.92 MiB 1001.71 KiB
9c5aec6 5.94 MiB 6.92 MiB 1001.61 KiB
4efee31 5.94 MiB 6.92 MiB 1003.76 KiB
72dfc83 5.94 MiB 6.92 MiB 1001.71 KiB
559d28f 5.94 MiB 6.92 MiB 1001.70 KiB
1c6eb5b 5.94 MiB 6.92 MiB 1001.53 KiB
3e5ee37 5.94 MiB 6.92 MiB 1001.19 KiB
af2d175 5.94 MiB 6.92 MiB 1001.83 KiB
21845e2 5.94 MiB 6.92 MiB 1003.77 KiB

Previous results on branch: fix/platform-dispatcher-onerror

Startup times

Revision Plain With Sentry Diff
faf5538 298.72 ms 343.32 ms 44.60 ms
c5b5a6f 333.18 ms 379.58 ms 46.40 ms
33ad3ec 289.00 ms 333.98 ms 44.98 ms
54a2e71 298.66 ms 354.82 ms 56.16 ms
cee7e1b 291.74 ms 322.79 ms 31.05 ms

App size

Revision Plain With Sentry Diff
faf5538 5.94 MiB 6.92 MiB 1003.44 KiB
c5b5a6f 5.94 MiB 6.92 MiB 1003.64 KiB
33ad3ec 5.94 MiB 6.92 MiB 1004.81 KiB
54a2e71 5.94 MiB 6.92 MiB 1003.64 KiB
cee7e1b 5.94 MiB 6.92 MiB 1003.68 KiB

CHANGELOG.md Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Oct 11, 2022

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1292.49 ms 1318.90 ms 26.41 ms
Size 8.15 MiB 9.13 MiB 994.14 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
21845e2 1279.37 ms 1298.81 ms 19.45 ms
49a149b 1296.47 ms 1320.20 ms 23.73 ms
633cf2e 1257.96 ms 1275.73 ms 17.77 ms
6d317ea 1277.27 ms 1287.47 ms 10.20 ms
af2d175 1280.37 ms 1282.24 ms 1.88 ms
559d28f 1265.04 ms 1288.96 ms 23.92 ms
1c6eb5b 1277.85 ms 1285.71 ms 7.86 ms
72dfc83 1262.50 ms 1289.75 ms 27.25 ms
56810ff 1267.59 ms 1293.48 ms 25.89 ms
3e5ee37 1248.25 ms 1265.38 ms 17.13 ms

App size

Revision Plain With Sentry Diff
21845e2 8.15 MiB 9.12 MiB 991.34 KiB
49a149b 8.15 MiB 9.12 MiB 986.26 KiB
633cf2e 8.15 MiB 9.12 MiB 986.26 KiB
6d317ea 8.15 MiB 9.12 MiB 986.26 KiB
af2d175 8.15 MiB 9.12 MiB 986.22 KiB
559d28f 8.15 MiB 9.12 MiB 987.32 KiB
1c6eb5b 8.15 MiB 9.12 MiB 986.27 KiB
72dfc83 8.15 MiB 9.12 MiB 987.30 KiB
56810ff 8.15 MiB 9.12 MiB 987.35 KiB
3e5ee37 8.15 MiB 9.12 MiB 986.23 KiB

Previous results on branch: fix/platform-dispatcher-onerror

Startup times

Revision Plain With Sentry Diff
c5b5a6f 1255.33 ms 1282.94 ms 27.61 ms
faf5538 1284.10 ms 1304.94 ms 20.84 ms
cee7e1b 1265.79 ms 1296.23 ms 30.44 ms
9839e17 1291.92 ms 1301.76 ms 9.84 ms
54a2e71 1278.61 ms 1292.63 ms 14.02 ms

App size

Revision Plain With Sentry Diff
c5b5a6f 8.15 MiB 9.12 MiB 990.77 KiB
faf5538 8.15 MiB 9.12 MiB 990.78 KiB
cee7e1b 8.15 MiB 9.12 MiB 990.46 KiB
9839e17 8.15 MiB 9.12 MiB 990.74 KiB
54a2e71 8.15 MiB 9.12 MiB 990.78 KiB

@denrase
Copy link
Collaborator Author

denrase commented Oct 11, 2022

@marandaneto Can't reproduce the lint error error below locally, do you have a guess?

Bildschirmfoto 2022-10-11 um 14 07 00

@marandaneto
Copy link
Contributor

@marandaneto Can't reproduce the lint error error below locally, do you have a guess?

Bildschirmfoto 2022-10-11 um 14 07 00

That's not a problem, something we already discussed before:
dart-lang/pana#1020

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 a lot @denrase

@marandaneto
Copy link
Contributor

@denrase there's a bug here, OnErrorIntegration isn't executing correctly because the binding is null and it returns.
Test out if errors are being captured correctly with the new (onError) and old integration (zonedGuard).

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 comment
#1039 (comment)

@denrase denrase requested a review from marandaneto October 11, 2022 16:29
@marandaneto
Copy link
Contributor

marandaneto commented Oct 12, 2022

@denrase

Should we update the documentation to reflect the change?

Let's amend the docs that runZonedGuarded is run when PlatformDispatcher.onError isn't available.

The same for the README files, examples, etc within the repo for Flutter.

@marandaneto marandaneto enabled auto-merge (squash) October 13, 2022 08:57
@marandaneto marandaneto merged commit 02419b7 into main Oct 13, 2022
@marandaneto marandaneto deleted the fix/platform-dispatcher-onerror branch October 13, 2022 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use PlatformDispatcher.onError instead of custom zone starting with Flutter 3.3
4 participants