-
Notifications
You must be signed in to change notification settings - Fork 153
Pass objects without toString conversion to Nlog MDC #179
Conversation
Usually one checks for the optimal-method, and if not available then it performs fallback. Now you have just replaced one with another without knowing which version has the new methods. Also there can be dangers of writing non-string-objects into the CallContext of log4net. |
@snakefoot thanks for bringing. what is the minimum supported NLog version for liblog? and can you also share where the approach with optimal-method checks is used in liblog? |
Not for me to investigate. I'm just stating the obvious. |
Created #181 for you |
|
Again AppDomains on IIS can throw random exception for complex objects saved to CallContext without ObjectHandle. |
if you're referring to the log4net issue, I can easily revert the change and keep the NLog piece only |
If you continue to write to MDC for NLog, then I have no objects against changing from string-value to object-value. Just change the reflection code to first check for typeof(object) and then fallback to typeof(string). But MDLC is needed for async-Task-context to work correctly. See #181 What you do for log4net, that I have no opinion about. Maybe use object when on NETSTANDARD, as log4net then uses AsyncLocal, that doesn't have the CallContext-restrictions. |
@damianh what do you think? |
It is on purpose that NDLC + MDLC is not used for NLog 4.5 (Will work with Nlog 4.6). See also #228 (comment) @damianh Guess this PR can be closed |
@AndreyNS NLog 4.6 has now been released: https://www.nuget.org/packages/NLog. Created #233 that updates the LibLog Test project. So just update to NLog 4.6 and all your worries are over |
should I close the PR? |
Fine by me. Unless you feel something is missing.
|
OpenMappedContext method already accepts object values, but NLog and log4net implementations cast the values to strings
The PR removes the cast
@damianh please take a look