NLog / NLog.Web

NLog integration for ASP.NET & ASP.NET Core 2-8
https://nlog-project.org
BSD 3-Clause "New" or "Revised" License
320 stars 166 forks source link

Could not create instance of custom target (when using dependency injection) #248

Closed marrrcin closed 5 years ago

marrrcin commented 6 years ago

I have an ASP.NET Core 2.0 web application. In the Main method of the app, when I build WebHostBuilder I use .UseNLog() method to enable NLog (after .UseStartup but I've tested it also whe .UseNLog() was before .UseStartup). I have the following custom target:

    [Target("RemoteNLogTarget")]
    public class RemoteNLogTarget : TargetWithLayout
    {
        protected ILogStorage LogStorage;

        public RemoteNLogTarget(ILogStorage logStorage)
        {
            LogStorage = logStorage;
        }

        protected override void Write(LogEventInfo logEvent)
        {
            LogStorage.Store(Layout.Render(logEvent));
        }
    }

ILogStorage is the following:

public interface ILogStorage
    {
        void Store(string logMessage);
    }

I have implementation of ILogStorage it registered in services (as scoped) in my Startup.

In Startup.Configure method I have the following code, which should enable me to use custom targets with dependency injection as per: https://stackoverflow.com/a/42101946/1955346

 var builtInProvider = ConfigurationItemFactory.Default.CreateInstance;
            ConfigurationItemFactory.Default.CreateInstance = type =>
            {
                return serviceProvider.GetService(type) ?? builtInProvider(type);
            };
            Target.Register<RemoteNLogTarget>("RemoteNLogTarget");

Finally, my nlog.config:

<extensions>
   <add assembly="MyAssembly"/>
</extensions>
<targets>
      <target xsi:type="RemoteNLogTarget" name="RemoteNLogTarget"
            layout="${pad:fixedLength=True:padding=-5:${uppercase:${level}}} | ${longdate} | ${processtime} | ${logger} | ${message} | ${exception:format=toString,Data}"
            />
</targets>

Regardless of having <extensions> declared or not, regardless of having Target.Register<> called or not, I always end up with the exception :

Cannot access the constructor of type: RemoteNLogTarget. Is the required permission granted?

from: https://github.com/NLog/NLog/blob/fd6e91a2daed54570a8e89cc2575e0e11158343f/src/NLog/Internal/FactoryHelper.cs#L57

It seems to me like setting CreateInstance does not work there. Fun fact is that when I initialize my application, my custom DI method is actually called (and obviously returns proper instance of my RemoteNLogTarget), but the exception is thrown anyway.

snakefoot commented 6 years ago

Its trying to call a parameter-less contructor, but can find none. Maybe the error text should be improved :)

marrrcin commented 6 years ago

I know that it's trying to call parameter-less constructor, but I don't care about error message - the problem is that I cannot use DI in my custom target.

304NotModified commented 6 years ago

@marrrcin could you debug if it's calling your ConfigurationItemFactory.Default.CreateInstance?

So:

