Auto performance monitoring for widgets

[marandaneto]

📜 Description

Adds a breadcrumb for taps
Starts an Idle transaction for taps

💡 Motivation and Context

Closes #706
Closes #723

💚 How did you test it?

📝 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

[github-actions]

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

Generated by 🚫 dangerJS against c2b2d87

[codecov-commenter]

Codecov Report

Base: 90.08% // Head: 91.00% // Increases project coverage by +0.92% 🎉

Coverage data is based on head (f75aa30) compared to base (873fb42).
Patch has no changes to coverable lines.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1137      +/-   ##
==========================================
+ Coverage   90.08%   91.00%   +0.92%     
==========================================
  Files         119        9     -110     
  Lines        3650      189    -3461     
==========================================
- Hits         3288      172    -3116     
+ Misses        362       17     -345     

Impacted Files	Coverage Δ
dart/lib/src/noop_sentry_span.dart
dart/lib/src/protocol/breadcrumb.dart
dart/lib/src/protocol/sentry_span.dart
dart/lib/src/sentry_options.dart
dart/lib/src/sentry_span_interface.dart
dart/lib/src/sentry_tracer.dart
dart/lib/src/protocol/sentry_geo.dart
dart/lib/src/sentry_span_context.dart
dart/lib/src/sentry_baggage.dart
...ib/src/environment/_web_environment_variables.dart
... and 100 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.

[ueman]

This should be tested with platform views. I.e. it shouldn't prohibit interactions with platform views.

[github-actions]

Android Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	350.77 ms	412.59 ms	61.83 ms
Size	5.94 MiB	6.96 MiB	1.02 MiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
f922f8f	332.31 ms	374.67 ms	42.37 ms
4efee31	308.92 ms	368.68 ms	59.76 ms
cdf7172	348.54 ms	390.81 ms	42.27 ms
eb1a7c1	332.98 ms	381.55 ms	48.57 ms
d7758e8	300.12 ms	349.88 ms	49.76 ms
aed5947	295.70 ms	348.18 ms	52.48 ms
f4cc744	349.53 ms	394.68 ms	45.15 ms
6d7a391	331.94 ms	367.04 ms	35.10 ms
633cf2e	289.36 ms	340.38 ms	51.02 ms
a7acb24	301.00 ms	357.38 ms	56.38 ms

App size

Revision	Plain	With Sentry	Diff
f922f8f	5.94 MiB	6.95 MiB	1.01 MiB
4efee31	5.94 MiB	6.92 MiB	1003.76 KiB
cdf7172	5.94 MiB	6.95 MiB	1.01 MiB
eb1a7c1	5.94 MiB	6.92 MiB	1005.76 KiB
d7758e8	5.94 MiB	6.95 MiB	1.01 MiB
aed5947	5.94 MiB	6.96 MiB	1.02 MiB
f4cc744	5.94 MiB	6.95 MiB	1.01 MiB
6d7a391	5.94 MiB	6.95 MiB	1.01 MiB
633cf2e	5.94 MiB	6.92 MiB	1001.53 KiB
a7acb24	5.94 MiB	6.95 MiB	1.01 MiB

Previous results on branch: feat/widget_instrumentation

Startup times

Revision	Plain	With Sentry	Diff
4328c3f	355.10 ms	394.71 ms	39.61 ms
d0ac278	311.08 ms	370.94 ms	59.85 ms
9d13808	346.46 ms	410.90 ms	64.44 ms
f75aa30	385.86 ms	446.16 ms	60.30 ms
af50b36	355.04 ms	449.56 ms	94.52 ms
640ade3	328.65 ms	368.76 ms	40.11 ms
31efee2	346.06 ms	375.71 ms	29.65 ms
5e59d38	302.77 ms	351.23 ms	48.46 ms

App size

Revision	Plain	With Sentry	Diff
4328c3f	5.94 MiB	6.96 MiB	1.02 MiB
d0ac278	5.94 MiB	6.96 MiB	1.02 MiB
9d13808	5.94 MiB	6.96 MiB	1.02 MiB
f75aa30	5.94 MiB	6.96 MiB	1.02 MiB
af50b36	5.94 MiB	6.96 MiB	1.02 MiB
640ade3	5.94 MiB	6.95 MiB	1.01 MiB
31efee2	5.94 MiB	6.96 MiB	1.02 MiB
5e59d38	5.94 MiB	6.96 MiB	1.02 MiB

