NLog / NLog.Extensions.Logging

NLog as Logging Provider for Microsoft Extension Logging
https://nlog-project.org
BSD 2-Clause "Simplified" License
392 stars 152 forks source link

Allow appsettings.json to specify extensions as keyed object #735

Closed TheXenocide closed 4 months ago

TheXenocide commented 5 months ago

Type: Feature request

NLog version: 5.2.8

NLog.Extensions.Logging version: 5.3.8

NLog.Web.AspNetCore version: 5.3.8

Platform: .NET Framework 4.8, .NET 8

Current NLog config

appsettings.NLog.json

{
  "NLog": {
    "extensions": {
      "010_NLogExtensionsLogging": {
        "assembly": "NLog.Extensions.Logging"
      }
    }
}

appsettings.NLog.WebExtensions.json

{
  "NLog": {
    "extensions": {
      "020_NLogWebExtensions": {
        "assembly": "NLog.Web.AspNetCore"
      }
    }
}

Similar to how the "rules" section works, it would be nice if appsettings support for extensions was extended to allow object-style notation instead of just array-style, particularly because arrays overwrite each other when compositing from multiple config sources so every additional json file that wants to add a single extension has to be hard-coded to include all the extensions added by the files before them.

snakefoot commented 5 months ago

So the goal is to allow to override/merge the extensions using environment-specific appsettings.json ?

Pull-requests are ofcourse wellcome.

TheXenocide commented 4 months ago

Correct (along with other layered sources, e.g. Environment Variables). The rules section of the settings accounts for this specifically, but it looks like extensions was not as fortunate. In our environment we have many separate appsettings files that are packaged with different shared components via their nuget packages which then get layered together.

Previously our general purpose NLog.config would import an NLog.WebExtensions.config, etc. if it existed, and that config file would add the extensions used by the package that included the config. This allowed each separate component to have their own <extensions> section which would only register the extensions that package brought in.

Now that we want to migrate our config to appsettings we'll have our general purpose logging library bring in an appsettings.NLog.config, and then a separate Web Host shared component will bring in an appsettings.NLog.WebExtensions.json (and other packages will include other extensions, like Windows-specific features). Since these can be combined as needed (one app might reference common logging and windows event log; another might reference common logging, windows event log, and web extensions; while another app may only reference common logging and web extensions but not include windows event log).

Since each package is able to provide its own json file and register it with the same DI extension methods used to register the other functionality inside, ideally each appsettings file from their respective packages would only register the extensions that they include/depend on. We don't want to just assume that everything that references the web app so we can't just make the Web package include all the extensions from the other packages because they can be combined in different permutations.

The dictionary-style rules section functionality works great for us and we use the same pattern in some of our own configuration collections and should solve this particular need as well.

snakefoot commented 4 months ago

My time is limited, so I will probably not take any action on this. But pull-requests are ofcourse wellcome.

TheXenocide commented 4 months ago

I'll see if I can manage to get some time allocated for that, though my capacity is pretty limited as well. Fortunately, at least I found this in some very early prototyping so it'll be a little before we're hoping to use it.

TheXenocide commented 4 months ago

Would it be more appropriate for this to be tagged "feature"/"up for grabs" then?

snakefoot commented 4 months ago

Well I really prefer to close exotic issues that has no traction, since it creates noise for what is important/relevant.

But I will leave this issue open for a month as up-for-grabs, but if nothing happens then it will probably be closed at some point.

Notice that with NLog v5.2 (or newer) then one doesn't need to explicity specify these extensions, unless loading NLog Configuration outside the HostBuilder (Before calling AddNLog / UseNLog)

    "extensions": [
      { "assembly": "NLog.Extensions.Logging" },
      { "assembly": "NLog.Web.AspNetCore" }
    ],

So for many use-cases then they can be removed from the appsettings.json.

TheXenocide commented 4 months ago

Oh, that's helpful to know. I'll try that the next time I get to experiment and see if it fits my needs. If it works out-of-box for us I doubt I'll put in the energy to submit a PR, but I may be able to if something is still needed. Does the extension already need to be loaded into the current AppDomain/AssemblyLoadContext when the host is built for it to automatically find them?

snakefoot commented 4 months ago

Does the extension already need to be loaded into the current AppDomain/AssemblyLoadContext when the host is built for it to automatically find them?

NLog v5 disabled the automatic extension loading and assembly-file-scanning. NLog v5.2 improved the manual extension loading so it supports application trimming. NLog v5.2 also extended UseNLog / AddNLog to include manual extension loading, instead of relying on dynamic loading of extension-assemblies.

When having specified an extension-assembly in the NLog-configuration-file, then NLog will use Assembly.Load of assembly-name when using "assembly": "MyExtension", and NLog will use Assembly.LoadFrom when using "assemblyFile": "MyExtension.dll".

TheXenocide commented 4 months ago

If we can register them easily enough from code I don't think we'll need the appsettings support mentioned here; each component has/can have its own extension method for configuring the host so that sounds like a great place to implement this desired behavior. Thanks kindly for the insight.

snakefoot commented 4 months ago

The important part is to perform the registration of extensions upfront, before loading the actual NLog configuration.

Sent from my Galaxy

-------- Original message -------- From: Jason DiOrio @.> Date: 29/04/2024 21:13 (GMT+01:00) To: "NLog/NLog.Extensions.Logging" @.> Cc: Rolf Kristensen @.>, Comment @.> Subject: Re: [NLog/NLog.Extensions.Logging] Allow appsettings.json to specify extensions as keyed object (Issue #735)

If we can register them easily enough from code I don't think we'll need the appsettings support mentioned here; each component has/can have its own extension method for configuring the host so that sounds like a great place to implement this desired behavior. Thanks kindly for the insight.

— Reply to this email directly, view it on GitHubhttps://github.com/NLog/NLog.Extensions.Logging/issues/735#issuecomment-2083478069, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACXZ7HBM652FSE72LCQN7WDY72LW7AVCNFSM6AAAAABGHW3MC2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBTGQ3TQMBWHE. You are receiving this because you commented.Message ID: @.***>