Skip to content
This repository has been archived by the owner on Dec 5, 2019. It is now read-only.

Pass objects without toString conversion to Nlog MDC #179

Closed

Conversation

andrey-noskov
Copy link

@andrey-noskov andrey-noskov commented Sep 5, 2018

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

@snakefoot
Copy link
Contributor

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.

@andrey-noskov
Copy link
Author

@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?

@snakefoot
Copy link
Contributor

thanks for bringing. what is the minimum supported NLog version for liblog?

Not for me to investigate. I'm just stating the obvious.

@snakefoot
Copy link
Contributor

Created #181 for you

@andrey-noskov
Copy link
Author

As of NLog 4.1.1, the Mapped Diagnostics Context supports any Object type, not just String.
yes, you're right. I believe that is an acceptable limitation. @damianh what do you think?

@snakefoot
Copy link
Contributor

Again AppDomains on IIS can throw random exception for complex objects saved to CallContext without ObjectHandle.

@andrey-noskov
Copy link
Author

if you're referring to the log4net issue, I can easily revert the change and keep the NLog piece only

@snakefoot
Copy link
Contributor

snakefoot commented Sep 6, 2018

@AndreyNS 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.

@andrey-noskov andrey-noskov changed the title Pass objects without toString conversion to Nlog and Log4Net MDC Pass objects without toString conversion to Nlog MDC Sep 7, 2018
@andrey-noskov
Copy link
Author

@damianh what do you think?

@snakefoot
Copy link
Contributor

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

@snakefoot
Copy link
Contributor

@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

@andrey-noskov
Copy link
Author

should I close the PR?

@snakefoot
Copy link
Contributor

snakefoot commented Mar 25, 2019 via email

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants