Control throwing of NLogConfigurationExceptions (LogManager.ThrowConfigExceptions)

[304NotModified]

Added ThrowConfigExceptions to control throwing of NLogConfigurationException. Before NLogConfigurationException where always thrown, which could lead to System.TypeInitializationException which is bad and confusing as sending info with the exception is difficult - see stackoverflow.

LogManager.ThrowConfigExceptions  = true | false | null

<nlog throwConfigExceptions="true"|"false"|"">

if ThrowConfigExceptions is null (or empty string from XML), then ThrowExceptions is used. So ThrowConfigExceptions inherits from ThrowExceptions.

This isn't fully backwards-compatible, but it is also a bug as we in code and docs is stated "By default exceptions are not thrown under any circumstances.". also this is a good change to include with #1143

With the addition of #1222, there is no good reason to throw exceptions even with config errors.

[codecov-io]

Current coverage is 74.67%

Merging #1238 into master will increase coverage by +0.01% as of 73bd2c5

@@            master   #1238   diff @@
======================================
  Files          267     267       
  Stmts        15412   15425    +13
  Branches      1615    1618     +3
  Methods          0       0       
======================================
+ Hit          11507   11519    +12
- Partial        377     378     +1
  Missed        3528    3528       

Review entire Coverage Diff as of 73bd2c5

---

Uncovered Suggestions

1. +0.09% via ...gingConfiguration.cs#282...295
2. +0.09% via ...gingConfiguration.cs#248...261
3. +0.09% via ...argets/FileTarget.cs#1704...1716
4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

[304NotModified]

UnitTests.Common.InternalLoggerTests.CreateDirectoriesIfNeededTests

Assert.False() Failure

[304NotModified]

@bhaeussermann I would like to have your opinion on this one :)

[bhaeussermann]

@304NotModified This seems to be the best solution. I'm also happy with the code.

However, what is going to happen if the configuration used is ThrowExceptions = true and ThrowConfigExceptions = false?
How about we throw an exception whenever Log() is called saying something like Configuration failed. See internal log for more details.?

[304NotModified]

@304NotModified This seems to be the best solution. I'm also happy with the code.

Thanks for the check!

However, what is going to happen if the configuration used is ThrowExceptions = true and ThrowConfigExceptions = false?

It will throw runtime exceptions and not config exceptions.

How about we throw an exception whenever Log() is called saying something like Configuration failed. See internal log for more details.?

Nice idea, but we like to throw less exceptions (by default!) because:

• We state that we never throw exceptions
• Throwing exceptions can lead to TypeInitializationException, which are hard to understand (sometimes even no innerException), hard to debug and can't be prevented (without big loss of performance), see stackoverflow

[bhaeussermann]

However, what is going to happen if the configuration used is ThrowExceptions = true and ThrowConfigExceptions = false?

It will throw runtime exceptions and not config exceptions.

My suggestion applied to this situation. What runtime exception would this throw? Would it be clear that something went wrong with configuration? I cannot check that right now.

If it is not clear from that "runtime" exception, then perhaps we should add a check to throw a more meaningful exception for this situation telling the developer to check the internal log.

[bhaeussermann]

(My comment above assumes the situation where there is an error in the configuration.)

[304NotModified]

I'm not sure if we are on the same line.

So we have now with the PR two properties:

• ThrowExceptions: throw runtime exceptions like, invalid query, unreachable database, wrong filename (in some cases)
• ThrowConfigExceptions: throws exceptions like unknown targetname, unknown target property , unknown layout renderer.

All the exceptions get logged to the internal logger.

ThrowExceptions was already in NLog (with default false). ThrowConfigExceptions is new (and you can see it as before true and since this PR false)

So it's indeed less clear when there is a config mistake now, but this PR will also lead to less errors at runtime (config mistake can be also at runtime when changing the config) and no unclear TypeInitializationException by default :)

If you like the previous behavior, just change ThrowConfigExceptions=true

[304NotModified]

docs:

• https://github.com/NLog/NLog/wiki/Configuration-file