Contrastive behavior of NLogIntegration against NLogs

[devop228]

An "FileNotFound" exception was thrown when I used NLogIntegration without nlog.config:

var logFactory = new NLogFactory();
var log = logFactory.Create("castle.log.Program");
log.Info("Hello world!");

The behavior is in contrast to NLogs. The code below will not throw exception even without nlog.config:

var log = NLog.LogManager.GetCurrentClassLogger();
log.Info("Hello world!");

Even though there is a workaround by creating factory with new NLogFactory(true), but I could not found it anywhere in the documentation. This behavior also propagates to LoggingFacility in Windsor.

I suggest to change the default behavior to matching NLogs as Castle's logging is just a wrapper around NLog and should not change its default behavior, or alternatively to update the documentation highlighting the different.

The version I used:

• netcoreapp2.1
• Castle.Core 4.3.1
• Castle.Core-NLog 4.3.1

[jonorossi]

I'm not sure what you are asking to be changed. Your example code of using NLog directly doesn't configure anything, they aren't doing the same thing, so I'm not sure why they should match.

The XML docs could be improved to make it clear the default constructor will configure NLog, or are you asking why doesn't the default constructor do nothing and just assume it'll be configured externally? My guess is that NLog probably did the same many years ago and they changed their API not to load that file by default, maybe with the changes to support .NET Core which doesn't have System.Configuration.

The XML docs could do with some general improvement and that reference to log4net should also be fixed.

/// <summary>
///   Initializes a new instance of the <see cref="NLogFactory" /> class.
/// </summary>
public NLogFactory()
    : this(defaultConfigFileName)
{
}

/// <summary>
///   Initializes a new instance of the <see cref="NLogFactory" /> class.
/// </summary>
/// <param name="configuredExternally">If <c>true</c>. Skips the initialization of log4net assuming it will happen externally. Useful if you're using another framework that wants to take over configuration of NLog.</param>
public NLogFactory(bool configuredExternally)
{
    if (configuredExternally)
    {
        return;
    }

    var file = GetConfigFile(defaultConfigFileName);
    LogManager.Configuration = new XmlLoggingConfiguration(file.FullName);
}

/// <summary>
///   Initializes a new instance of the <see cref="NLogFactory" /> class.
/// </summary>
/// <param name="configFile"> The config file. </param>
public NLogFactory(string configFile)
{
    var file = GetConfigFile(configFile);
    LogManager.Configuration = new XmlLoggingConfiguration(file.FullName);
}

/// <summary>
///   Initializes a new instance of the <see cref="NLogFactory" /> class.
/// </summary>
/// <param name="loggingConfiguration"> The NLog Configuration </param>
public NLogFactory(LoggingConfiguration loggingConfiguration)
{
    LogManager.Configuration = loggingConfiguration;
}

[devop228]

What I tried to point out is a undocumened behavior difference between NLogIntegration and NLog when they are not explicitly configured. NLog does not throw an FileNotFound exception while NLogIntegration does.

I would suggest a change to NLogFactory's parameterless construtor, instead of configuring it with defaultConfigFileName, it should use external configuration, which will match NLog's behavior when you do no explicit configuration.

/// <summary>
///   Initializes a new instance of the <see cref="NLogFactory" /> class.
/// </summary>
public NLogFactory()
    : this(true)
{
}

An alternative change is to document the different:

/// <summary>
///   Initializes a new instance of the <see cref="NLogFactory" /> class 
///   with <b>nlog.config</b> configuration file.
/// </summary>
public NLogFactory()
    : this(defaultConfigFileName)
{
}

[snakefoot]

I'm not sure what you are asking to be changed. Your example code of using NLog directly doesn't configure anything, they aren't doing the same thing, so I'm not sure why they should match.

NLog will automatically scan for NLog.config and attempt configure itself. So saying that no configuration is happening is not completely true.

See also https://github.com/nlog/NLog/wiki/Configuration-file#configuration-file-locations

[jonorossi]

Thanks @devop228 and @snakefoot, I don't use NLog so I didn't know that is how it works, thanks for correcting me.

This Castle code was written over a decade ago and I'm not against changing it, I actually don't see any reason not to drop all code that configures NLog for you, and force the user to configure NLog with NLog's API. I don't recall NLog having such a comprehensive list of files/locations it could load config from many years ago, so getting rid of these would simplify the code here. When we added Serilog support more recently that is exactly what we did, you have to configure Serilog externally.

[snakefoot]

Created #419

[jonorossi]

I suggest to change the default behavior to matching NLogs as Castle's logging is just a wrapper around NLog and should not change its default behavior, or alternatively to update the documentation highlighting the different.

@devop228 Castle's logging isn't an NLog wrapper and never intended to be. Castle code was written without having to know what sink is hooked up, just that it can get an ILogger from an ILoggerFactory. Unfortunately these factories configured things for users to follow convention-over-configuration principles, however 10+ years later this is now getting in the way. As I said I'm not against changing things, and actually proposed a change in October.

I've closed the pull request. I think we need to make the larger changes we've been talking about for a a couple of years to the logging facility and logging abstractions to simplify them rather than small changes which make things inconsistent.

Currently you can do the following with Windsor:

container.AddFacility<LoggingFacility>(f => f
    .LogUsing<Log4netFactory>()
    .ConfiguredExternally()
);

I think we should work towards removing all configuration being automatically discovered/injected, especially since most developers configure logging before Windsor so they can log stuff right from startup. Once we remove this stuff you won't need the ConfiguredExternally() call anymore. We've floated a few ideas in Windsor's issue tracker on how a brand new logging facility without external dependencies could work.