[marandaneto]

This should be tested with platform views. I.e. it shouldn't prohibit interactions with platform views.

What do you mean by platform view, this?

[ueman]

This should be tested with platform views. I.e. it shouldn't prohibit interactions with platform views.

What do you mean by platform view, this?

Yeah, it can be tested by using one of the webview plugins, google maps or the camera plugin. The reason is that I've seen global click listeners prohibiting interactions with those.

[github-actions]

iOS Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1245.56 ms	1266.92 ms	21.36 ms
Size	8.16 MiB	9.17 MiB	1.01 MiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
9f9f94f	1268.33 ms	1284.73 ms	16.41 ms
f4cc744	1274.57 ms	1290.79 ms	16.22 ms
7ade5af	1296.24 ms	1297.43 ms	1.18 ms
49a149b	1296.47 ms	1320.20 ms	23.73 ms
0db91cc	1267.63 ms	1279.69 ms	12.06 ms
f922f8f	1249.53 ms	1266.51 ms	16.98 ms
21845e2	1279.37 ms	1298.81 ms	19.45 ms
2331d89	1260.86 ms	1281.24 ms	20.39 ms
3e9fb0e	1262.49 ms	1280.65 ms	18.16 ms
25e9b59	1289.76 ms	1295.27 ms	5.51 ms

App size

Revision	Plain	With Sentry	Diff
9f9f94f	8.15 MiB	9.15 MiB	1020.76 KiB
f4cc744	8.16 MiB	9.16 MiB	1.01 MiB
7ade5af	8.15 MiB	9.15 MiB	1015.93 KiB
49a149b	8.15 MiB	9.12 MiB	986.26 KiB
0db91cc	8.15 MiB	9.15 MiB	1018.56 KiB
f922f8f	8.15 MiB	9.13 MiB	1003.20 KiB
21845e2	8.15 MiB	9.12 MiB	991.34 KiB
2331d89	8.16 MiB	9.17 MiB	1.01 MiB
3e9fb0e	8.15 MiB	9.12 MiB	989.77 KiB
25e9b59	8.16 MiB	9.15 MiB	1021.15 KiB

Previous results on branch: feat/widget_instrumentation

Startup times

Revision	Plain	With Sentry	Diff
9d13808	1278.42 ms	1287.31 ms	8.89 ms
af50b36	1270.88 ms	1299.10 ms	28.22 ms
640ade3	1266.26 ms	1290.06 ms	23.80 ms
31efee2	1267.02 ms	1300.55 ms	33.53 ms
5e59d38	1258.96 ms	1292.49 ms	33.53 ms
d0ac278	1252.29 ms	1277.10 ms	24.82 ms
4328c3f	1268.45 ms	1293.55 ms	25.10 ms

App size

Revision	Plain	With Sentry	Diff
9d13808	8.16 MiB	9.17 MiB	1.01 MiB
af50b36	8.16 MiB	9.17 MiB	1.01 MiB
640ade3	8.16 MiB	9.15 MiB	1021.93 KiB
31efee2	8.16 MiB	9.17 MiB	1.01 MiB
5e59d38	8.16 MiB	9.17 MiB	1.01 MiB
d0ac278	8.16 MiB	9.17 MiB	1.01 MiB
4328c3f	8.16 MiB	9.17 MiB	1.01 MiB

[marandaneto]

This should be tested with platform views. I.e. it shouldn't prohibit interactions with platform views.

What do you mean by platform view, this?

Yeah, it can be tested by using one of the webview plugins, google maps or the camera plugin. The reason is that I've seen global click listeners prohibiting interactions with those.

gotcha, I recall this from here #706 (comment)
Will do, thanks for the reminder.

[marandaneto]

@ueman is it possible to find out which screen (maybe the root Scaffold?) a widget belongs to?
For example, MainScaffold.btn_login, I know that the btn_login comes from the MainScaffold screen.
I could not find any way that really makes sense.

[ueman]

If you have the context of the widget, you can do ModalRoute.of(context).settings.name;.

You have to track which StatefulElement is the closest to the widget which gets tracked. Via that StatefulElement, you can do ModalRoute.of(statefulElement.state.context).settings.name.

So in _getDescriptionFrom you can do something like

