feat(Logical Context): The equivalent to log4net.ThreadLogicalContext

[bfcamara]

Adds Support for the equivalent to log4net.ThreadLogicalContext

The MappedDiagnosticsContext stores the dictionary in the thread locall storage, which it does not flow in async points. For example, if I am using the async/await pattern in my web application, and I want to log a specific property (being set in the begining of the request processing), in all my log statements, if I use the MappedDiagnosticsContext, I will lose the property in logs as soon the request jump to another thread.

This PR will allows us to use a class,MappedDiagnosticsLogicalContext, which uses the CallContext to store the items. A new layout renderer is included as well (${mdlc}).

[toddmeinershagen]

Hi Bruno - thanks for this submission. You know - I wonder if this would be better for the NLog project overall. It seems to be a bug fix for the existing MappedDiagnosticContext that does not work properly in asynchronous situations.

Have you tried submitting this directly to NLog (https://github.com/NLog/NLog)? If not, would you like me to work with the owner of that project to incorporate these changes?

Let me know.

Todd

[ghost]

It was me how recommended it to be contributed here first. The reason being
that it would released to the public more quickly. And that this repository
in the future could be used to help determine the features which would be
included in the official release of nlog.

[bfcamara]

Hi @toddmeinershagen,

my first approach was to open an issue in the NLog project

NLog/NLog#272

As @Xharze said, I just followed the @Xharze suggestion, who recommended to contribute here first. I understand the reasons pointed by @Xharze, and I think he is right. Anyway, and if you consider to merge the PR, who better than you to decide what is the correct project to receive the PR.

I don't consider that this problem is a bug in the MappedDiagnosticsContext, since I think it is clear that the MappedDiagnostcisContext (MDC) is the thread context, and it is equivalent to the log4net MDC, which in the current implementation forwards the items to the ThreadContext.Properties, which in turn is bound to the thread local storage.

https://logging.apache.org/log4net/release/sdk/log4net.MDC.html

I think this is more a new feature, equivalent to the log4net ThreadLogicalContext

http://logging.apache.org/log4net/release/manual/contexts.html

[toddmeinershagen]

Okay - we can include this as part of the NLogContrib project then. I will work on incorporating and getting it to package out to NuGet.

A couple of things I noticed:

1. When we do the check for the dictionary, if it does not exist we create the dictionary and load it into the CallContext. I recommend that we lock there to make sure that this is thread-safe. (I will add this to the code.) Everything else should be thread-safe from the ConcurrentDictionary.
2. Wanted to get your thoughts on calling this AsyncableMappedDiagnosticsContext (amdc) instead to make it immediately obvious to users what it provides feature-wise (async) rather than how it is implemented (CallContext). This seems consistent with NLog's choice of MappedDiagnosticContext instead of log4net's ThreadContext.

Thoughts?

Todd

[toddmeinershagen]

You know - now that I think about it, CallContext should have slots that are unique to a logical thread. So, there wouldn't be multiple threads accessing that same slot - right? So, I guess there is no need to add locking (I was going to do the double lock check pattern) in this case.

Can you confirm that?

[bfcamara]

@toddmeinershagen

Regarding point 1, I think that it is not necessary to use a locking mechanism when creating the ConcurrentDictionary, since the slots provided by callcontext are not shared between threads..

Regarding point 2, I understand your point of view and I like the name you AsyncableMappedDiagnosticsContext. My only concern about this name is about those developers that have used log4net for a while and are trying to find what class should be used to get the equivalent behavior of the LogicalThreadContext. I know that MappedDiagnosticsLogicalContext (the name that I chose) does not exists either in log4net, but I think it drives developer to remind some similarities. MappedDiagnosticsContext exists in log4net, in the form of MDC, which is currently deprecated and replaced by ThreadContext. So, AsyncableMappedDiagnosticsContext (amdc) is a good name. Eventually, in the class documentation it will be useful to mention the log4net.ThreadLogicalContext.

I hope it helps

[UgurAldanmaz]

@bfcamara
I read code and found some difference between MappedDiagnosticsContext and MappedDiagnosticsLogicalContext.

MappedDiagnosticsLogicalContext has only one Set method that accepts string value. Also it stores values in ConcurrentDictionary that TVal is string.

But MappedDiagnosticsContext has two Set methods that accepts string and object values. Also it stores values in Dictionary that TVal is object.

In other words, MappedDiagnosticsLogicalContext does not store object type values while MappedDiagnosticsContext do.

Why does this difference exist? Is there any problem with using object value in MappedDiagnosticsLogicalContext.

I read this issue to find why, but I could not see any reason. Maybe it is so trivial, but I want to ask.

[bfcamara]

@brutalcode

The support to storing objects in MappedDiagnosticsContext is recent, since last June. You can check the commit here NLog/NLog@db66454. The MappedDiagnosticLogicalContextwas added 2 years ago.

I think there is no reason to not use the same approach in MappedDiagnosticLogicalContext as long as the objects being added to the concurrent dictionary are serializable. The MappedDiagnosticLogicalContext was mainly written with await/async in mind, in order to have values flowing between threads. By that time I just needed to have string values.

[UgurAldanmaz]

@bfcamara Thank you for response! You are right. I can see the reason.

I am working for MDLC wiki page, and to do this, I am reading the source code both of them (MappedDiagnosticsLogicalContext and MappedDiagnosticsContext).

So I just needed to ask you to write wiki page correctly.

Thank you for your efforts!

By the way, I need to be sure for its wiki page before save it. Could you help me?

[304NotModified]

@brutalcode. If the page is not in the index yet, there is no problem to save it.

And see also NLog/NLog#883

[Fresa]

The implementation does have some quirky behavior when you are using a mutable dictionary. You should change to an immutable dictionary since CallContext uses shallow copy on write on thread/task delegation.

Mr Cleary explains it well in his blog: http://blog.stephencleary.com/2013/04/implicit-async-context-asynclocal.html

[304NotModified]

Thanks for the info.

Note: this repository is deprecated. This feature has been moved to NLog core.

[Fresa]

I noticed that, so I've created an issue in the NLog repo instead.

[304NotModified]

Thanks!