search

getsentry / sentry-dart

Sentry SDK for Dart and Flutter
https://sentry.io/for/flutter/
MIT License
754 stars 237 forks source link

Migration to usage of PlatformView #2249

Closed martinhaintz closed 2 months ago

martinhaintz commented 2 months ago

:scroll: 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

:bulb: Motivation and Context

close #2225

:green_heart: How did you test it?

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

:pencil: Checklist

:crystal_ball: Next steps

github-actions[bot] commented 2 months ago
Fails
:no_entry_sign: 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 :no_entry_sign: dangerJS against 3fb08ac0d38beda4640fd7a67d52a3321e1a98e0

codecov[bot] commented 2 months ago

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 :warning:
...nt_processor/flutter_enricher_event_processor.dart 90.24% 4 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

github-actions[bot] commented 2 months ago

Android Performance metrics :rocket:

  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
64af39c26c91cb82e402c65852a0e6abdd890f53 386.80 ms 471.11 ms 84.31 ms
0ceb89ca0b313636a4afdb0f3171e26d89b705e2 304.57 ms 357.18 ms 52.61 ms
be08ed1118c3890fabac3197bcc4e9650145c8a8 361.37 ms 414.84 ms 53.47 ms
3f23617e3346caa128c5f27b9c935de4974c3067 385.77 ms 476.10 ms 90.33 ms
42f6e7e6bf2882e21c1f664a1a6c8277ab07ad3b 308.71 ms 360.06 ms 51.35 ms
c70e01a14827d9a624c0002c84fafbdef163f976 331.04 ms 401.46 ms 70.42 ms
9d7e8623dbcfd7498c1cf1bcf5ba4c8b3e99a860 426.35 ms 510.88 ms 84.53 ms
7f75f32d9707c5a5abdb869a9eb5de05dd91bed8 347.36 ms 419.58 ms 72.22 ms
86d48419a55b5e88a17d31b0574e7e2974d4c18b 286.35 ms 372.43 ms 86.08 ms
3637a220905894d7ad621e9b2324a045e5a1979f 322.59 ms 390.00 ms 67.41 ms

App size

Revision Plain With Sentry Diff
64af39c26c91cb82e402c65852a0e6abdd890f53 6.27 MiB 7.20 MiB 958.83 KiB
0ceb89ca0b313636a4afdb0f3171e26d89b705e2 5.94 MiB 6.95 MiB 1.01 MiB
be08ed1118c3890fabac3197bcc4e9650145c8a8 6.33 MiB 7.26 MiB 946.42 KiB
3f23617e3346caa128c5f27b9c935de4974c3067 5.94 MiB 6.96 MiB 1.02 MiB
42f6e7e6bf2882e21c1f664a1a6c8277ab07ad3b 6.27 MiB 7.20 MiB 956.06 KiB
c70e01a14827d9a624c0002c84fafbdef163f976 5.94 MiB 6.97 MiB 1.03 MiB
9d7e8623dbcfd7498c1cf1bcf5ba4c8b3e99a860 6.33 MiB 7.26 MiB 943.41 KiB
7f75f32d9707c5a5abdb869a9eb5de05dd91bed8 6.26 MiB 7.20 MiB 959.18 KiB
86d48419a55b5e88a17d31b0574e7e2974d4c18b 6.15 MiB 7.13 MiB 1000.49 KiB
3637a220905894d7ad621e9b2324a045e5a1979f 6.06 MiB 7.09 MiB 1.03 MiB

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

Startup times

Revision Plain With Sentry Diff
28869566f828c50568fe6a3ce5e06defddf0d55e 453.89 ms 504.72 ms 50.83 ms
e4499413ff83bb448ba706c9349fffee004edb01 404.27 ms 447.04 ms 42.77 ms
7f9b83f366e7edb4a7a49d345c20209e4f2f3cf3 436.02 ms 463.04 ms 27.02 ms
1b757534ea98c7fe79ea19e4674b9a69819bfd9a 471.22 ms 543.42 ms 72.20 ms
6883e29d230640ca752b0c93341b5897f2ab55d4 445.52 ms 484.45 ms 38.93 ms
221dadd56b3dc0ddef3dba31a8dd1fc9076d130d 721.34 ms 773.37 ms 52.03 ms
cb75ff1d8f623aa9bd7c3249bc5e62fa704cd464 410.88 ms 469.84 ms 58.96 ms
4e694861ab7dde70079baacb9c728d0507460fcd 428.79 ms 474.69 ms 45.90 ms
7d14655528c4831d187077543e1d57afe1e80f2c 419.79 ms 457.67 ms 37.88 ms
1396dfb170833ac514fea28d933376532fa7e116 475.13 ms 511.56 ms 36.43 ms

App size

Revision Plain With Sentry Diff
28869566f828c50568fe6a3ce5e06defddf0d55e 6.52 MiB 7.61 MiB 1.08 MiB
e4499413ff83bb448ba706c9349fffee004edb01 6.52 MiB 7.58 MiB 1.06 MiB
7f9b83f366e7edb4a7a49d345c20209e4f2f3cf3 6.52 MiB 7.61 MiB 1.08 MiB
1b757534ea98c7fe79ea19e4674b9a69819bfd9a 6.52 MiB 7.61 MiB 1.08 MiB
6883e29d230640ca752b0c93341b5897f2ab55d4 6.49 MiB 7.55 MiB 1.06 MiB
221dadd56b3dc0ddef3dba31a8dd1fc9076d130d 6.52 MiB 7.61 MiB 1.08 MiB
cb75ff1d8f623aa9bd7c3249bc5e62fa704cd464 6.52 MiB 7.61 MiB 1.08 MiB
4e694861ab7dde70079baacb9c728d0507460fcd 6.52 MiB 7.61 MiB 1.08 MiB
7d14655528c4831d187077543e1d57afe1e80f2c 6.52 MiB 7.61 MiB 1.08 MiB
1396dfb170833ac514fea28d933376532fa7e116 6.52 MiB 7.58 MiB 1.06 MiB
github-actions[bot] commented 2 months ago

iOS Performance metrics :rocket:

  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
8776cdf4688ec58628a4b68a24f5d8840f2b523f 1237.64 ms 1246.83 ms 9.20 ms
f79eecfc96c81e45b312c72aea32e4943329666b 1210.25 ms 1221.65 ms 11.40 ms
abfcdb5365060597e8a7479eafb93c0cfb6a210b 1230.87 ms 1244.94 ms 14.06 ms
9d7e8623dbcfd7498c1cf1bcf5ba4c8b3e99a860 1236.88 ms 1258.62 ms 21.74 ms
7e7f0b1ba00d87ca0c564c0aee2b80b9e66a8253 1230.52 ms 1251.49 ms 20.97 ms
3d305b96e5469a711cd2c67191b28bbcd997ca41 1230.29 ms 1247.39 ms 17.10 ms
1596141c018a1029c280be2e0a0b2cd4849239ec 1230.77 ms 1241.90 ms 11.13 ms
d4d08071f7cfa6a1fb4b2a27151bb30a36d607f8 1246.94 ms 1260.69 ms 13.75 ms
051e97a45b67f5c431e8c540585c1786c42670f9 1245.94 ms 1249.51 ms 3.57 ms
d089990aa16ee19071abb70d6bab8231c452fc13 1206.19 ms 1233.08 ms 26.89 ms

App size

