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

Migration to usage of PlatformView #2249

Closed
wants to merge 23 commits into from

Conversation

martinhaintz
Copy link
Collaborator

@martinhaintz martinhaintz commented Aug 27, 2024

📜 Description

Fix problems using flutter multiview. At the moment the deprecated SingleFlutterWindows is used which is not compatible with multiview.

Here you find the migration guide

At the moment you need these additional run args to run a Flutter for MultiView App: --web-renderer auto

💡 Motivation and Context

close #2225

💚 How did you test it?

Adapted the unittest and adapted the example app, supporting two instances of the sample app.

📝 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

  • chore: refactor sentry-dart to handle multiple views at the same time, therefor we must get rid of the global keys.

Copy link
Contributor

github-actions bot commented Aug 27, 2024

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Migration to usage of PlatformView ([#2249](https://github.com/getsentry/sentry-dart/pull/2249))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 3fb08ac

Copy link

codecov bot commented Aug 27, 2024

Codecov Report

Attention: Patch coverage is 85.21739% with 17 lines in your changes missing coverage. Please review.

Project coverage is 88.23%. Comparing base (ea60f10) to head (3fb08ac).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
dart/lib/src/protocol/sentry_device.dart 72.91% 13 Missing ⚠️
...nt_processor/flutter_enricher_event_processor.dart 90.24% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2249      +/-   ##
==========================================
- Coverage   88.35%   88.23%   -0.13%     
==========================================
  Files         247      247              
  Lines        8578     8635      +57     
==========================================
+ Hits         7579     7619      +40     
- Misses        999     1016      +17     

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

@martinhaintz martinhaintz marked this pull request as ready for review August 28, 2024 13:01
@martinhaintz martinhaintz enabled auto-merge (squash) August 28, 2024 13:01
@martinhaintz martinhaintz marked this pull request as draft August 28, 2024 13:02
Copy link
Contributor

github-actions bot commented Aug 28, 2024

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 500.76 ms 540.00 ms 39.24 ms
Size 6.49 MiB 7.55 MiB 1.06 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
64af39c 386.80 ms 471.11 ms 84.31 ms
0ceb89c 304.57 ms 357.18 ms 52.61 ms
be08ed1 361.37 ms 414.84 ms 53.47 ms
3f23617 385.77 ms 476.10 ms 90.33 ms
42f6e7e 308.71 ms 360.06 ms 51.35 ms
c70e01a 331.04 ms 401.46 ms 70.42 ms
9d7e862 426.35 ms 510.88 ms 84.53 ms
7f75f32 347.36 ms 419.58 ms 72.22 ms
86d4841 286.35 ms 372.43 ms 86.08 ms
3637a22 322.59 ms 390.00 ms 67.41 ms

App size

Revision Plain With Sentry Diff
64af39c 6.27 MiB 7.20 MiB 958.83 KiB
0ceb89c 5.94 MiB 6.95 MiB 1.01 MiB
be08ed1 6.33 MiB 7.26 MiB 946.42 KiB
3f23617 5.94 MiB 6.96 MiB 1.02 MiB
42f6e7e 6.27 MiB 7.20 MiB 956.06 KiB
c70e01a 5.94 MiB 6.97 MiB 1.03 MiB
9d7e862 6.33 MiB 7.26 MiB 943.41 KiB
7f75f32 6.26 MiB 7.20 MiB 959.18 KiB
86d4841 6.15 MiB 7.13 MiB 1000.49 KiB
3637a22 6.06 MiB 7.09 MiB 1.03 MiB

Previous results on branch: fix/flutter-multiview-support

Startup times

Revision Plain With Sentry Diff
2886956 453.89 ms 504.72 ms 50.83 ms
e449941 404.27 ms 447.04 ms 42.77 ms
7f9b83f 436.02 ms 463.04 ms 27.02 ms
1b75753 471.22 ms 543.42 ms 72.20 ms
6883e29 445.52 ms 484.45 ms 38.93 ms
221dadd 721.34 ms 773.37 ms 52.03 ms
cb75ff1 410.88 ms 469.84 ms 58.96 ms
4e69486 428.79 ms 474.69 ms 45.90 ms
7d14655 419.79 ms 457.67 ms 37.88 ms
1396dfb 475.13 ms 511.56 ms 36.43 ms

App size

Revision Plain With Sentry Diff
2886956 6.52 MiB 7.61 MiB 1.08 MiB
e449941 6.52 MiB 7.58 MiB 1.06 MiB
7f9b83f 6.52 MiB 7.61 MiB 1.08 MiB
1b75753 6.52 MiB 7.61 MiB 1.08 MiB
6883e29 6.49 MiB 7.55 MiB 1.06 MiB
221dadd 6.52 MiB 7.61 MiB 1.08 MiB
cb75ff1 6.52 MiB 7.61 MiB 1.08 MiB
4e69486 6.52 MiB 7.61 MiB 1.08 MiB
7d14655 6.52 MiB 7.61 MiB 1.08 MiB
1396dfb 6.52 MiB 7.58 MiB 1.06 MiB

Copy link
Contributor

github-actions bot commented Aug 28, 2024

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1252.86 ms 1276.17 ms 23.31 ms
Size 8.38 MiB 9.73 MiB 1.35 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
8776cdf 1237.64 ms 1246.83 ms 9.20 ms
f79eecf 1210.25 ms 1221.65 ms 11.40 ms
abfcdb5 1230.87 ms 1244.94 ms 14.06 ms
9d7e862 1236.88 ms 1258.62 ms 21.74 ms
7e7f0b1 1230.52 ms 1251.49 ms 20.97 ms
3d305b9 1230.29 ms 1247.39 ms 17.10 ms
1596141 1230.77 ms 1241.90 ms 11.13 ms
d4d0807 1246.94 ms 1260.69 ms 13.75 ms
051e97a 1245.94 ms 1249.51 ms 3.57 ms
d089990 1206.19 ms 1233.08 ms 26.89 ms

App size

Revision Plain With Sentry Diff
8776cdf 8.33 MiB 9.40 MiB 1.07 MiB
f79eecf 8.29 MiB 9.36 MiB 1.07 MiB
abfcdb5 8.33 MiB 9.64 MiB 1.31 MiB
9d7e862 8.32 MiB 9.38 MiB 1.05 MiB
7e7f0b1 8.33 MiB 9.61 MiB 1.27 MiB
3d305b9 8.33 MiB 9.63 MiB 1.29 MiB
1596141 8.29 MiB 9.37 MiB 1.08 MiB
d4d0807 8.33 MiB 9.64 MiB 1.31 MiB
051e97a 8.28 MiB 9.34 MiB 1.06 MiB
d089990 8.33 MiB 9.40 MiB 1.07 MiB

Previous results on branch: fix/flutter-multiview-support

Startup times

Revision Plain With Sentry Diff
2886956 1241.90 ms 1266.65 ms 24.75 ms
7f9b83f 1249.25 ms 1272.88 ms 23.63 ms
7d14655 1243.18 ms 1273.00 ms 29.82 ms
4e69486 1237.12 ms 1260.67 ms 23.55 ms
1396dfb 1227.92 ms 1244.87 ms 16.95 ms
221dadd 1240.65 ms 1261.70 ms 21.06 ms
e449941 1229.31 ms 1239.63 ms 10.32 ms
1b75753 1245.48 ms 1270.96 ms 25.48 ms
6883e29 1236.39 ms 1277.02 ms 40.63 ms
cb75ff1 1239.06 ms 1268.98 ms 29.92 ms

App size

Revision Plain With Sentry Diff
2886956 8.38 MiB 9.73 MiB 1.35 MiB
7f9b83f 8.38 MiB 9.73 MiB 1.35 MiB
7d14655 8.38 MiB 9.73 MiB 1.35 MiB
4e69486 8.38 MiB 9.73 MiB 1.35 MiB
1396dfb 8.38 MiB 9.71 MiB 1.34 MiB
221dadd 8.38 MiB 9.73 MiB 1.35 MiB
e449941 8.38 MiB 9.71 MiB 1.34 MiB
1b75753 8.38 MiB 9.73 MiB 1.35 MiB
6883e29 8.38 MiB 9.73 MiB 1.35 MiB
cb75ff1 8.38 MiB 9.73 MiB 1.35 MiB

@martinhaintz
Copy link
Collaborator Author

I am stuck with this console output:

Launching lib/main.dart on Chrome in debug mode...
Waiting for connection from debug service on Chrome...
Warning: In index.html:48: Manual service worker registration deprecated. Use flutter.js service worker bootstrapping instead. See https://docs.flutter.dev/platform-integration/web/initialization for more details.
This app is linked to the debug service: ws://127.0.0.1:54138/W2P0FZ1IU98=/ws
Debug service listening on ws://127.0.0.1:54138/W2P0FZ1IU98=/ws
Debug service listening on ws://127.0.0.1:54138/W2P0FZ1IU98=/ws
[sentry] [debug] release: sentry_flutter_example@8.8.0
[sentry] [debug] Capture from onError The render object for Listener cannot find ancestor render object to attach to.
The ownership chain for the RenderObject in question was:
  Listener ← SentryUserInteractionWidget ← Container-[GlobalKey#15e26 sentry_widget] ← SentryWidget ← [root]
Try wrapping your widget in a View widget or any other widget that is backed by a RenderTreeRootElement to serve as the root of the render tree.
[sentry] [debug] Capture from onError The Element for ViewCollection cannot be inserted into slot "null" of its ancestor.
The ownership chain for the Element in question was:
  ViewCollection ← MultiViewApp ← DefaultAssetBundle ← RepaintBoundary-[GlobalKey#f9eb4 sentry_screenshot_widget] ← SentryScreenshotWidget ← Listener ← SentryUserInteractionWidget ← Container-[GlobalKey#15e26 sentry_widget] ← SentryWidget ← [root]
This Element allows the creation of multiple independent render trees, which cannot be attached to an ancestor in an existing render tree. However, an ancestor RenderObject is expecting that a child will be attached.
Try moving the subtree that contains the ViewCollection widget into the view property of a ViewAnchor widget or to the root of the widget tree, where it is not expected to attach its RenderObject to its ancestor.
[sentry.flutterError] [error] Exception caught by Flutter framework
                      The render object for Listener cannot find ancestor render object to attach to.
The ownership chain for the RenderObject in question was:
  Listener ← SentryUserInteractionWidget ← Container-[GlobalKey#15e26 sentry_widget] ← SentryWidget ← [root]
Try wrapping your widget in a View widget or any other widget that is backed by a RenderTreeRootElement to serve as the root of the render tree.
[sentry.flutterError] [error] Exception caught by Flutter framework
                      The Element for ViewCollection cannot be inserted into slot "null" of its ancestor.
The ownership chain for the Element in question was:
  ViewCollection ← MultiViewApp ← DefaultAssetBundle ← RepaintBoundary-[GlobalKey#f9eb4 sentry_screenshot_widget] ← SentryScreenshotWidget ← Listener ← SentryUserInteractionWidget ← Container-[GlobalKey#15e26 sentry_widget] ← SentryWidget ← [root]
This Element allows the creation of multiple independent render trees, which cannot be attached to an ancestor in an existing render tree. However, an ancestor RenderObject is expecting that a child will be attached.
Try moving the subtree that contains the ViewCollection widget into the view property of a ViewAnchor widget or to the root of the widget tree, where it is not expected to attach its RenderObject to its ancestor.
[sentry] [warning] Failed to send envelope to Spotlight: ClientException: XMLHttpRequest error., uri=http://localhost:8969/stream
[sentry] [error] Taking screenshot failed.
         View.of() was called with a context that does not contain a View widget.
No View widget ancestor could be found starting from the context that was passed to View.of().
The context used was:
  RepaintBoundary-[GlobalKey#f9eb4 sentry_screenshot_widget](renderObject: RenderRepaintBoundary#fb921 NEEDS-LAYOUT NEEDS-PAINT DETACHED)
This usually means that the provided context is not associated with a View.
         
[sentry] [error] Taking screenshot failed.
         View.of() was called with a context that does not contain a View widget.
No View widget ancestor could be found starting from the context that was passed to View.of().
The context used was:
  RepaintBoundary-[GlobalKey#f9eb4 sentry_screenshot_widget](renderObject: RenderRepaintBoundary#fb921 NEEDS-LAYOUT NEEDS-PAINT DETACHED)
This usually means that the provided context is not associated with a View.
         
[sentry] [warning] Failed to send envelope to Spotlight: ClientException: XMLHttpRequest error., uri=http://localhost:8969/stream
[sentry] [debug] Envelope b8532f950910441d80d0dda863f7b004 was sent successfully to Sentry.
[sentry] [debug] Envelope a49e1602c6054091a9218647ebeb1f23 was sent successfully to Sentry.

======== Exception caught by Flutter framework =====================================================
The following assertion was thrown:
The Element for ViewCollection cannot be inserted into slot "null" of its ancestor. 

The ownership chain for the Element in question was:
  ViewCollection ← MultiViewApp ← DefaultAssetBundle ← RepaintBoundary-[GlobalKey#f9eb4 sentry_screenshot_widget] ← SentryScreenshotWidget ← Listener ← SentryUserInteractionWidget ← Container-[GlobalKey#15e26 sentry_widget] ← SentryWidget ← [root]
This Element allows the creation of multiple independent render trees, which cannot be attached to an ancestor in an existing render tree. However, an ancestor RenderObject is expecting that a child will be attached.

Try moving the subtree that contains the ViewCollection widget into the view property of a ViewAnchor widget or to the root of the widget tree, where it is not expected to attach its RenderObject to its ancestor.
====================================================================================================

======== Exception caught by Flutter framework =====================================================
The following assertion was thrown:
The render object for Listener cannot find ancestor render object to attach to.

The ownership chain for the RenderObject in question was:
  Listener ← SentryUserInteractionWidget ← Container-[GlobalKey#15e26 sentry_widget] ← SentryWidget ← [root]

Try wrapping your widget in a View widget or any other widget that is backed by a RenderTreeRootElement to serve as the root of the render tree.
====================================================================================================

Any ideas how to resolve this? @buenaflor @denrase

ps: if you run the example application, don't forget to add --web-renderer auto as run args.

@denrase
Copy link
Collaborator

denrase commented Sep 3, 2024

Looks like we have more issues in the Sentry/Screenshot Widget. I created a PR with one guard in #2263, but i don't think that is the solution for the problems outlined in #1881

I will check out your branch and reproduce this.

@denrase
Copy link
Collaborator

denrase commented Sep 3, 2024

Ok, on first sight, as we are supporting multiple view, we also need to move to multiple SentryWidget widgets wrapping those views. This would mean that we cannot use the static global key of SentryWidget anymore, and need to support multiple ones, just like what you have done with the navigator. Played around a bit and this was my starting point:

final GlobalKey<NavigatorState> navigatorKey1 = GlobalKey<NavigatorState>();
final GlobalKey<NavigatorState> navigatorKey2 = GlobalKey<NavigatorState>();

final navigatorKeys = [navigatorKey1, navigatorKey2];

final sentryWidgetKey1 = GlobalKey(debugLabel: 'sentry_screenshot_widget_1');
final sentryWidgetKey2 = GlobalKey(debugLabel: 'sentry_screenshot_widget_2');
final sentryWidgetKeys = [sentryWidgetKey1, sentryWidgetKey2];

Future<void> main() async {
  await setupSentry(
    () => runWidget(
      MultiViewApp(
        viewBuilder: (BuildContext context) => SentryWidget(
          globalKey: sentryWidgetKeys[View.of(context).viewId - 1],
          child: DefaultAssetBundle(
            bundle: SentryAssetBundle(),
            child: const MyApp(),
          ),
        ),
      ),
    ),
    exampleDsn,
  );
}

At least this is my understanding on the limited exposure i just had to multi-view apps. Wdyt?

@martinhaintz
Copy link
Collaborator Author

I tried to make the example app run, with your approach, but ran into the problem, that even if we use multiple globalkeys, the ScreenshotEventProcessor and WidgetEventProcessor need to access the currentContext through the globalKey without knowing which viewId or which globalKey is the correct one. This was currently done with a single GlobalKey.currentContext.

The Main.dart currently looks like this:

final GlobalKey<NavigatorState> navigatorKey1 = GlobalKey<NavigatorState>();
final GlobalKey<NavigatorState> navigatorKey2 = GlobalKey<NavigatorState>();

final navigatorKeys = [navigatorKey1, navigatorKey2];

final sentryWidgetKey1 = GlobalKey(debugLabel: 'sentry_widget_1');
final sentryWidgetKey2 = GlobalKey(debugLabel: 'sentry_widget_2');
final sentryWidgetKeys = [sentryWidgetKey1, sentryWidgetKey2];

final sentryScreenshotWidgetKey1 =
    GlobalKey(debugLabel: 'sentry_screenshot_widget_1');
final sentryScreenshotWidgetKey2 =
    GlobalKey(debugLabel: 'sentry_screenshot_widget_2');
final sentryScreenshotWidgetKeys = [
  sentryScreenshotWidgetKey1,
  sentryScreenshotWidgetKey2
];

Future<void> main() async {
  await setupSentry(
    () => runWidget(
      MultiViewApp(
        viewBuilder: (BuildContext context) => SentryWidget(
          sentryWidgetGlobalKey: sentryWidgetKeys[View.of(context).viewId - 1],
          sentryScreenshotWidgetGlobalKey:
              sentryScreenshotWidgetKeys[View.of(context).viewId - 1],
          child: DefaultAssetBundle(
            bundle: SentryAssetBundle(),
            child: const MyApp(),
          ),
        ),
      ),
    ),
    exampleDsn,
  );
}

ScreenshotEventProcessor needs the currentContext:

  /// This is true when the SentryWidget is in the view hierarchy
  bool get _hasSentryScreenshotWidget =>
      sentryScreenshotWidgetGlobalKey.currentContext != null;

Also WidgetEventProcessor needs the currentContext:

final context = sentryWidgetGlobalKey.currentContext;

To access the current viewId you have to call View.of(context).viewId.

So we would need to save the viewId and or the globalKeys in the SentryWidget and make it accessible. But ScreenshotEventProcessor and WidgetEventProcessor are instantiated in the setupSentry method before the views are even created and then stored in the SentryOptions which is global. So we need to somehow add these values afterward for every view.

@martinhaintz
Copy link
Collaborator Author

martinhaintz commented Sep 9, 2024

Flutter 3.13.0 is the first version the min_version_test build completes successfully. (without error about missing platformView.viewId)
To build the adapted multiview example app, you need at least Flutter 3.22.0

Also, if you don't use the SentryWidget and do not rely on the single global NavigatorKey or find a solution to handle multiple NavigatorKeys, the use of MultiView is working.

Using the current main branch, without refactoring to the platformView another possibility is to disable integrations.add(WidgetsBindingIntegration()); in sentry_flutter.dart. Therefore the deprecated code is not executed and MultiView is also working. This also doesn't affect the backward compatibility. The navigatorKey problem as written in the last paragraph is also valid for this solution.

Future changes should include, the better handling of multiple views and moving away from using single global keys in sentry, which results in problems if you have them multiple times.

@martinhaintz martinhaintz changed the title Flutter multiview support Migration to usage of PlatformView Sep 10, 2024
@martinhaintz
Copy link
Collaborator Author

Also, have a look at the troubleshooting guide here:
PR
Guide

…tsentry/sentry-dart into fix/flutter-multiview-support

* 'fix/flutter-multiview-support' of https://github.com/getsentry/sentry-dart:
  Update CHANGELOG.md
  release: 8.9.0
  chore: rename errorSampleRate to onErrorSampleRate (#2270)
  fix: repost replay screenshots on android while idle (#2275)
  feat: capture touch breadcrumbs for all buttons (#2242)
  Symbolicate Dart stacktrace on Flutter Android and iOS without debug images from native sdks (#2256)
  Fix: Support allowUrls, denyUrls (#2271)
  chore(deps): update Flutter SDK (metrics) to v3.24.2 (#2272)
@buenaflor
Copy link
Contributor

We've decided not to go ahead with this PR as it's too invasive currently to the SDK given the low priority of the new feature (it's not used very much yet)

The current plan is to document the limitation of our features in combination with multiview embedding. See getsentry/sentry-docs#11308

If in the future we want to support this fully we will revisit this PR.

In that regard, I'm closing this now.

@buenaflor buenaflor closed this Sep 10, 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.

Flutter multiview support
3 participants