TappedWidget? _getDescriptionFrom(Element element) {
    String? currentRouteName;
    // Add the following lines
    final widget = element.widget;
    if(element is StatefulElement) {
      currentRouteName = ModalRoute.of(statefulElement.state.context).settings.name;
    }
    // Used by ElevatedButton, TextButton, OutlinedButton.
    if (widget is ButtonStyleButton) {
      if (widget.enabled) {
        return TappedWidget(
          element: element,
          description: _findDescriptionOf(element, true),
          type: 'ButtonStyleButton',
        );
      }
    } else if (widget is MaterialButton) {
      if (widget.enabled) {
        return TappedWidget(
          element: element,
          description: _findDescriptionOf(element, true),
          type: 'MaterialButton',
        );
      }
    } else if (widget is CupertinoButton) {
      if (widget.enabled) {
        return TappedWidget(
          element: element,
          description: _findDescriptionOf(element, true),
          type: 'CupertinoButton',
        );
      }
    } else if (widget is InkWell) {
      if (widget.onTap != null) {
        return TappedWidget(
          element: element,
          description: _findDescriptionOf(element, false),
          type: 'InkWell',
        );
      }
    } else if (widget is IconButton) {
      if (widget.onPressed != null) {
        return TappedWidget(
          element: element,
          description: _findDescriptionOf(element, false),
          type: 'IconButton',
        );
      }
    } else if (widget is GestureDetector) {
      if (widget.onTap != null) {
        return TappedWidget(
          element: element,
          description: '',
          type: 'GestureDetector',
        );
      }
    }

    return null;
  }

No clue about the performance impact of calling ModalRoute.of for each StatefulElement but it could be a bit too much. Better to optimize it :D

[marandaneto]

Yeah and then it works only for StatefulElement as well, StatelessElement elements don't have a state.
Maybe I will mention in the docs for now that the key must be unique across screens as well.
The other option is if I make it work along with SentryNavigatorObserver, because SentryNavigatorObserver always knows what's the current route if the observer is installed.

[ueman]

True, but I would bet that it's basically guaranteed that there's one, though.

I wouldn't use SentryNavigatorObserver tbh, because it's common to have multiple navigators and then you have to figure out which one is the navigator to get the current route from

[brustolin]

h: Consider this:

Card(
                child: GestureDetector(
                  onTap: () => {
                    //open Card
                  },
                  child: Stack(
                    children: [
                      //fancy card layout
                      ElevatedButton(
                        onPressed: () => {
                          //mark card as favorite
                        },
                        child: const Text('Favorite'),
                      ),
                    ],
                  ),
                ),
              )

If a user press the "Favorite" ElevatedButton, the element found will always be the GestureDetector.
I guess this will be a problem to really detect a potential problem.

[brustolin]

It's kind ok to keep this as it is. Since UI Transactions are meant to a bag for spans. Maybe we're ok to not have the perfect origin of a transaction as long we have a transaction.

But if you can figure it out how to solve this would be nice.

[marandaneto]

Very good catch, the elementFinder always goes to the GestureDetector.
@ueman any ideas what to do here? since we don't know the instance of the onTap or onPressed to compare, I don't know if we can solve it.
@brustolin The question is, does this even make sense? Having a parent onTap and an onPressed child? Is this a real use case? Flutter always executes the onPressed from the ElevatedButton, always ignores the onTap from GestureDetector, maybe we should just go till the very end of the tree and use the last found child?

[brustolin]

Yes, this is a real use case.
I had used this a lot.

[ueman]

@ueman any ideas what to do here? since we don't know the instance of the onTap or onPressed to compare, I don't know if we can solve it.

maybe we should just go till the very end of the tree and use the last found child?

Yeah, maybe don't stop searching on the first hit, but for the smallest hit?

Maybe there's a way to get some information from the gesture arena (which is the mechanism which decides which widget was tapped)? There's some information here https://docs.flutter.dev/development/ui/advanced/gestures#gesture-disambiguation

[marandaneto]

It turns out that going to the latest level of child is also wrong, because it ends up with an InkWell which is a child of the desired ElevatedButton.
Will do a bit of research.

[marandaneto]

For now, I dropped support for the nested GestureDetector so the PR is unblocked, I will keep searching on how to apply the gesture disambiguation
https://docs.flutter.dev/development/ui/advanced/gestures#gesture-disambiguation

[brustolin]

There is this thing with the elementFinder that is worth taking a look at.
Apart from that, LGTM.