Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(MAUI): Automatically captured breadcrumbs #2900

Merged
merged 25 commits into from
Dec 1, 2023
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@

## Unreleased

### Fixes

- Reduced noise created by automatically captured breadcrumbs on MAUI ([#2900](https://github.com/getsentry/sentry-dotnet/pull/2900))
- The SDK no longer consumes third-party events on MAUI ([#2900](https://github.com/getsentry/sentry-dotnet/pull/2900))
bitsandfoxes marked this conversation as resolved.
Show resolved Hide resolved

### API breaking Changes

#### Removed APIs
Expand Down
25 changes: 24 additions & 1 deletion samples/Sentry.Samples.Maui/MainPage.xaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

protected override void OnAppearing()
{
AddHandlerBreadcrumbs();
bitsandfoxes marked this conversation as resolved.
Show resolved Hide resolved

#if !ANDROID
JavaCrashBtn.IsVisible = false;
#endif
Expand All @@ -27,6 +29,28 @@
base.OnAppearing();
}

private void AddHandlerBreadcrumbs()
{
foreach (var visualElement in GetVisualTreeDescendants())

Check failure on line 34 in samples/Sentry.Samples.Maui/MainPage.xaml.cs

View workflow job for this annotation

GitHub Actions / .NET (ubuntu-latest)

The name 'GetVisualTreeDescendants' does not exist in the current context

Check failure on line 34 in samples/Sentry.Samples.Maui/MainPage.xaml.cs

View workflow job for this annotation

GitHub Actions / .NET (ubuntu-latest)

The name 'GetVisualTreeDescendants' does not exist in the current context

Check failure on line 34 in samples/Sentry.Samples.Maui/MainPage.xaml.cs

View workflow job for this annotation

GitHub Actions / .NET (windows-latest)

The name 'GetVisualTreeDescendants' does not exist in the current context

Check failure on line 34 in samples/Sentry.Samples.Maui/MainPage.xaml.cs

View workflow job for this annotation

GitHub Actions / .NET (windows-latest)

The name 'GetVisualTreeDescendants' does not exist in the current context

Check failure on line 34 in samples/Sentry.Samples.Maui/MainPage.xaml.cs

View workflow job for this annotation

GitHub Actions / .NET (windows-latest)

The name 'GetVisualTreeDescendants' does not exist in the current context

Check failure on line 34 in samples/Sentry.Samples.Maui/MainPage.xaml.cs

View workflow job for this annotation

GitHub Actions / .NET (windows-latest)

The name 'GetVisualTreeDescendants' does not exist in the current context

Check failure on line 34 in samples/Sentry.Samples.Maui/MainPage.xaml.cs

View workflow job for this annotation

GitHub Actions / .NET (macos-13)

The name 'GetVisualTreeDescendants' does not exist in the current context

Check failure on line 34 in samples/Sentry.Samples.Maui/MainPage.xaml.cs

View workflow job for this annotation

GitHub Actions / .NET (macos-13)

The name 'GetVisualTreeDescendants' does not exist in the current context

Check failure on line 34 in samples/Sentry.Samples.Maui/MainPage.xaml.cs

View workflow job for this annotation

GitHub Actions / .NET (macos-13)

The name 'GetVisualTreeDescendants' does not exist in the current context

Check failure on line 34 in samples/Sentry.Samples.Maui/MainPage.xaml.cs

View workflow job for this annotation

GitHub Actions / .NET (macos-13)

The name 'GetVisualTreeDescendants' does not exist in the current context

Check failure on line 34 in samples/Sentry.Samples.Maui/MainPage.xaml.cs

View workflow job for this annotation

GitHub Actions / .NET (macos-13)

The name 'GetVisualTreeDescendants' does not exist in the current context

Check failure on line 34 in samples/Sentry.Samples.Maui/MainPage.xaml.cs

View workflow job for this annotation

GitHub Actions / .NET (macos-13)

The name 'GetVisualTreeDescendants' does not exist in the current context
bruno-garcia marked this conversation as resolved.
Show resolved Hide resolved
{
if (visualElement is Element element)
{
element.HandlerChanged += (sender, _) =>
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

{
// The StyleId correlates to the element's name if one is set in XAML
var elementName = element.StyleId;
if (elementName is not null)
{
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");
Copy link
Member

Choose a reason for hiding this comment

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

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

}
};
}
}
}

private void OnCounterClicked(object sender, EventArgs e)
{
_count++;
Expand Down Expand Up @@ -88,4 +112,3 @@
#endif
}
}

2 changes: 0 additions & 2 deletions src/Sentry.Maui/Internal/IMauiEventsBinder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ internal interface IMauiEventsBinder
{
void BindApplicationEvents(Application application);

void BindReflectedEvents(BindableObject bindableObject, bool includeExplicitlyHandledTypes = false);

void BindWindowEvents(Window window);

void BindElementEvents(Element element);
Expand Down
121 changes: 0 additions & 121 deletions src/Sentry.Maui/Internal/MauiEventsBinder.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
using Microsoft.Extensions.Options;
using Sentry.Extensibility;

namespace Sentry.Maui.Internal;

Expand All @@ -13,26 +12,11 @@ internal class MauiEventsBinder : IMauiEventsBinder
internal const string NavigationType = "navigation";
internal const string SystemType = "system";
internal const string UserType = "user";
internal const string HandlersCategory = "ui.handlers";
internal const string LifecycleCategory = "ui.lifecycle";
internal const string NavigationCategory = "navigation";
internal const string RenderingCategory = "ui.rendering";
internal const string UserActionCategory = "ui.useraction";

// This list should contain all types that we have explicitly added handlers for their events.
// Any elements that are not in this list will have their events discovered by reflection.
internal static readonly HashSet<Type> ExplicitlyHandledTypes = new()
{
typeof(BindableObject),
typeof(Element),
typeof(VisualElement),
typeof(Application),
typeof(Window),
typeof(Shell),
typeof(Page),
typeof(Button),
};

public MauiEventsBinder(IHub hub, IOptions<SentryMauiOptions> options)
{
_hub = hub;
Expand All @@ -52,10 +36,6 @@ public void BindApplicationEvents(Application application)
BindVisualElementEvents(visualElement);
}

// We'll use reflection to attach to other events
// This allows us to attach to events from custom controls
BindReflectedEvents(e.Element);
bitsandfoxes marked this conversation as resolved.
Show resolved Hide resolved

// We can also attach to specific events on built-in controls
// Be sure to update ExplicitlyHandledTypes when adding to this list
switch (e.Element)
Expand Down Expand Up @@ -87,15 +67,9 @@ public void BindApplicationEvents(Application application)
application.PageDisappearing += (sender, page) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Application.PageDisappearing), NavigationType, NavigationCategory,
data => data.AddElementInfo(_options, page, nameof(Page)));
application.ModalPushing += (sender, e) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Application.ModalPushing), NavigationType, NavigationCategory,
data => data.AddElementInfo(_options, e.Modal, nameof(e.Modal)));
application.ModalPushed += (sender, e) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Application.ModalPushed), NavigationType, NavigationCategory,
data => data.AddElementInfo(_options, e.Modal, nameof(e.Modal)));
application.ModalPopping += (sender, e) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Application.ModalPopping), NavigationType, NavigationCategory,
data => data.AddElementInfo(_options, e.Modal, nameof(e.Modal)));
application.ModalPopped += (sender, e) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Application.ModalPopped), NavigationType, NavigationCategory,
data => data.AddElementInfo(_options, e.Modal, nameof(e.Modal)));
Expand All @@ -107,47 +81,6 @@ public void BindApplicationEvents(Application application)
data => data.Add(nameof(e.RequestedTheme), e.RequestedTheme.ToString()));
}

