Added WithProperty and SetProperty on Logger#3298
Conversation
b44e2a2 to
84304df
Compare
…included for all LogEvents
84304df to
47f861d
Compare
Codecov Report
@@ Coverage Diff @@
## dev #3298 +/- ##
======================================
+ Coverage 80% 80% +<1%
======================================
Files 356 356
Lines 28077 28123 +46
Branches 3739 3753 +14
======================================
+ Hits 22485 22551 +66
+ Misses 4488 4481 -7
+ Partials 1104 1091 -13 |
185ea00 to
47f861d
Compare
|
Thanks for the PR, but i'm afraid it won't change the NLog API viewpoint, mentioned in #2496 (comment)? |
|
The Logger currently has name that matches the context it represents. For every context you create a new logger.
This PR allows you to inject more detail into the context that the Logger represents (besides its name).
I don't like global context properties that affects all users of the same logger. Kind of ruins the idea of context.
|
|
If we also add SetProperty, next to WithProperty, do you think that is OK? (from viewpoint API design) |
|
@304NotModified Added a |
|
Cool! |
|
Need to check it in depth, but I think I will merge it soon :) |
e910771 to
7585208
Compare
304NotModified
left a comment
There was a problem hiding this comment.
Thanks!
Is this safe for 4.6.3?
| { | ||
| if (IsEnabled(logEvent.Level)) | ||
| var targetsForLevel = IsEnabled(logEvent.Level) ? GetTargetsForLevel(logEvent.Level) : null; | ||
| if (targetsForLevel != null) |
There was a problem hiding this comment.
is this a performance tweak?
|
Think it is safe for NLog 4.6.3
Not sure what you are referring to regarding "performance tweak"
|
|
Docs added here: https://github.com/NLog/NLog/wiki/EventProperties-Layout-Renderer |
|
Updated the docs a little so the difference between |
|
It's not a trap but a feature :) |
|
Updated it with a 2nd log. The set is really useful is some cases (e.g when you get more context along the way) |
Not when having the global NLog Logger-cache where multiple locations are referencing the same Logger-object, and then one location suddenly decides to call SetProperty (can become a surprise for everyone else) |
|
It's not suddenly? It's by (logger) name. |
|
Buy anyway, I'm curious when we will implement WithProperties and ClearProperty :p (Maybe nice with default interfaces - c# 8) |
Not sure I understand. There is no guarantee that two isolated locations doesn't call Could also be two threads allocating their isolated controller of the same class, then the two controller-classes will be sharing the same Logger-object (Independent of using static Logger-variable, but just because it calls That is why I prefer |
Logger - WithProperty for adding Logger specific properties that are included for all LogEvents
Different attempt of implementing #2496
Still trying to resolve #528
This change is