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

Release Builds Crashing on Launch when using NLog Integration and FailedRequestStatusCodes Options #3680

Open
mgr44 opened this issue Oct 14, 2024 · 6 comments

Comments

@mgr44
Copy link

mgr44 commented Oct 14, 2024

Package

Sentry

.NET Flavor

.NET

.NET Version

8.0.0

OS

Android

SDK Version

4.12.1

Self-Hosted Sentry Version

No response

Steps to Reproduce

Context:
I have been having issues with my Maui app crashing on launch when building for release. Eventually I was able to trace the issue to a few configuration options that reproduce the issue in a simple sample project. These include setting up Sentry and NLog and then running a release build on Android or iOS. If trimming is enabled for the project in the Android project settings (or linking for iOS) on debug builds then the issue seems to reproduce in debug builds as well and I get this error: System.ArgumentException: 'Arg_GetMethNotFnd' when calling LoadConfiguration on the NLog ISetupBuilder object.

Interestingly one of the configuration options I have to set to reproduce the error is setting sentryOptions.FailedRequestStatusCodes to a non-null value. Both an empty list or specified range seems to "work" (cause the crash). It doesn't seem to mater if SentryOptions.CaptureFailedRequests is enabled or not, so I have not included that option in my repro steps.

As a work around, the following snippet can be added to the project file to prevent trimming of the Sentry assembly.
<ItemGroup> <TrimmerRootAssembly Include="Sentry" RootMode="library"/> </ItemGroup>

Repro in a new/existing Maui project

  1. Add nuget packages Sentry.Maui 4.12.1 and Sentry.NLog 4.12.1
  2. Add nuget packages NLog 5.3.4, NLog.Extensions.Logging 5.3.14, NLog.Targets.MauiLog 8.0.0
  3. Add the the following configuration to MauiProgram.CreateMauiApp:
.UseSentry(options =>
                {
                    options.Dsn = "<Your DSN here>";
                    options.Release = "releaseString";
                    options.Distribution = "distributionString";
                    options.Environment = "environmentString";
                    options.Debug = true;
                    options.FailedRequestStatusCodes = new List<HttpStatusCodeRange>() {};
                })
                .Logging.ClearProviders().AddNLog()
  1. Add this code to setup NLog after setting up the app builder but before calling builder.Build():
            ISetupBuilder setup = LogManager.Setup();
            setup.RegisterMauiLog();
            setup.LoadConfiguration(Configure);
  1. Add this configure method inside of MauiProgram:
        private static void Configure(ISetupLoadConfigurationBuilder configBuilder)
        {
            configBuilder.Configuration.AddSentry(options =>
            {
                options.InitializeSdk = false;
                options.MinimumBreadcrumbLevel = NLog.LogLevel.Info;
                options.MinimumEventLevel = NLog.LogLevel.Error;
            });
        }
  1. Deploy a release build, or run a debug build with trimming (android) or linking (iOS) enabled.
  2. Launch the app.

Repro with Sample Project

  1. Download reproducing sample project: https://github.com/mgr44/Maui-Sentry-TrimmingSample
  2. Add a valid DSN to MauiProgram.cs line 22
  3. Deploy to an Android device with debug or release build (this project has trimming enabled for android debug builds)
  4. Launch the App

Expected Result

The app launches normally and Sentry and NLog are functional.

Actual Result

The app crashes during the launch

@jamescrosswell
Copy link
Collaborator

@mgr44 thank you for the detailed investigation. We'll try to reproduce this as well... it might take a few days though as there's a lot going on at the moment.

@jamescrosswell
Copy link
Collaborator

jamescrosswell commented Oct 20, 2024

I can reproduce this thanks to the excellent minimal repro you provided @mgr44 . When I run this in Debug on Android I get a crash from this line:

setup.LoadConfiguration(Configure);

The crash seems to be coming from NLog:

