Skip to content

Auto performance monitoring for widgets#1137

Merged
marandaneto merged 20 commits into
mainfrom
feat/widget_instrumentation
Nov 25, 2022
Merged

Auto performance monitoring for widgets#1137
marandaneto merged 20 commits into
mainfrom
feat/widget_instrumentation

Conversation

@marandaneto

@marandaneto marandaneto commented Nov 16, 2022

Copy link
Copy Markdown
Contributor

📜 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

@marandaneto marandaneto changed the title Widget instrumentation draft Auto performance monitoring for widgets Nov 16, 2022
Comment thread flutter/example/lib/main.dart Outdated
@github-actions

github-actions Bot commented Nov 16, 2022

Copy link
Copy Markdown
Contributor
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-commenter commented Nov 16, 2022

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.08%. Comparing base (873fb42) to head (c2b2d87).
⚠️ Report is 1345 commits behind head on main.

Files with missing lines Patch % Lines
dart/lib/src/noop_sentry_span.dart 0.00% 1 Missing ⚠️
dart/lib/src/protocol/breadcrumb.dart 83.33% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

Comment thread flutter/lib/src/sentry_user_interaction_widget.dart Outdated

@ueman ueman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@github-actions

github-actions Bot commented Nov 16, 2022

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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

ueman commented Nov 16, 2022

Copy link
Copy Markdown
Collaborator

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

github-actions Bot commented Nov 16, 2022

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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.

return;
}

// TODO: name should be screenName.widgetName, maybe get from router?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@marandaneto
marandaneto marked this pull request as ready for review November 23, 2022 14:41
Comment thread dart/lib/src/sentry_tracer.dart

UserInteractionWidget? tappedWidget;

void elementFinder(Element element) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 brustolin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

Breadcrumbs for widgets Auto performance monitoring for widgets

4 participants