fix(MAUI): Automatically captured breadcrumbs #2900

bitsandfoxes · 2023-11-24T18:36:40Z

Fixes #2804
Resolves #2827

This PR does a couple of things:

It completely removes the binding to events via reflection. It would not work with AOT anyway. But the red flags are the unintended side-effects (see Rendering UI issue when Sentry SDK is enabled #2804) where the SDK seems to consume third-party events.
Breadcrumbs are incredibly noisy right now and do not seem to offer any value. Our bare sample app created 94 breadcrumbs. I trimmed them down to the most essential ones - Navigation, Buttons, Lifecycle - and opted to stick with the finalizing ones for the "duplicates. (i.e. Pushing vs Pushed).
It also sets up unbinding upon element removal to prevent memory leaks

New:

"message": "App.ChildAdded",
"message": "AppShell.Appearing"
"message": "App.PageAppearing",
"message": "Window.Created"
"message": "App.PageAppearing",
"message": "AppShell.Navigated",
"message": "ShellContent.ChildAdded",
"message": "ShellContent.ChildAdded",
"message": "MainPage.LayoutChanged"
"message": "Window.Activated"
"message": "Button.Pressed",
"message": "Button.Released",
"message": "The button has been clicked 1 times"
"message": "Button.Clicked",
"message": "Button.Pressed",
"message": "Button.Released",

Old:

"App.HandlerChanging",
"App.HandlerChanged",
"App.ChildAdded",
"AppShell.Appearing"
"App.PageAppearing",
"Window.HandlerChanging",
"AppShell.HandlerChanging",
"AppShell.HandlerChanged",
"AppShell.MeasureInvalidated"
"Window.HandlerChanged",
"Window.Created"
"AppShell.Loaded"
"AppShell.SizeChanged"
"ShellContent.Appearing"
"App.PageAppearing",
"AppShell.Navigated",
"ShellContent.ChildAdded",
"MainPage.HandlerChanging",
"ScrollView.HandlerChanging",
"VerticalStackLayout.HandlerChanging",
"Image.HandlerChanging",
"Image.HandlerChanged",
"Image.MeasureInvalidated"
"Label.HandlerChanging",
"Label.HandlerChanged",
"Label.MeasureInvalidated"
"Label.HandlerChanging",
"Label.HandlerChanged",
"Label.MeasureInvalidated"
"Button.HandlerChanging",
"Button.HandlerChanged",
"Button.MeasureInvalidated",
"Button.HandlerChanging",
"Button.HandlerChanged",
"Button.MeasureInvalidated",
"Button.HandlerChanging",
"Button.HandlerChanged",
"Button.MeasureInvalidated",
"Button.HandlerChanging",
"Button.HandlerChanged",
"Button.MeasureInvalidated",
"Button.HandlerChanging",
"Button.HandlerChanged",
"Button.MeasureInvalidated",
"Button.HandlerChanging",
"Button.HandlerChanged",
"Button.MeasureInvalidated",
"VerticalStackLayout.HandlerChanged",
"ScrollView.MeasureInvalidated"
"VerticalStackLayout.MeasureInvalidated"
"ScrollView.HandlerChanged",
"MainPage.MeasureInvalidated"
"ScrollView.MeasureInvalidated"
"MainPage.HandlerChanged",
"MainPage.MeasureInvalidated"
"Image.Loaded"
"Label.Loaded"
"Label.Loaded"
"Button.Loaded",
"Button.Loaded",
"Button.Loaded",
"Button.Loaded",
"Button.Loaded",
"Button.Loaded",
"VerticalStackLayout.Loaded"
"ScrollView.Loaded"
"MainPage.Loaded"
"ShellContent.ChildAdded",
"Image.Loaded"
"Label.Loaded"
"Grid.Loaded"
"Grid.SizeChanged"
"Image.SizeChanged"
"Label.SizeChanged"
"ScrollView.SizeChanged"
"MainPage.LayoutChanged"
"MainPage.SizeChanged"
"VerticalStackLayout.SizeChanged"
"Image.SizeChanged"
"Label.SizeChanged"
"Label.SizeChanged"
"Button.SizeChanged",
"Button.SizeChanged",
"Button.SizeChanged",
"Button.SizeChanged",
"Button.SizeChanged",
"Window.Activated"
"Button.Pressed",
"Button.Released",
"Button.MeasureInvalidated",
"The button has been clicked 1 times"
"Button.Clicked",
"Button.SizeChanged",
"Button.Pressed",
"Button.Released",

src/Sentry.Maui/Internal/MauiEventsBinder.cs

CHANGELOG.md

samples/Sentry.Samples.Maui/MainPage.xaml.cs

Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>

samples/Sentry.Samples.Maui/MainPage.xaml.cs

jamescrosswell · 2023-11-28T21:53:55Z

Delete fest! 🎉

Redth · 2023-11-28T22:13:42Z

Really good start to helping performance.

I'd like to suggest also looking at either removing, or making opt-in, everything in BindElementEvents. Profiling my virtual listview control usage in an app, it looks like a lot of time is spent dealing with adding breadcrumbs in the element.BindingContextChanged handler. When you're scrolling through a list quickly and views are being recycled, the binding context is constantly being swapped out as you scroll. This adds a lot of unnessary churn/overhead for what I tend to think is little value from a logging/breadcrumb perspective.

Could these be options in the extension configuration to opt-out (or opt-in if they are default opt-out) to wiring up Element events? Or, perhaps another delegate similar to the BeforeBreadcrumb one to help decide if a particular element should be wired up or not?

bruno-garcia · 2023-11-29T02:25:27Z

samples/Sentry.Samples.Maui/MainPage.xaml.cs

+        {
+            if (visualElement is Element element)
+            {
+                element.HandlerChanged += (sender, _) =>


We're subscribing to events here but not unsubscribing anywhere. This is often a memory leak source.

Here's some tips on looking for leaks: https://github.com/dotnet/maui/wiki/Memory-Leaks

#2919 (comment) this

bruno-garcia · 2023-11-29T02:25:40Z

samples/Sentry.Samples.Maui/MainPage.xaml.cs

+                        var type = element.Handler?.GetType();
+                        var handlerName = type is not null ? type.Name : string.Empty;
+
+                        SentrySdk.AddBreadcrumb($"'{elementName}' handler changed to '{handlerName}'", "system", "ui.handlers");


I see we're accessing SentrySdk (static method) so not really testable (usually we'd take IHub ?)

bitsandfoxes · 2023-11-29T11:37:28Z

Could these be options in the extension configuration to opt-out (or opt-in if they are default opt-out) to wiring up Element events?

Definitely! Thanks for the feedback. I was just looking for even more reasons to trim them down!

Redth

Thanks for incorporating my feedback :)

bruno-garcia · 2023-11-29T20:33:10Z

src/Sentry.Maui/Internal/MauiEvents.cs

+    public static void OnWindowOnBackgrounding(object? sender, BackgroundingEventArgs e) =>
+        Hub.AddBreadcrumbForEvent(Options, sender, nameof(Window.Backgrounding), SystemType, LifecycleCategory, data =>
+        {
+            if (!Options?.IncludeBackgroundingStateInBreadcrumbs ?? true)


default is null so we're actually defaulting to true here? that'd be nice

samples/Sentry.Samples.Maui/MainPage.xaml.cs

src/Sentry.Maui/BindableSentryMauiOptions.cs

src/Sentry.Maui/Internal/MauiEvents.cs

src/Sentry.Maui/Internal/MauiEventsBinder.cs

bruno-garcia · 2023-11-29T23:47:52Z

src/Sentry.Maui/Internal/MauiEventsBinder.cs

    }

-    public void BindApplicationEvents(Application application)
+    public void HandleApplicationEvents(Application application, bool bind = true)


Where do we call this with bind=false? Looking at when do we unregister events. I imagine when the object we subscribed from is done with we need to do that?

There are two places:

We hook in the lifecycle events in src/Sentry.Maui/SentryMauiAppBuilderExtensions.cs and remove ourselves on shutdown/terminate

The application has application.DescendantRemoved += OnApplicationOnDescendantRemoved; and we crawl through all the events we bound to and remove ourselves.

bruno-garcia

Nice! With a feature of no leaks! :)

Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>

bruno-garcia · 2023-12-01T18:07:10Z

This seems ready to go but CI stuck on some xharness error?

bruno-garcia · 2023-12-01T18:07:50Z

/usr/bin/sh -c pwsh scripts/device-test.ps1 android -Run
INFO    | Boot completed in 28075 ms
INFO    | Increasing screen off timeout, logcat buffer size to 2M.
Skipping NuGet package signature verification.
You can invoke the tool using the following command: xharness
Tool 'microsoft.dotnet.xharness.cli' (version '1.0.0-prerelease.23252.4') was successfully installed.
[1.0.0-prerelease.23252.4+d4d9f33ac28fe4680c3c4d12ce69a1a44d6f6b20] XHarness command issued: android test --app bin/io.sentry.dotnet.maui.device.testapp-Signed.apk --package-name io.sentry.dotnet.maui.device.testapp --output-directory=test_output
info: Will attempt to find device supporting architectures: 'x86_64'
info: Finding attached devices/emulators...
info: Active Android device set to serial 'emulator-5554'
info: Waiting for device to be available (max 5 minutes)
info: Attempting to remove apk 'io.sentry.dotnet.maui.device.testapp'..
info: APK 'io.sentry.dotnet.maui.device.testapp' was not on device
info: Attempting to install /home/runner/work/sentry-dotnet/sentry-dotnet/bin/io.sentry.dotnet.maui.device.testapp-Signed.apk
info: Successfully installed /home/runner/work/sentry-dotnet/sentry-dotnet/bin/io.sentry.dotnet.maui.device.testapp-Signed.apk
info: Killing all running processes for 'io.sentry.dotnet.maui.device.testapp': 
info: Starting default instrumentation class on io.sentry.dotnet.maui.device.testapp (exit code 0 == success)
info: Running instrumentation class {default} took 14.5516671 seconds
info: Found XML result file: '/storage/emulated/0/Download/io.sentry.dotnet.maui.device.testapp/1a5516b1b5a14c779257c121851d6501/TestResults.xml'(key: test-results-path)
info: Attempting to pull contents of /storage/emulated/0/Download/io.sentry.dotnet.maui.device.testapp/1a5516b1b5a14c779257c121851d6501/TestResults.xml to /home/runner/work/sentry-dotnet/sentry-dotnet/test_output
E1201 17:45:49.910855    2939 FrameBuffer.cpp:3765] Failed to find ColorBuffer:60
info: Test execution summary:
      Tests run: 1590 Passed: [158](https://github.com/getsentry/sentry-dotnet/actions/runs/7063255444/job/19228957446?pr=2900#step:8:162)0 Inconclusive: 0 Failed: 1 Ignored: 9
info: Instrumentation finished normally with exit code 1
info: Wrote current ADB log to /home/runner/work/sentry-dotnet/sentry-dotnet/test_output/adb-logcat-io.sentry.dotnet.maui.device.testapp-default.log
fail: Non-success instrumentation exit code: 1, expected: 0
info: Attempting to remove apk 'io.sentry.dotnet.maui.device.testapp'..
info: Successfully uninstalled io.sentry.dotnet.maui.device.testapp
XHarness exit code: 1 (TESTS_FAILED)
Exception: /home/runner/work/sentry-dotnet/sentry-dotnet/scripts/device-test.ps1:73
Line |
  73 |                  throw "xharness run failed with non-zero exit code"
     |                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | xharness run failed with non-zero exit code

Error: The process '/usr/bin/sh' failed with exit code 1
Terminate Emulator
  /usr/local/lib/android/sdk/platform-tools/adb -s emulator-5554 emu kill
  OK: killing emulator, bye bye
  OK
  INFO    | Wait for emulator (pid 2785) 20 seconds to shutdown gracefully before kill;you can set environment variable ANDROID_EMULATOR_WAIT_TIME_BEFORE_KILL(in seconds) to change the default value (20 seconds)
INFO    | Discarding the changed state: command-line flag
WARNING | Discarding the changed state (command-line flag).
ERROR   | stop: Not implemented

@vaind is this something you've seen before?

vaind · 2023-12-01T18:23:47Z

@vaind is this something you've seen before?

nothing wrong with xharness, it's a test failure, says so in the log:

Tests run: 1590 Passed: 1580 Inconclusive: 0 Failed: 1 Ignored: 9

Specifically Sentry.Maui.Tests.SentryMauiAppBuilderExtensionsTests.UseSentry_BindsToApplicationStartupEvent_Android https://github.com/getsentry/sentry-dotnet/actions/runs/7063255444?pr=2900

bitsandfoxes · 2023-12-01T18:48:06Z

Yes. I'm crawling through the logs and fixing the tests.

bruno-garcia · 2023-12-01T23:14:16Z

Oh sorry Ivan! Thanks

removed 'BindReflect'

caaccd6

jamescrosswell reviewed Nov 26, 2023

View reviewed changes

src/Sentry.Maui/Internal/MauiEventsBinder.cs Show resolved Hide resolved

This was referenced Nov 27, 2023

Rendering UI issue when Sentry SDK is enabled #2804

Closed

Automatically trigger all events in the application when using the Sentry. #2827

Closed

bitsandfoxes added 7 commits November 28, 2023 12:41

merged main

adeae7d

decluttering

b54a651

tests

b1c48c1

updated the samples with the handler breadcrumbs

6842e95

.

e9df311

Merge branch 'main' into fix/maui-event-binding

bbb6312

Updated CHANGELOG.md

98bb4ae

bitsandfoxes self-assigned this Nov 28, 2023

bitsandfoxes requested review from jamescrosswell and bruno-garcia November 28, 2023 17:05

bitsandfoxes marked this pull request as ready for review November 28, 2023 17:06

bitsandfoxes changed the title ~~fix(MAUI): Removed reflection based breadcrumb creation~~ fix(MAUI): Automatically captured breadcrumbs Nov 28, 2023

bruno-garcia approved these changes Nov 28, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

samples/Sentry.Samples.Maui/MainPage.xaml.cs Outdated Show resolved Hide resolved

Update samples/Sentry.Samples.Maui/MainPage.xaml.cs

df9ca7a

Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>

bruno-garcia reviewed Nov 28, 2023

View reviewed changes

samples/Sentry.Samples.Maui/MainPage.xaml.cs Outdated Show resolved Hide resolved

Update samples/Sentry.Samples.Maui/MainPage.xaml.cs

946a04a

jamescrosswell mentioned this pull request Nov 28, 2023

MAUI - Breadcrumbs for Element/VisualElement add too much overhead in some cases #2919

Closed

jamescrosswell linked an issue Nov 28, 2023 that may be closed by this pull request

MAUI - Breadcrumbs for Element/VisualElement add too much overhead in some cases #2919

Closed

bruno-garcia reviewed Nov 29, 2023

View reviewed changes

bitsandfoxes added 2 commits November 29, 2023 15:37

make 'BindElementEvents' opt-in

368af9c

unbind everything

5e7ee3b

Redth approved these changes Nov 29, 2023

View reviewed changes

simplify

7f0e135

bitsandfoxes added 4 commits November 29, 2023 20:01

tests

294d9df

Merge branch 'main' into fix/maui-event-binding

7a8ee83

cleanup

7350c48

Updated CHANGELOG.md

fe05ea3

bitsandfoxes requested a review from bruno-garcia November 29, 2023 19:49

bruno-garcia reviewed Nov 29, 2023

View reviewed changes

bitsandfoxes added 3 commits November 30, 2023 09:53

getting rid of the static

3e7289b

clean samples

4ab815d

added naive unbind tests

8489a3f

bruno-garcia approved these changes Dec 1, 2023

View reviewed changes

bitsandfoxes and others added 3 commits December 1, 2023 16:47

Update src/Sentry.Maui/BindableSentryMauiOptions.cs

b92fc03

Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>

Merge branch 'main' into fix/maui-event-binding

704adf6

added the interface back

af6d108

bitsandfoxes added 2 commits December 1, 2023 21:17

do I need to add it like that?

061ad4d

missed one

388fa4a

bitsandfoxes merged commit 610f5fa into main Dec 1, 2023
19 checks passed

bitsandfoxes deleted the fix/maui-event-binding branch December 1, 2023 20:58

bitsandfoxes mentioned this pull request Dec 18, 2023

Ui Component Crash When Sentry is Initialized #2971

Closed

bitsandfoxes mentioned this pull request Jan 11, 2024

The System.ObjectDisposed exception is thrown when applying clipping to an element in MAUI #2996

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(MAUI): Automatically captured breadcrumbs #2900

fix(MAUI): Automatically captured breadcrumbs #2900

bitsandfoxes commented Nov 24, 2023 •

edited

jamescrosswell commented Nov 28, 2023

Redth commented Nov 28, 2023

bruno-garcia Nov 29, 2023

bruno-garcia Nov 29, 2023

bruno-garcia Nov 29, 2023

bitsandfoxes commented Nov 29, 2023

Redth left a comment

bruno-garcia Nov 29, 2023

bruno-garcia Nov 29, 2023

bitsandfoxes Dec 1, 2023

bruno-garcia left a comment

bruno-garcia commented Dec 1, 2023

bruno-garcia commented Dec 1, 2023

vaind commented Dec 1, 2023

bitsandfoxes commented Dec 1, 2023

bruno-garcia commented Dec 1, 2023

fix(MAUI): Automatically captured breadcrumbs #2900

fix(MAUI): Automatically captured breadcrumbs #2900

Conversation

bitsandfoxes commented Nov 24, 2023 • edited

jamescrosswell commented Nov 28, 2023

Redth commented Nov 28, 2023

bruno-garcia Nov 29, 2023

Choose a reason for hiding this comment

bruno-garcia Nov 29, 2023

Choose a reason for hiding this comment

bruno-garcia Nov 29, 2023

Choose a reason for hiding this comment

bitsandfoxes commented Nov 29, 2023

Redth left a comment

Choose a reason for hiding this comment

bruno-garcia Nov 29, 2023

Choose a reason for hiding this comment

bruno-garcia Nov 29, 2023

Choose a reason for hiding this comment

bitsandfoxes Dec 1, 2023

Choose a reason for hiding this comment

bruno-garcia left a comment

Choose a reason for hiding this comment

bruno-garcia commented Dec 1, 2023

bruno-garcia commented Dec 1, 2023

vaind commented Dec 1, 2023

bitsandfoxes commented Dec 1, 2023

bruno-garcia commented Dec 1, 2023

bitsandfoxes commented Nov 24, 2023 •

edited