System.ArgumentException: Arg_GetMethNotFnd
   at System.Reflection.RuntimePropertyInfo.GetValue(Object , Object[] )
   at NLog.Internal.ObjectGraphScanner.ScanProperties[Object](ConfigurationItemFactory configFactory, Boolean aggressiveSearch, Object targetObject, List`1 result, Int32 level, HashSet`1 visitedObjects)
   at NLog.Internal.ObjectGraphScanner.ScanPropertyForObject[Object](ConfigurationItemFactory configFactory, Boolean aggressiveSearch, Object propValue, PropertyInfo prop, List`1 result, Int32 level, HashSet`1 visitedObjects)
   at NLog.Internal.ObjectGraphScanner.ScanProperties[Object](ConfigurationItemFactory configFactory, Boolean aggressiveSearch, Object targetObject, List`1 result, Int32 level, HashSet`1 visitedObjects)
   at NLog.Internal.ObjectGraphScanner.ScanPropertiesList[Object](ConfigurationItemFactory configFactory, Boolean aggressiveSearch, IList list, List`1 result, Int32 level, HashSet`1 visitedObjects)
   at NLog.Internal.ObjectGraphScanner.ScanPropertyForObject[Object](ConfigurationItemFactory configFactory, Boolean aggressiveSearch, Object propValue, PropertyInfo prop, List`1 result, Int32 level, HashSet`1 visitedObjects)
   at NLog.Internal.ObjectGraphScanner.ScanProperties[Object](ConfigurationItemFactory configFactory, Boolean aggressiveSearch, Object targetObject, List`1 result, Int32 level, HashSet`1 visitedObjects)
   at NLog.Internal.ObjectGraphScanner.FindReachableObjects[Object](ConfigurationItemFactory configFactory, Boolean aggressiveSearch, Object[] rootObjects)
   at NLog.Config.LoggingConfiguration.ValidateConfig()
   at NLog.Config.LoggingConfiguration.InitializeAll()
   at NLog.LogFactory.RefreshExistingLoggers()
   at NLog.LogFactory.ReconfigExistingLoggers()
   at NLog.LogFactory.ActivateLoggingConfiguration(LoggingConfiguration config)
   at NLog.LogFactory.set_Configuration(LoggingConfiguration value)
   at NLog.SetupBuilderExtensions.LoadConfiguration(ISetupBuilder setupBuilder, Action`1 configBuilder)
   at SentryPOC.MauiProgram.CreateMauiApp() in /Users/jamescrosswell/code/Maui-Sentry-TrimmingSample/MauiProgram.cs:line 36

Presumably something isn't Trim safe...

@jamescrosswell
Copy link
Collaborator

jamescrosswell commented Oct 21, 2024

So by turning on NLog's internal logging, I got a bit more information about where the error is coming from. It looks like NLog uses reflection inspect various properties in the SentryOptions object. Just before the crash, it logs: Scanning Property TagFilters... which I believe is coming from here in the NLog source.

So it's trying to loop over the values here:

/// <summary>
/// List of substrings or regular expression patterns to filter out tags
/// </summary>
public ICollection<SubstringOrRegexPattern> TagFilters { get; set; } = new List<SubstringOrRegexPattern>();

Those are exposed as an ICollection<SubstringOrRegexPattern>. By default we initialise these to a new List<SubstringOrRegexPattern>() but there's nothing to stop you assigning any ICollection<SubstringOrRegexPattern> to that property.

If you enable trimming then, the Linker doesn't know what code has to be retained and something that is required is being trimmed here. It might be the propListValue.Count property referred to here or it might be other properties and methods of that collection that NLog is trying to call via reflection.

You'd see something similar with the IList<SubstringOrRegexPattern> FailedRequestTargets if NLog is trying to loop over that as well.

It might be possible to force the types and members that NLog needs to be retained by customising the linker configuration via a LinkDescription element in your csproj file (example here). But that feels pretty unreliable - you'd basically have to run your app and see if/when it breaks. There's no static analysis to help you build confidence that your app will work, if you're doing that.

Ultimately, you want static analysers to make sure it will work properly. This is something we did to support AOT in Sentry library with the 4.0.0 release but it doesn't look like NLog is yet trimming or AOT compatible. That's a goal for the project in NLog 6.

Until then, I wouldn't recommend using NLog in a trimmed application.

@snakefoot
Copy link
Contributor

snakefoot commented Oct 21, 2024

I'm a maintainer of NLog, and and curious about the advice about not using NLog when using trimming.

Think the Sentry.NLog project should update to NLog v5.2.2 (or newer).

And then you should provide a registration-method, that marks the relevant types in Sentry.NLog to not be trimmed (This already happens for all types inside NLog):

NLog.LogManager.Setup().SetupExtensions(ext => {
	ext.RegisterTarget<Sentry.NLog.SentryTarget>();
	ext.RegisterType<Sentry.NLog.SentryNLogOptions>();
});

You should probably avoid mixing options that are not intended to be assigned from NLog-configuration-file. Maybe SentryNLogOptions should not inherit from SentryOptions. But should instead just expose the properties that are intended to be assigned from NLog-configuration-file. Then you could have a property in SentryNLogOptions marked like this:

[NLogConfigurationItem]
public class SentryNLogOptions 
{
    [NLogConfigurationIgnoreProperty]
    public SentryOptions SentryOptions { get; } = new SentryOptions();

    public int ShutdownTimeoutSeconds
    {
        get => (int)SentryOptions.ShutdownTimeout.TotalSeconds;
        set => SentryOptions.ShutdownTimeout = TimeSpan.FromSeconds(value);
    }
}

Notice the AOT-issues that NLog v6 will be fixing is removing the dependency on RegEx and XmlReader and ExpressionTrees, since they include code-generation-abilities and there is no JIT-compiler included with pure AOT-builds.

@snakefoot
Copy link
Contributor

@mgr44 Could you try this:

ISetupBuilder setup = LogManager.Setup();
setup.RegisterMauiLog();
setup.SetupExtensions(ext => {
   ext.RegisterTarget<Sentry.NLog.SentryTarget>();
   ext.RegisterType<Sentry.NLog.SentryNLogOptions>();
});
setup.LoadConfiguration(Configure);

@jamescrosswell
Copy link
Collaborator

jamescrosswell commented Oct 21, 2024

I'm a maintainer of NLog, and and curious about the advice about not using NLog when using trimming.

Think the Sentry.NLog project should update to NLog v5.2.2 (or newer).

Thanks @snakefoot ! Lots of good info in that post. I've created:

And then you should provide a registration-method, that marks the relevant types in Sentry.NLog to not be trimmed (This already happens for all types inside NLog):

NLog.LogManager.Setup().SetupExtensions(ext => {
ext.RegisterTarget<Sentry.NLog.SentryTarget>();
ext.RegisterType<Sentry.NLog.SentryNLogOptions>();
});

It looks like the Sentry NLog integration is already using RegisterTarget but not yet RegisterType for the SentryNLogOptions:

LogManager.Setup().SetupExtensions(e => e.RegisterTarget<SentryTarget>("Sentry"));

You should probably avoid mixing options that are not intended to be assigned from NLog-configuration-file. Maybe SentryNLogOptions should not inherit from SentryOptions. But should instead just expose the properties that are intended to be assigned from NLog-configuration-file.

This was all built before I started maintaining the Sentry SDK but I believe this was intentional... so that when you're configuring Sentry from a config file, all of the properties would be in one place regardless of which integration you're using. With the ASP.NET Core integration, for example, it would be weird to specify the properties that are specific to ASP.NET Core in a separate place in the config file from the properties that relate to Sentry Core. And if people are using Sentry with ASP.NET Core and NLog, it would get even more confusing... particularly if some of those properties are relevant to multiple integrations (people don't want to have to specify them multiple times).

But I get your point... and the challenge above is really just complexity of the Sentry repo that we have to find a good solution for. We might not be able to do exactly what you've suggested but maybe we can find a better solution than the current one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Status: No status
Development

No branches or pull requests

3 participants