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

add multiview helper to make the sentry multiview aware. #2366

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

martinhaintz
Copy link
Contributor

@martinhaintz martinhaintz commented Oct 21, 2024

📜 Description

Currently, the Sentry Dart Plugin does not completely support the new multi-view feature for the web. Especially the SentryScreenshotWidget,SentryUserInteractionWidget and the WidgetsBindingIntegrationare incompatible.
Therefore we deactivate these features if we detect a multi-view application.

To find out if multiViewEnabled is set in the flutter_bootstrap.js I check the PlatformDispatcher.instance.implicitView return value.
In a regular non multiview app PlatformDispatcher.instance.implicitView returns a FlutterView object. If you try to call PlatformDispatcher.instance.implicitView you receive null. This is also explained here in the Flutter Docs

An alternative approach I found out, could be accessing the __flutterState object and look at the elements. In a regular non multiview application the first element is always a meta event.
image
For a MultiView App the __flutterState only contains flutter-view elements for the number of active views.
In this example, there were two active views:
image

💡 Motivation and Context

Based on the discussion here I added a MultiViewHelper, which is only active for the Web Platform.

💚 How did you test it?

local

📝 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

🔮 Next steps

Copy link
Contributor

github-actions bot commented Oct 21, 2024

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against addcc64

Copy link

codecov bot commented Oct 21, 2024

Codecov Report

Attention: Patch coverage is 63.63636% with 4 lines in your changes missing coverage. Please review.

Project coverage is 84.93%. Comparing base (8f95e33) to head (f0e82e4).

Files with missing lines Patch % Lines
...r/lib/src/integrations/screenshot_integration.dart 50.00% 1 Missing ⚠️
.../src/integrations/widgets_binding_integration.dart 50.00% 1 Missing ⚠️
flutter/lib/src/sentry_widget.dart 66.66% 1 Missing ⚠️
flutter/lib/src/utils/multi_view_helper.dart 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2366      +/-   ##
==========================================
+ Coverage   84.78%   84.93%   +0.14%     
==========================================
  Files         253       80     -173     
  Lines        9070     2801    -6269     
==========================================
- Hits         7690     2379    -5311     
+ Misses       1380      422     -958     

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

Copy link
Contributor

github-actions bot commented Oct 21, 2024

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 456.78 ms 504.18 ms 47.40 ms
Size 6.49 MiB 7.57 MiB 1.08 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
e2d89fc 323.84 ms 376.23 ms 52.39 ms
be173fa 375.94 ms 458.36 ms 82.42 ms
7b2e0ad 415.59 ms 457.26 ms 41.67 ms
322aa66 284.98 ms 341.76 ms 56.78 ms
eecbbca 324.37 ms 352.49 ms 28.12 ms
f3a18f2 456.29 ms 504.41 ms 48.12 ms
e0ba81f 390.38 ms 465.40 ms 75.02 ms
33527b4 403.58 ms 507.44 ms 103.86 ms
3334ac1 303.98 ms 366.65 ms 62.67 ms
5e7abc5 360.82 ms 401.18 ms 40.37 ms

App size

Revision Plain With Sentry Diff
e2d89fc 6.06 MiB 7.03 MiB 989.37 KiB
be173fa 6.35 MiB 7.33 MiB 1005.54 KiB
7b2e0ad 6.52 MiB 7.61 MiB 1.08 MiB
322aa66 5.94 MiB 6.92 MiB 1005.75 KiB
eecbbca 5.94 MiB 6.89 MiB 975.78 KiB
f3a18f2 6.49 MiB 7.57 MiB 1.08 MiB
e0ba81f 6.52 MiB 7.59 MiB 1.06 MiB
33527b4 6.35 MiB 7.42 MiB 1.07 MiB
3334ac1 6.06 MiB 7.03 MiB 993.54 KiB
5e7abc5 6.34 MiB 7.28 MiB 966.66 KiB

Previous results on branch: feat/multiview-aware

Startup times

Revision Plain With Sentry Diff
218975a 445.76 ms 496.16 ms 50.40 ms
cae9153 497.60 ms 574.20 ms 76.60 ms
f0e82e4 483.52 ms 570.02 ms 86.50 ms
1b36c56 458.39 ms 535.86 ms 77.47 ms
179091d 450.02 ms 498.72 ms 48.70 ms
9e610e4 464.50 ms 501.08 ms 36.58 ms
1dab88e 445.29 ms 501.21 ms 55.93 ms
f6239fb 466.59 ms 513.08 ms 46.49 ms

App size