var builtInProvider = ConfigurationItemFactory.Default.CreateInstance; ConfigurationItemFactory.Default.CreateInstance = type => { //will we get here? <--------------------------------------------- return serviceProvider.GetService(type) ?? builtInProvider(type); }; Target.Register("RemoteNLogTarget");

snakefoot commented 6 years ago

Think you have to create a pull-request for the NLog project where one can provide a custom factory method when registering the target.

Sent from my Samsung device

-------- Original message -------- From: Marcin Zabłocki notifications@github.com Date: 12/02/2018 09:03 (GMT+00:00) To: "NLog/NLog.Web" NLog.Web@noreply.github.com Cc: Rolf Kristensen sweaty1@hotmail.com, Comment comment@noreply.github.com Subject: Re: [NLog/NLog.Web] Could not create instance of custom target (when using dependency injection) (#248)

I know that it's trying to call parameter-less constructor, but I don't care about error message - the problem is that I cannot use DI in my custom target.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/NLog/NLog.Web/issues/248#issuecomment-364863064, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AK-fnJAp2KBqA8EXRRhOSfoqIR9HXvnuks5tT_5ggaJpZM4SBEnB.

marrrcin commented 6 years ago

@304NotModified like I said in the last paragraph:

Fun fact is that when I initialize my application, my custom DI method is actually called (and obviously returns proper instance of my RemoteNLogTarget), but the exception is thrown anyway.

Yes, debugger stops there and my serviceProvider returns proper instance, but NLog code seems to slip out from this ConfigurationItemFactory.Default.CreateInstance provider and calls NLog.Internal.FactoryHelper.CreateInstance instead.

304NotModified commented 6 years ago

Do you have a stacktrace? The issue is in XmlLoggingConfiguration.AddArrayItemFromElement?

304NotModified commented 6 years ago

Bump

marrrcin commented 6 years ago

Here's the stacktrace from internal NLog log:

NLog.NLogConfigurationException: Cannot access the constructor of type: MyNamespace.RemoteNLogTarget. Is the required permission granted?
   at NLog.Internal.FactoryHelper.CreateInstance(Type t)
   at NLog.Config.Factory`2.TryCreateInstance(String itemName, TBaseType& result)
   at NLog.Config.Factory`2.CreateInstance(String name)
   at NLog.Config.XmlLoggingConfiguration.ParseTargetsElement(NLogXmlElement targetsElement)
   at NLog.Config.XmlLoggingConfiguration.ParseNLogElement(NLogXmlElement nlogElement, String filePath, Boolean autoReloadDefault)
   at NLog.Config.XmlLoggingConfiguration.Initialize(XmlReader reader, String fileName, Boolean ignoreErrors)
   --- End of inner exception stack trace ---
marrrcin commented 6 years ago

Bump

snakefoot commented 6 years ago

@marrrcin A workaround could be to instantiate a custom inner-class during InitializeTarget, that can trigger the dependency injection properly. Then you can grab the ILogStorage as a property from that custom inner-class. Just an idea.

marrrcin commented 6 years ago

@snakefoot could you provide a code snippet for that?

snakefoot commented 6 years ago

Was hoping you knew your own chosen dependency injection framework better than me, and how to instantiate objects with it

marrrcin commented 6 years ago

Do you suggest using service locator here?

304NotModified commented 6 years ago

Is this still an issue?

marrrcin commented 6 years ago

Did you changed something related to this in NLog.Web.AspNetCore 4.5.1 ?

304NotModified commented 6 years ago

Nope, but it should work, so I don't get it why it doesn't work

snakefoot commented 6 years ago

I think the problem is that you update the ConfigurationItemFactory.Default.CreateInstance after the NLog configuration has loaded in Program.cs

304NotModified commented 6 years ago

So reload the config is a quick fix.

A better solution is to update ConfigurationItemFactory.Default.CreateInstance before nlog, which is a bit tricky if static loggers are used.

fairking commented 6 years ago

I had other issue: "Unable to resolve ILogger on HomeController" Asp.Net Core 2.1

So it is actually a bug https://github.com/aspnet/Logging/issues/700

I solved it by adding services.AddSingleton<ILogger>(s => s.GetService<ILogger<Program>>());

Please someone update the documentation as I got stuck doing those steps from https://github.com/NLog/NLog.Web/wiki/Getting-started-with-ASP.NET-Core-2 and lost an hour or so to figure out a problem.

304NotModified commented 6 years ago

@denis-pujdak-adm-it first this is another issue,

second, we aren't injecting ILogger in the "getting started", but ILogger<HomeController> and that is also support and not a bug ( https://github.com/aspnet/Logging/issues/700 tells the same)

fuzzzerd commented 6 years ago

I am also experiencing this issue. It seems that NLog is loading the configuration earlier in the startup sequence Startup.Configure is called.

I'm working with a slightly modified version of the sample for getting started with .NET Core 2.0:

    WebHost.CreateDefaultBuilder(args)
        .UseStartup<Startup>()
        .UseNLog()
        .Build()
        .Run();

In Startup.Configure I have this:

    var builtInProvider = ConfigurationItemFactory.Default.CreateInstance;
    ConfigurationItemFactory.Default.CreateInstance = type =>
    {
        return app.ApplicationServices.GetService(type) ?? builtInProvider(type);
    };
    NLog.Targets.Target.Register("NLogCustomTarget", typeof(NLogCustomTarget));

If I turn on NLog tracing and step through the debugger, I see the same error posted above occurs before I even get to Startup.Configure.

    2018-06-22 15:44:29.6057 Error Parsing configuration from bin\Debug\netcoreapp2.1\NLog.config failed. Exception: NLog.NLogConfigurationException: Exception when parsing \bin\Debug\netcoreapp2.1\NLog.config.  ---> System.ArgumentException: Target cannot be found: 'NLogCustomTarget'
       at NLog.Config.Factory`2.CreateInstance(String name)
       at NLog.Config.XmlLoggingConfiguration.ParseTargetsElement(NLogXmlElement targetsElement)
       at NLog.Config.XmlLoggingConfiguration.ParseNLogElement(NLogXmlElement nlogElement, String filePath, Boolean autoReloadDefault)
       at NLog.Config.XmlLoggingConfiguration.Initialize(XmlReader reader, String fileName, Boolean ignoreErrors)
       --- End of inner exception stack trace ---

What is the recommended approach to inject dependencies into custom targets? I already have all of my configuration being loaded into the services collection; I don't want to re-deploy that data into the NLog configuration file just to get NLog to be able to push data into a service.

304NotModified commented 6 years ago

What is the recommended approach to inject dependencies into custom targets?

overwrite the ConfigurationItemFactory.Default.CreateInstance (as in the first post)

304NotModified commented 6 years ago

Has your question being answered?

fuzzzerd commented 6 years ago

@304NotModified -- I believe I am overwriting the ConfigurationItemFactory.Default.CreateInstance by doing the following (which I pieced together from other links in this thread:

var builtInProvider = ConfigurationItemFactory.Default.CreateInstance;
ConfigurationItemFactory.Default.CreateInstance = type =>
{
    return app.ApplicationServices.GetService(type) ?? builtInProvider(type);
};
NLog.Targets.Target.Register("NLogCustomTarget", typeof(NLogCustomTarget));

While this does produce the desired behavior, NLog still generates an error (silently, I only know about it if I turn on NLog internal logging). I guess my original question wasn't clear and maybe I can clarify: is that internal logging exception expected while using this method? Where should I be overwriting it, I'm doing it in Startup.Config but I've also tried it in Program.Main and I get the same internal exception from NLog:

2018-06-22 15:44:29.6057 Error Parsing configuration from bin\Debug\netcoreapp2.1\NLog.config failed. Exception: NLog.NLogConfigurationException: Exception when parsing \bin\Debug\netcoreapp2.1\NLog.config.  ---> System.ArgumentException: Target cannot be found: 'NLogCustomTarget'
   at NLog.Config.Factory`2.CreateInstance(String name)
   at NLog.Config.XmlLoggingConfiguration.ParseTargetsElement(NLogXmlElement targetsElement)
   at NLog.Config.XmlLoggingConfiguration.ParseNLogElement(NLogXmlElement nlogElement, String filePath, Boolean autoReloadDefault)
   at NLog.Config.XmlLoggingConfiguration.Initialize(XmlReader reader, String fileName, Boolean ignoreErrors)
   --- End of inner exception stack trace ---
304NotModified commented 6 years ago

could you log to the internallogger yourself? (just before NLog.Targets.Target.Register("NLogCustomTarget", typeof(NLogCustomTarget));) - which one is earlier, the register of the error.

While this does produce the desired behavior, NLog still generates an error

I depends, if you are using a static logger and the logger is initalized before the "CreateInstance" overwrite, I would expect that error.

304NotModified commented 5 years ago

Closing this due to inactivity. Please let us know if this still an issue and please provide the requested info - thanks!

fuzzzerd commented 5 years ago

@304NotModified still having errors logged. I'm not 100% confident its a bug in NLog, as it could be that I'm not doing something correctly.

My problem appears to be related to the fact that I have a custom Target defined in my nlog.config and when I call .UseNLog() in program.cs main method it loads that configuration file and the target is not registered yet.

If I move my call of

NLog.Targets.Target.Register("NLogCustomTarget", typeof(NLogCustomTarget)); // register our custom target

ahead of the .UseNLog() call I get the same error OP has here, that the constructor is not accessible, presumably because I am injecting dependencies into the target constructor so it knows where/how to write log entries.

If there is a better/different way to register custom targets with dependency injection I'd love to learn more about it.

Like I said, what I have does work in the sense my target is called and can write log data, but it causes an internal nlog error as well as asp.net core sdtout errors on startup.

snakefoot commented 5 years ago

@fuzzzerd The combination of dynamic logging configuration and dependency injection can give a catch22. You want to have logging up and running early, but this will fail if dependent on custom objects that requires a dependency injection provider is fully loaded.

The "ugly" work-around to this catch22 is to have two constructors for your custom-objects:

Alternative override the CreateInstance-method as first thing (Before creating first Logger-object or loading NLog config). To call the specialized constructor with parameters that signals that it should be in disabled state.

Then one can "just" perform a reload of the NLog config, after the the dependency injection provider has been fully loaded and performed final override of CreateInstance. This will make NLog recreate all configuration items once again, and now with a working dependency injection provider that calls the specialized constructor with correct parameters.

NLog.LogManager.Configuration = NLog.LogManager.Configuration.Reload();

Also created a wiki-page: https://github.com/NLog/NLog/wiki/Dependency-injection-with-NLog

304NotModified commented 5 years ago

I assume your question has been answered, if not, please let us know!

fuzzzerd commented 5 years ago

I have made these modifications to my app and it has corrected the internal nlog errors for me. Thanks for the good write up!