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

Trimming support for NLog (Bump to NLog 5.2.2) #3698

Open
jamescrosswell opened this issue Oct 21, 2024 · 1 comment
Open

Trimming support for NLog (Bump to NLog 5.2.2) #3698

jamescrosswell opened this issue Oct 21, 2024 · 1 comment

Comments

@jamescrosswell
Copy link
Collaborator

NLog 5.2.2 added trimming support. With growing adoption of MAUI, more and more users are trying to trim their apps.

We need to:

  • Bump to 5.2.2
  • Provide enough information about the custom configuration type used with NLog that it either doesn't try to load certain properties or the linker preserves any types required to load those properties.

Worth investigating @snakefoot 's comment about the complexity of the SentryNLogOptions class. If people are only using the NLog integration then ideally they'd be putting all of their Sentry config in one place, but maybe worth challenging that as it creates additional complexity (like this) elsewhere.

Originally posted by @snakefoot in #3680

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

snakefoot commented Oct 21, 2024

Alternative you could remove [NLogConfigurationItem] from SentryNLogOptions, and maybe move any convience-properties from SentryNLogOptions to the NLog SentryTarget directly:

[Target("Sentry")]
public sealed class SentryTarget : TargetWithContext
{
    [NLogConfigurationIgnoreProperty]
    public SentryNLogOptions Options { get; }

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

No idea how often Options are assigned using NLog.config like this:

<!-- Advanced options can be configured here-->
<options
sendDefaultPii="true"
attachStacktrace="false"
shutdownTimeoutSeconds="5"
debug="false"
>
<!--Advanced options can be specified as attributes or elements-->
<includeEventDataOnBreadcrumbs>true</includeEventDataOnBreadcrumbs>
</options>

Right now [NLogConfigurationItem] has 2 meanings. Its safe for NLog to perform object-reflection for assigning properties. And its safe for NLog to perform object-reflection to enlist any NLog Layouts.

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