public void BindReflectedEvents(BindableObject bindableObject, bool includeExplicitlyHandledTypes = false)
{
// This reflects over the object's events.
// By default, it attaches only to events that are *NOT* declared by types in the ExplicitlyHandledTypes list.
// We will only include such events when testing.

var type = bindableObject.GetType();

IEnumerable<EventInfo> events = type.GetEvents(BindingFlags.Instance | BindingFlags.Public);
if (!includeExplicitlyHandledTypes)
{
events = events.Where(e => !ExplicitlyHandledTypes.Contains(e.DeclaringType!));
}

foreach (var eventInfo in events)
{
var browsable = eventInfo.GetCustomAttribute<EditorBrowsableAttribute>();
if (browsable != null && browsable.State != EditorBrowsableState.Always)
{
// These events are not meant for typical consumption.
continue;
}

Action<object, object> handler = (sender, _) =>
{
_hub.AddBreadcrumbForEvent(_options, sender, eventInfo.Name);
};

try
{
var typedHandler = Delegate.CreateDelegate(eventInfo.EventHandlerType!, handler.Target, handler.Method);
eventInfo.AddEventHandler(bindableObject, typedHandler);
}
catch (Exception ex)
{
// Don't throw if we can't bind the event handler
_options.DiagnosticLogger?.LogError(ex, "Couldn't bind to {0}.{1}", type.Name, eventInfo.Name);
}
}
}

public void BindWindowEvents(Window window)
{
// Lifecycle Events
Expand Down Expand Up @@ -192,15 +125,9 @@ public void BindWindowEvents(Window window)
});

