Auto performance monitoring for widgets#1137
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1137 +/- ##
=======================================
Coverage 90.08% 90.08%
=======================================
Files 119 119
Lines 3650 3672 +22
=======================================
+ Hits 3288 3308 +20
- Misses 362 364 +2 ☔ View full report in Codecov by Sentry. |
ueman
left a comment
There was a problem hiding this comment.
This should be tested with platform views. I.e. it shouldn't prohibit interactions with platform views.
Android Performance metrics 🚀
|
| 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 |
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. |
iOS Performance metrics 🚀
|
| 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 |
gotcha, I recall this from here #706 (comment) |
| return; | ||
| } | ||
|
|
||
| // TODO: name should be screenName.widgetName, maybe get from router? |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
|
||
| UserInteractionWidget? tappedWidget; | ||
|
|
||
| void elementFinder(Element element) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yes, this is a real use case.
I had used this a lot.
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
There is this thing with the elementFinder that is worth taking a look at.
Apart from that, LGTM.
…ntry-dart into feat/widget_instrumentation
📜 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
🔮 Next steps