Revision Plain With Sentry Diff
218975a 6.49 MiB 7.57 MiB 1.08 MiB
cae9153 6.49 MiB 7.57 MiB 1.08 MiB
f0e82e4 6.49 MiB 7.57 MiB 1.08 MiB
1b36c56 6.49 MiB 7.57 MiB 1.08 MiB
179091d 6.49 MiB 7.57 MiB 1.08 MiB
9e610e4 6.49 MiB 7.57 MiB 1.08 MiB
1dab88e 6.49 MiB 7.57 MiB 1.08 MiB
f6239fb 6.49 MiB 7.57 MiB 1.08 MiB

Copy link
Contributor

github-actions bot commented Oct 21, 2024

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1247.15 ms 1269.11 ms 21.96 ms
Size 8.38 MiB 9.75 MiB 1.37 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
8cb6557 1265.14 ms 1266.08 ms 0.94 ms
bc29768 1247.25 ms 1268.63 ms 21.38 ms
3e5078c 1239.73 ms 1248.69 ms 8.96 ms
6957bfd 1283.80 ms 1289.00 ms 5.20 ms
0ceb89c 1252.02 ms 1271.78 ms 19.75 ms
d883d62 1221.39 ms 1230.18 ms 8.80 ms
1d47eb7 1212.57 ms 1222.00 ms 9.43 ms
21d4150 1252.86 ms 1280.55 ms 27.69 ms
6a5a65d 1237.22 ms 1250.29 ms 13.07 ms
b0811cc 1238.23 ms 1255.82 ms 17.59 ms

App size

Revision Plain With Sentry Diff
8cb6557 8.10 MiB 9.18 MiB 1.08 MiB
bc29768 8.32 MiB 9.38 MiB 1.05 MiB
3e5078c 8.28 MiB 9.34 MiB 1.06 MiB
6957bfd 8.15 MiB 9.15 MiB 1020.71 KiB
0ceb89c 8.15 MiB 9.12 MiB 989.78 KiB
d883d62 8.29 MiB 9.36 MiB 1.07 MiB
1d47eb7 8.29 MiB 9.39 MiB 1.09 MiB
21d4150 8.16 MiB 9.17 MiB 1.01 MiB
6a5a65d 8.34 MiB 9.65 MiB 1.31 MiB
b0811cc 8.32 MiB 9.38 MiB 1.06 MiB

Previous results on branch: feat/multiview-aware

Startup times

Revision Plain With Sentry Diff
f6239fb 1243.48 ms 1259.36 ms 15.88 ms
cae9153 1228.17 ms 1244.92 ms 16.75 ms
9e610e4 1250.29 ms 1282.87 ms 32.59 ms
1b36c56 1244.15 ms 1268.33 ms 24.18 ms
179091d 1253.28 ms 1279.98 ms 26.70 ms
218975a 1256.33 ms 1289.69 ms 33.36 ms
f0e82e4 1254.55 ms 1279.82 ms 25.27 ms
1dab88e 1246.98 ms 1274.31 ms 27.33 ms

App size

Revision Plain With Sentry Diff
f6239fb 8.38 MiB 9.75 MiB 1.37 MiB
cae9153 8.38 MiB 9.75 MiB 1.37 MiB
9e610e4 8.38 MiB 9.75 MiB 1.37 MiB
1b36c56 8.38 MiB 9.75 MiB 1.37 MiB
179091d 8.38 MiB 9.75 MiB 1.37 MiB
218975a 8.38 MiB 9.75 MiB 1.37 MiB
f0e82e4 8.38 MiB 9.75 MiB 1.37 MiB
1dab88e 8.38 MiB 9.75 MiB 1.37 MiB

@martinhaintz martinhaintz changed the title add multiview helper to make the sentry widget multiview aware. add multiview helper to make the sentry multiview aware. Oct 22, 2024
@martinhaintz martinhaintz marked this pull request as ready for review October 22, 2024 11:47
Copy link
Contributor

@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.

not sure how it's gonna work but any chance we can set up some simple tests for this?

I wanna make sure that this doesn't regress in future flutter versions

CHANGELOG.md Outdated Show resolved Hide resolved
flutter/lib/src/sentry_flutter.dart Outdated Show resolved Hide resolved
flutter/lib/src/sentry_flutter.dart Outdated Show resolved Hide resolved
@buenaflor
Copy link
Contributor

buenaflor commented Oct 28, 2024

I'll put this PR into blocked until we can properly test it. Until then we will rely on docs to keep users informed about what to disable to make multiview work

@buenaflor buenaflor marked this pull request as draft November 1, 2024 13:12
@buenaflor buenaflor removed the Blocked label Dec 16, 2024
@buenaflor
Copy link
Contributor

I'll add an integration test for this

@buenaflor buenaflor self-assigned this Dec 16, 2024
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.

2 participants