// Navigation events
window.ModalPushing += (sender, e) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Window.ModalPushing), NavigationType, NavigationCategory,
data => data.AddElementInfo(_options, e.Modal, nameof(e.Modal)));
window.ModalPushed += (sender, e) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Window.ModalPushed), NavigationType, NavigationCategory,
data => data.AddElementInfo(_options, e.Modal, nameof(e.Modal)));
window.ModalPopping += (sender, e) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Window.ModalPopping), NavigationType, NavigationCategory,
data => data.AddElementInfo(_options, e.Modal, nameof(e.Modal)));
window.ModalPopped += (sender, e) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Window.ModalPopped), NavigationType, NavigationCategory,
data => data.AddElementInfo(_options, e.Modal, nameof(e.Modal)));
Expand All @@ -210,37 +137,13 @@ public void BindWindowEvents(Window window)

public void BindElementEvents(Element element)
{
// Element handler events
// https://docs.microsoft.com/dotnet/maui/user-interface/handlers/customize#handler-lifecycle
element.HandlerChanging += (sender, e) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Element.HandlerChanging), SystemType, HandlersCategory,
data =>
{
data.Add(nameof(e.OldHandler), e.OldHandler?.ToStringOrTypeName() ?? "");
data.Add(nameof(e.NewHandler), e.NewHandler?.ToStringOrTypeName() ?? "");
});
element.HandlerChanged += (sender, _) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Element.HandlerChanged), SystemType, HandlersCategory,
data =>
{
var e = sender as Element;
data.Add(nameof(e.Handler), e?.Handler?.ToStringOrTypeName() ?? "");
});

// Rendering events
element.ChildAdded += (sender, e) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Element.ChildAdded), SystemType, RenderingCategory,
data => data.AddElementInfo(_options, e.Element, nameof(e.Element)));
element.ChildRemoved += (sender, e) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Element.ChildRemoved), SystemType, RenderingCategory,
data => data.AddElementInfo(_options, e.Element, nameof(e.Element)));
element.ParentChanging += (sender, e) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Element.ParentChanging), SystemType, RenderingCategory,
data =>
{
data.AddElementInfo(_options, e.OldParent, nameof(e.OldParent));
data.AddElementInfo(_options, e.NewParent, nameof(e.NewParent));
});
element.ParentChanged += (sender, _) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Element.ParentChanged), SystemType, RenderingCategory,
data =>
Expand Down Expand Up @@ -271,10 +174,6 @@ public void BindElementEvents(Element element)
data =>
data.Add(nameof(BindableObject.BindingContext), bindingContext.ToStringOrTypeName()));
};

// NotifyPropertyChanged events are too noisy to be useful
// element.PropertyChanging
// element.PropertyChanged
}

public void BindVisualElementEvents(VisualElement element)
Expand All @@ -284,21 +183,6 @@ public void BindVisualElementEvents(VisualElement element)

element.Unfocused += (sender, _) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(VisualElement.Unfocused), SystemType, RenderingCategory);

element.Loaded += (sender, _) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(VisualElement.Loaded), SystemType, RenderingCategory);

element.Unloaded += (sender, _) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(VisualElement.Unloaded), SystemType, RenderingCategory);

element.ChildrenReordered += (sender, _) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(VisualElement.ChildrenReordered), SystemType, RenderingCategory);

element.MeasureInvalidated += (sender, _) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(VisualElement.MeasureInvalidated), SystemType, RenderingCategory);

element.SizeChanged += (sender, _) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(VisualElement.SizeChanged), SystemType, RenderingCategory);
}

public void BindShellEvents(Shell shell)
Expand Down Expand Up @@ -337,11 +221,6 @@ public void BindPageEvents(Page page)

// Navigation events
// https://github.com/dotnet/docs-maui/issues/583
page.NavigatingFrom += (sender, _) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Page.NavigatingFrom), NavigationType, NavigationCategory);
page.NavigatedFrom += (sender, e) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Page.NavigatedFrom), NavigationType, NavigationCategory,
data => data.AddElementInfo(_options, e.GetDestinationPage(), "DestinationPage"));
page.NavigatedTo += (sender, e) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Page.NavigatedTo), NavigationType, NavigationCategory,
data => data.AddElementInfo(_options, e.GetPreviousPage(), "PreviousPage"));
Expand Down
2 changes: 0 additions & 2 deletions test/Sentry.Maui.Tests/MauiEventsBinderTests.Application.cs
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,7 @@ public static IEnumerable<object[]> ApplicationModalEventsData
return new List<object[]>
{
// Note, these are distinct from the Window events with the same names.
new object[] {nameof(Application.ModalPushing), new ModalPushingEventArgs(modelPage)},
new object[] {nameof(Application.ModalPushed), new ModalPushedEventArgs(modelPage)},
new object[] {nameof(Application.ModalPopping), new ModalPoppingEventArgs(modelPage)},
new object[] {nameof(Application.ModalPopped), new ModalPoppedEventArgs(modelPage)}
};
}
Expand Down