Logger with support for WithProperty and WithLogicalContext

[snakefoot]

Maybe consider to extend with xml-config support as well. See also #528

But then it needs to be a capture of a functor, that either returns the WithProperty-object, or calls Layout-render.

---

This change is 

[codecov]

Codecov Report

Merging #2496 into dev will increase coverage by 1%.
The diff coverage is 71%.

@@           Coverage Diff           @@
##             dev   #2496     +/-   ##
=======================================
+ Coverage     81%     81%     +1%     
=======================================
  Files        327     322      -5     
  Lines      24604   23342   -1262     
  Branches    3150    2911    -239     
=======================================
- Hits       19822   19003    -819     
+ Misses      3920    3556    -364     
+ Partials     862     783     -79

[304NotModified]

Could you please explain me why we're creating new (temporary guid) loggers?

Is it for the following cases?

var logger1 = LogManager.GetLogger("logger1");
var logger2 = logger1.WithProperty("prop1", "val1");

logger1 wont add the property prop1, but logger 2 does. Is that correct?

Discussion: if logger1 would also add prop1 (and maybe rename it to AddProperty instead of WithProperty), then no temp loggers are needed? Do you agree?

Benefit in that case, this would work also:

var logger1 = LogManager.GetLogger("logger1");
 logger1.AddProperty("prop1", "val1"); //don't use the return value

[snakefoot]

@304NotModified I was thinking of a usage pattern similar to what I can see in Serilog-ForContext, where the newly created logger can be handed over to a new owner.

If wanting multiple loggers with the same name, that are initialized with different property-values, then your solution will not work (adding a property will affect the global logger instance used by all).

[304NotModified]

If wanting multiple loggers with the same name, that are initialized with different property-values, then your solution will not work.

true, but it would be easier to merge with the XML config (#528)

doubting about this. What do self think about the temporary guid loggers?

[snakefoot]

@304NotModified But it would be easier to merge with the XML config #528

Yes the properties added using logging-rules will be "global" and should be automatically applied to the main-logger.

[snakefoot]

Must admit I don't need the overhead from either WithProperty or logging-rules-properties. Though I guess their overhead is smaller compared to using MDLC.

But I wanted to test out different solutions for solving #2465, because I can see lots of NLog targets not supporting logging-context very well (besides the logger-name).

The alternative is #2467 that implements the WithProperty using the Target-ContextProperties, though it requires modification of the custom NLog-targets to inherit from the new abstract TargetWithContext. (This PR gives the user the ability to add custom properties independent of target inherits from TargetWithContext)

[304NotModified]

I was looking at this.

I think changing it to AddLogicalContext and AddProperty would be more in sync with the current GDC/MDC etc. (or maybe even CaptureLogicalContext and SetProperty)

Also it makes it technical easier and more straight forward.

Must admit I don't need the overhead from either WithProperty or logging-rules-properties. Though I guess their overhead is smaller compared to using MDLC.

it's a bit magic IMO, and I hope it's not needed. Also the non-destructive update is nice, but (unfortunately?) not in-line with the current methods.

So my proposal is to change this to AddLogicalContext/AddProperty or CaptureLogicalContext/SetProperty

Maybe we should postpone this to after 4.5?

[snakefoot]

Maybe we should postpone this to after 4.5?

Good idea

[snakefoot]

if logger1 would also add prop1 (and maybe rename it to AddProperty instead of WithProperty), then no temp loggers are needed? Do you agree?

So my proposal is to change this to AddLogicalContext/AddProperty or CaptureLogicalContext/SetProperty

Think it should be two PRs. Think both solutions are very valid.

• Properties-IDictionary - That affects all active logger objects with same name (Can also be configured from XML, maybe with Layout-ability)
• WithProperty -That only affects the returned logger object (This PR)

I don't agree that the first solution solves the second scenario. See also https://stackoverflow.com/questions/48992861/nlog-how-to-use-class-member-variable-in-log (Same logger is used in different contexts)

[304NotModified]

This PR is still open and I like to give it an direction.

While I like fluent style programming, I'm doubting if that will fit in the current NLog API. In NLog it's not common to create multiple loggers in that way. But there is a fluent style already in the NLog.Fluent namespace.

Also, if we really need a like to add a fluent calls, like .WithProperty, then I think it needs to return an intermediate (state) object, and it should not reuse the Logger for that. I really think it's a bad idea to create guid-temporary loggers and prune them.

So my proposal is:

• or do a fluent style and use an intermediate (state) object, and make it consistent with NLog.Fluent stuff.
• or add SetProperty and change the logger object.

[304NotModified]

polite bump @snakefoot :)

[snakefoot]

@304NotModified Well I like WithProperty, what if I change the logic, so instead of using a Guid, then the LoggerCache becomes:

Dictionary<LoggerCacheKey, KeyValuePair<WeakReference, IList<WeakReference>> _loggerCache;

So instead of using Guid, then Loggers with special context is inserted in the IList<WeakReferences>?

[304NotModified]

So my proposal is:

or do a fluent style and use an intermediate (state) object, and make it consistent with NLog.Fluent stuff.
or add SetProperty and change the logger object.

You don't like one of these proposals?

Guid of weakreference, it's has the same issue (temp loggers and prune, but without guid)

really think it's a bad idea to create guid-temporary loggers and prune them.

[snakefoot]

or do a fluent style and use an intermediate (state) object,

The returned state object should be a Logger-object or an ILogger-object. Sadly enough the ILogger-interface is broken by design. So I prefer a Logger-object.

add SetProperty and change the logger object.

Probably useful in some cases, but I prefer to have the dynamic first, and then introduce the global/fixed property afterwards (easy part). To make sure the interface and the naming becomes similar and not apples and potatoes.

[snakefoot]

Closing as superseded by #3298