Rerun logging setup on NLog config change

[emlautarom1]

Fixes #5847

Changes

• Rerun logging setup when NLog detects a change to the NLog.config file.

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• New feature (a non-breaking change that adds functionality)
• Breaking change (a change that causes existing functionality not to work as expected)
• Optimization
• Refactoring
• Documentation update
• Build-related changes

Testing

Requires testing

• Yes
• No

If yes, did you write tests?

• Yes
• No

Notes on testing

Tested manually on a running Nethermind instance.

Documentation

Requires documentation update

• Yes
• No

Requires explanation in Release Notes

• Yes
• No

[emlautarom1]

What are the best practices when dealing with this kind of event handler?

I've read that you could use LogManager.ConfigurationChanged = null to remove all handlers, but what if we attach a new one on other parts of the code?

Ideally, we would only remove this handler during Shutdown.

[Demuirgos]

is that a static event? and do we want to attach a handler to it every time we create a new NLogManager ?

[emlautarom1]

Yes, it is:

public static event EventHandler<LoggingConfigurationChangedEventArgs> ConfigurationChanged;

NLogManager is created in two scenarios (ignoring tests):

nethermind/src/Nethermind/Nethermind.Runner/Program.cs

Line 101 in 90a18eb

return new NLogManager("logs.txt").GetClassLogger();

nethermind/src/Nethermind/Nethermind.Runner/Program.cs

Line 163 in 90a18eb

NLogManager logManager = new(initConfig.LogFileName, initConfig.LogDirectory, initConfig.LogRules);

IMO, it should be treated as a singleton (does it even make sense to have two NLog instances?)

[tkstanczak]

make IDisposable and make sure it unregisters them later?

[LukaszRozmej]

Why do you need to unregister it at all if you want it to live throughout the whole application lifecycle?
Or like Tomasz said make NLogManager or ILogManager implement IDisposable unhook there and leave it to user to dispose the manager. (Or if you don't trust your users you can use disposable/finalizer pattern)

[emlautarom1]

Why do you need to unregister it at all if you want it to live throughout the whole application lifecycle?

That's the thing: I do not know if we want/need to unregister it. We already have a shutdown method that kind of looks like Dispose, but from reading the source of NLog I don't see that it removes the event handler (https://github.com/NLog/NLog/blob/3ff3d5b2ab091e2171e81c410b4c4aa558aee4ba/src/NLog/LogFactory.cs#L1020).

I think that it should not be problematic to do nothing, but I wanted to double check with someone who has worked with NLog and EventHandlers before.

[LukaszRozmej]

If we are shutting down you can kind of ignore things that are not crucial for shutting down. But Also you can make it IDisposable and add it to DisposeStack - a stack of things that will be disposed during shutdown

[emlautarom1]

So I guess our options are:

1. Do nothing with the event handler since it lasts for the whole application lifetime.
2. Use IDisposable and the dispose pattern to ensure that we remove the event handler.
3. Change the config to use Configuration-Variables, removing the requirement of running SetupLogFile and SetupLogDirectory (but what about SetupLogRules?).

I think 1. should be good enough.

[LukaszRozmej]

I would add IDisposable for completeness sake - so 2.- but shouldn't affect the end result,

[snakefoot]

It is recommended to use Configuration-Variables or GlobalDiagnosticsContext, instead of modifying NLog-configuration at runtime.

Ex. using FileTarget.FileName="${gdc:AppDirectory}RandomFile.txt" or LoggingRule.MinLevel="${gdc:AppMinLevel:whenEmpty=Warn}"

See also: https://github.com/NLog/NLog/wiki/Environment-specific-NLog-Logging-Configuration

See also: https://github.com/NLog/NLog/wiki/Filtering-log-messages#semi-dynamic-routing-rules

One could also use NLog-Configuration-Variables:

<nlog autoReload="true">
    <variable name="BaseDir" value="${basedir}" />
    <variable name="AppLevel" value="Info" />
    <targets>
          <target name="logfile" xsi:type="file" filename="${basedir}/logfile.txt" />
    </targets>
    <minlevel>
          <logger minlevel="${AppLevel}" writeTo="logfile" />
    </minlevel>
</nlog>

Then during application startup:

NLog.LogManager.Configuration.Variables["BaseDir"] = appBaseDir; // Will survieve future config-reloads
NLog.LogManager.Configuration = NLog.LogManager.Configuration.Reload();

[LukaszRozmej]

I would add IDisposable for completeness sake - so 2.- but shouldn't affect the end result,

[snakefoot]

Notice that NLog.LogManager.ConfigurationChanged will also trigger when NLog.LogManager.Configuration is reset:

NLog.LogManager.Shutdown(); // Similar to NLog.LogManager.Configuration = null

This has been handled here:

if (LogManager.Configuration?.AllTargets is not null)

But here it can throw with NullReferenceException:

IList<LoggingRule> configurationLoggingRules = LogManager.Configuration.LoggingRules;

Maybe consider changing the logic to only patch the NLog-configuration when assigned:

_logManagerOnConfigurationChanged = (sender, args) => { if (args.ActivatedConfiguration != null) Setup(logFileName, logDirectory, logRules); };