Revision Plain With Sentry Diff
8776cdf4688ec58628a4b68a24f5d8840f2b523f 8.33 MiB 9.40 MiB 1.07 MiB
f79eecfc96c81e45b312c72aea32e4943329666b 8.29 MiB 9.36 MiB 1.07 MiB
abfcdb5365060597e8a7479eafb93c0cfb6a210b 8.33 MiB 9.64 MiB 1.31 MiB
9d7e8623dbcfd7498c1cf1bcf5ba4c8b3e99a860 8.32 MiB 9.38 MiB 1.05 MiB
7e7f0b1ba00d87ca0c564c0aee2b80b9e66a8253 8.33 MiB 9.61 MiB 1.27 MiB
3d305b96e5469a711cd2c67191b28bbcd997ca41 8.33 MiB 9.63 MiB 1.29 MiB
1596141c018a1029c280be2e0a0b2cd4849239ec 8.29 MiB 9.37 MiB 1.08 MiB
d4d08071f7cfa6a1fb4b2a27151bb30a36d607f8 8.33 MiB 9.64 MiB 1.31 MiB
051e97a45b67f5c431e8c540585c1786c42670f9 8.28 MiB 9.34 MiB 1.06 MiB
d089990aa16ee19071abb70d6bab8231c452fc13 8.33 MiB 9.40 MiB 1.07 MiB

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

Startup times

Revision Plain With Sentry Diff
28869566f828c50568fe6a3ce5e06defddf0d55e 1241.90 ms 1266.65 ms 24.75 ms
7f9b83f366e7edb4a7a49d345c20209e4f2f3cf3 1249.25 ms 1272.88 ms 23.63 ms
7d14655528c4831d187077543e1d57afe1e80f2c 1243.18 ms 1273.00 ms 29.82 ms
4e694861ab7dde70079baacb9c728d0507460fcd 1237.12 ms 1260.67 ms 23.55 ms
1396dfb170833ac514fea28d933376532fa7e116 1227.92 ms 1244.87 ms 16.95 ms
221dadd56b3dc0ddef3dba31a8dd1fc9076d130d 1240.65 ms 1261.70 ms 21.06 ms
e4499413ff83bb448ba706c9349fffee004edb01 1229.31 ms 1239.63 ms 10.32 ms
1b757534ea98c7fe79ea19e4674b9a69819bfd9a 1245.48 ms 1270.96 ms 25.48 ms
6883e29d230640ca752b0c93341b5897f2ab55d4 1236.39 ms 1277.02 ms 40.63 ms
cb75ff1d8f623aa9bd7c3249bc5e62fa704cd464 1239.06 ms 1268.98 ms 29.92 ms

App size

Revision Plain With Sentry Diff
28869566f828c50568fe6a3ce5e06defddf0d55e 8.38 MiB 9.73 MiB 1.35 MiB
7f9b83f366e7edb4a7a49d345c20209e4f2f3cf3 8.38 MiB 9.73 MiB 1.35 MiB
7d14655528c4831d187077543e1d57afe1e80f2c 8.38 MiB 9.73 MiB 1.35 MiB
4e694861ab7dde70079baacb9c728d0507460fcd 8.38 MiB 9.73 MiB 1.35 MiB
1396dfb170833ac514fea28d933376532fa7e116 8.38 MiB 9.71 MiB 1.34 MiB
221dadd56b3dc0ddef3dba31a8dd1fc9076d130d 8.38 MiB 9.73 MiB 1.35 MiB
e4499413ff83bb448ba706c9349fffee004edb01 8.38 MiB 9.71 MiB 1.34 MiB
1b757534ea98c7fe79ea19e4674b9a69819bfd9a 8.38 MiB 9.73 MiB 1.35 MiB
6883e29d230640ca752b0c93341b5897f2ab55d4 8.38 MiB 9.73 MiB 1.35 MiB
cb75ff1d8f623aa9bd7c3249bc5e62fa704cd464 8.38 MiB 9.73 MiB 1.35 MiB
martinhaintz commented 2 months ago

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 commented 2 months ago

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 https://github.com/getsentry/sentry-dart/issues/1881

I will check out your branch and reproduce this.

denrase commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

buenaflor commented 2 months ago

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 https://github.com/getsentry/sentry-docs/pull/11308

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

In that regard, I'm closing this now.