MAUI CommunityToolkit MVVM auto instrumentation of async commands

[aritchie]

This is a POC to try to insert auto instrumentation of async operations that occur in commands. The MVVM Community Toolkit is insanely popular in .NET MAUI. This will be a key component to making #3181 awesome.

TODO

• Documentation

Performance considerations

There has been a lot of back and forth before merging this PR about the performance implications of building a service provider to resolve the options during service registration itself (since there's a bit of inception going on wiring up the MauiEventBinders on startup here).

Initially I (@jamescrosswell) created some benchmarks comparing two different ways to initialise the Maui integrations. Basically I temporarily changed the implementation of the UseSentry app builder extension to take an optional bool param and do this with it:

        // Resolve the configured options and register any element event binders from these
        SentryMauiOptions options = null!;
        if (resolveOptionsWithServiceProvider)
        {
            IServiceProvider serviceProvider = services.BuildServiceProvider();
            options = serviceProvider.GetRequiredService<IOptions<SentryMauiOptions>>().Value;
            services.TryAddSingleton<SentryOptions>(options); // Ensure this doesn't get resolved again in AddSentry
        }
        else
        {
            options = new SentryMauiOptions();
            configureOptions?.Invoke(options);
        }

So ifresolveOptionsWithServiceProvider == true we use the existing code from this PR to get the options... otherwise we use the "fake options" hack/workaround.

Benchmark results

Method	ResolveOptionsWithServiceProvider	Mean	Error	StdDev	Gen0	Gen1	Allocated
'Build MAUI App'	False	819.0 us	1,542.4 us	84.54 us	11.7188	2.9297	99.63 KB
'Build MAUI App'	True	601.4 us	1,048.6 us	57.48 us	11.7188	3.9063	99.45 KB

So the hack/alternative looked to be about 33% slower and allocate a fraction more memory.

As such, we left the original code in the PR (that builds a service provider to resolve the options during init).

Here's the code I used for the original benchmark:

[ShortRunJob]
public class MvvmBenchmarks
{
    private MauiAppBuilder AppBuilder;

    [Params(true, false)]
    public bool ResolveOptionsWithServiceProvider;

    [GlobalSetup]
    public void Setup()
    {
        AppBuilder = MauiApp.CreateBuilder()
            // This adds Sentry to your Maui application
            .UseSentry(options =>
            {
                // The DSN is the only required option.
                options.Dsn = DsnSamples.ValidDsn;
                // Automatically create traces for async relay commands in the MVVM Community Toolkit
                options.AddCommunityToolkitIntegration();
            }, ResolveOptionsWithServiceProvider);
    }

    [Benchmark(Description = "Build MAUI App")]
    public void BuildMauiAppBenchmark()
    {
        AppBuilder.Build();
    }
}

After receiving further comments from @Redth I figured I'd put together some more comprehensive benchmarks:

• MauiAppBuilderExtensions Benchmarks - DO NOT MERGE #4213

The new benchmarks differ in two key respects

1. They compare 3 (not 2) different initialisation options including
   • Building a service provider to resolve the Sentry Options (from the original benchmark)
   • Creating some SentryOptions directly and invoking the configOptionsCallback on them (from the original benchmark)
   • Not resolving any options... just registering the services on the Service provider directly (new)
2. The original benchmarks were run as a ShortJob, so the results weren't as reliable as they could be

Results of the new benchmarks are:

Method	ResolveOptionsWithServiceProvider	Mean	Error	StdDev	Gen0	Gen1	Allocated
'Build MAUI App'	Directly	470.8 us	9.02 us	9.65 us	11.7188	2.9297	99.25 KB
'Build MAUI App'	ServiceProvider	473.6 us	8.73 us	13.85 us	11.7188	2.9297	98.66 KB
'Build MAUI App'	InvokeConfigOptions	462.0 us	8.84 us	10.18 us	11.7188	2.9297	98.74 KB

Curiously, this time the performance for options 2 and 3 are reversed (in the initial benchmark it was quicker to build the service provider than to create the options ourselves)... This is probably because we were running a ShortJob before, so the results weren't very reliable.

This time round, the winner appears to be invoking the config options callback ourselves... although it allocates slightly more memory.

Overall, the difference between all 3 different techniques is less than 10 microseconds. So we could save 10 microseconds by creating the options and invoking the callback ourselves, rather than building a service provider to do this for us.

[bruno-garcia]

This is quite popular: https://nugettrends.com/packages?ids=CommunityToolkit.Mvvm&months=24

Worth adding it in

[bruno-garcia]

worth profiling the Init code just to see if building the container has any significant bottleneck (or anything else we added there).

But outside of that,lgtm

[aritchie]

People using MAUI tend to change the container library. It is extremely common. You want to profile all of them?

[Syed-RI]

docs on usage please?

[BrycensRanch]

Better yet, how do I use this in my Avalonia app? I use CommunityToolkit MVVM too. I just can't directly consume your Sentry.Maui.CommunityToolkit package.

[jamescrosswell]

docs on usage please?

Wow - I think you must have asked for this at the precise moment this new integration was released 😄

There aren't any docs yet but there's an example of how to enable the integration in Sentry.Samples.Maui:

sentry-dotnet/samples/Sentry.Samples.Maui/MauiProgram.cs

Lines 29 to 35 in 34d8490

	// The Sentry MVVM Community Toolkit integration automatically creates traces for async relay commands,
	// but only if tracing is enabled. Here we capture all traces (in a production app you'd probably only
	// capture a certain percentage)
	options.TracesSampleRate = 1.0F;
	// Automatically create traces for async relay commands in the MVVM Community Toolkit
	options.AddCommunityToolkitIntegration();

Once enabled, the new integration automatically creates traces for any MVVM AsyncRelayCommands in the application.

This new integration is pretty minimal at the moment but we're hoping to get feedback from SDK about it's usefulness and whether/how it might be extended or tweaked to be even better.

[jamescrosswell]

Better yet, how do I use this in my Avalonia app?

@BrycensRanch this particular integration is MAUI specific. It leverages event hooks in the Microsoft.Maui.Controls.Application class to detect when an AsynReplayCommand starts or stops and start/finish a tracing span that represents that relay command. I think Avalonia would require a separate solution.

[Syed-RI]

docs on usage please?

Wow - I think you must have asked for this at the precise moment this new integration was released 😄

There aren't any docs yet but there's an example of how to enable the integration in Sentry.Samples.Maui:

sentry-dotnet/samples/Sentry.Samples.Maui/MauiProgram.cs

Lines 29 to 35 in 34d8490

	// The Sentry MVVM Community Toolkit integration automatically creates traces for async relay commands,
	// but only if tracing is enabled. Here we capture all traces (in a production app you'd probably only
	// capture a certain percentage)
	options.TracesSampleRate = 1.0F;
	// Automatically create traces for async relay commands in the MVVM Community Toolkit
	options.AddCommunityToolkitIntegration();

Once enabled, the new integration automatically creates traces for any MVVM AsyncRelayCommands in the application.

This new integration is pretty minimal at the moment but we're hoping to get feedback from SDK about it's usefulness and whether/how it might be extended or tweaked to be even better.

Yes I did ask the question right after I have received notification of a new release 😄 .

Just to be clear -

• To use and add options.AddCommunityToolkitIntegration(); you will have to add Sentry.Maui.CommunityToolkit.Mvvm package?
• Without it, the SDK will continue to work as it was before without impacting the app startup (concern raise by @Redth MAUI CommunityToolkit MVVM auto instrumentation of async commands #4125 (comment)) ? Im asking dumb question to understand whether updating to 5.8.0 without the Sentry.Maui.CommunityToolkit.Mvvm package will have any impact on the app start as everyone is quite touchy about it.

[github-actions]

	Fails
🚫	Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- MAUI CommunityToolkit MVVM auto instrumentation of async commands ([#4125](https://github.com/getsentry/sentry-dotnet/pull/4125))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 70525e6

[jamescrosswell]

• To use and add options.AddCommunityToolkitIntegration(); you will have to add Sentry.Maui.CommunityToolkit.Mvvm package?

Yes, indeed. That's where that extension on the SentryOptions is defined. Sorry, I should have mentioned that.

Without it, the SDK will continue to work as it was before without impacting the app startup (concern raise by @Redth MAUI CommunityToolkit MVVM auto instrumentation of async commands #4125 (comment)) ?

No, the comments on potential performance impact relate to code changes in the Sentry.Maui package. However in the benchmarks that I've run (which I just updated about 10 minutes ago) the performance impact would be in the vicinity of 6-10 microseconds (although that is admittedly running on a Mac M2 - it could be more impactful on a really old/slow mobile device).

I pushed up a branch containing those benchmarks. I had to break the software a bit in order to build them, so we'll never merge that branch but it means other folks can run the benchmarks themselves to reproduce the results... so we can make decisions based on data rather than strong opinions. Data tends to argue less 😜

@Redth @aritchie @bruno-garcia on the back of the benchmarks, it looks like we could shave 10 microseconds (that's millionths of a second) off startup by creating the config options and invoking the callback ourselves. So we could do that in another PR. It would be a trivial change.

[Syed-RI]

it could be more impactful on a really old/slow mobile device)

That could be potentially dealbreaker as enterprises devices like Zebra and Honeywell (most of our target audience) tend to run on archaic chips with low memory but I dont have an way of benchmarking them at the moment to compare Apple with Apple.

[jamescrosswell]

That could be potentially dealbreaker

@Syed-RI I spotted a potential issue with the benchmarks... I think a lot of what needed to be measured was hidden in the GlobalSetup method of the benchmark tests. I corrected this and re-ran them. The new benchmarks actually show the existing code/solution to be the fastest option

Feel free to check the benchmarks out for yourself.

Again, the difference between all of the three options is less than 12 millionths of a second, which on a machine that is 1000 times slower than mine would be around 10 milliseconds (not something a human being would perceive)

btw: if you're concerned about memory, this change will have little to no impact. But this PR could be a huge win for you:

• UnsampledTransactions to reduce memory pressure #4212

[jamescrosswell]

Closing this particular PR for comments as the discussion is getting quite long (and off topic).

If you've got any performance concerns, please add comments on the related Benchmarking PR instead (so that we can use data to drive the discussion):

• MauiAppBuilderExtensions Benchmarks - DO NOT MERGE #4213

[aritchie]

I pushed up a #4213. I had to break the software a bit in order to build them, so we'll never merge that branch but it means other folks can run the benchmarks themselves to reproduce the results... so we can make decisions based on data rather than strong opinions. Data tends to argue less 😜

@jamescrosswell - you're benchmarks don't account for real world situations. The "strong opinions" isn't a fair comment. We're all professionals and some of us have lots of experience in different areas. So let me summarize some additional reasoning for why I'm 1000% against doing what this PR is doing

1. .NET MAUI developers don't just use the MSFT extensions DI. There are projects out there that I've actually worked with that use DryIoc (prism - extremely popular .net library - requires a mutable container) and autofac (this is the killer - the performance for building the container is awful) which verifies the dependency references regardless of the buildservicecontainer flag. I don't think we want to get into the market of testing every DI container here.

2. Your tests assume a minimal set of dependencies. Depending on where UseSentry is in the setup phase, depends on how large of a chain you actually end up building the container with. Your test isn't up against anything but UseSentry. Real world scenario may have 100s of services registered. Let's take this pig of a method from a real world project. Note where sentry is in this chain

public static MauiApp CreateMauiApp() => MauiApp
        .CreateBuilder()
        .UseMauiApp<App>()
        .ConfigureServices(x =>
        {
            // this method is about 40 lines longer, so just a sample here is fine
            x.AddMauiBlazorWebView();
            x.AddMudServices();
            x.AddSingleton<ILogEnricher, Redact>();
            x.AddSingleton(TimeProvider.System);
            x.AddSingleton<SqliteConnection>();
            x.AddJob(typeof(Redact));
            x.AddBluetoothLE();
            x.AddGps<Redact>();
            x.AddGeofencing<Redact>();
            x.AddHttpTransfers<Redact>();
            #if DEBUG
            x.AddBlazorWebViewDeveloperTools();
            #endif
        })
        .UseShiny()
        .UseBarcodeScanning()
        .UseMauiCommunityToolkit()
        .UseMauiCommunityToolkitMediaElement()
        .UseMauiCommunityToolkitMarkup()
        .UseBottomSheet()
        .UseMauiCameraView()
        .UseBiometricAuthentication()
        .UseSettingsView()
        .UsePrism(
            new DryIocContainerExtension(), // different DI container because prism requires mutable containers 
            prism => prism.CreateWindow("redacted")
        )
        .UseMauiMaps()
        .UseCardsView()
        .AddRemoteConfigurationMaui("https://redacted")
        .UseSentry(options =>
        {
            options.Dsn = "redacted";
            options.TracesSampleRate = 1.0;
        })
        .ConfigureFonts(fonts =>
        {
            //.... redact
        })
        .Build();

3. What I've proposed in the following PR is small, easy for users to implement, and has zero risks of anything @Redth & myself have stated previously and in this note directly. MAUI SentryAppBuilder  #4208

CC: @bruno-garcia

[jamescrosswell]

@aritchie I'm happy to look at your alternative solution to the problem once that PR is passing all the CI tests.

Do you think you can also put together some benchmarks illustrating the scenario you're describing (maybe something using Autofac and/or emulating a real world / pig setup method)?

Actually, @aritchie I think we can address all of the concerns you have above by using the second method from the benchmark fork:

sentry-dotnet/src/Sentry.Maui/SentryMauiAppBuilderExtensions.cs

Lines 88 to 96 in 7f64d50

	case RegisterEventBinderMethod.InvokeConfigOptions:
	var options = new SentryMauiOptions();
	configureOptions?.Invoke(options);
	services.TryAddSingleton<SentryOptions>(options); // Ensure this doesn't get resolved again in AddSentry
	foreach (var eventBinder in options.DefaultEventBinders)
	{
	eventBinder.Register(services);
	}
	break;

That way people still setup the new integration as an extension on SentryOptions (i.e. from within the call to UseSentry) and we don't build a service provider to resolve the options. The time required to do this would be absolutely trivial, regardless of the container technology that people have chosen... and we can do this without creating any extra benchmarks.

I've created a new issue for this here:

• Invoke configOptions callback directly to derive event binders during Maui init #4216

Should be trivial to implement.