Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added WithProperty and SetProperty on Logger #3298

Merged
merged 3 commits into from Apr 16, 2019

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Apr 10, 2019

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 Reviewable

@codecov-io
Copy link

codecov-io commented Apr 10, 2019

Codecov Report

Merging #3298 into dev will increase coverage by <1%.
The diff coverage is 85%.

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

@304NotModified
Copy link
Member

Thanks for the PR,

but i'm afraid it won't change the NLog API viewpoint, mentioned in #2496 (comment)?

@snakefoot
Copy link
Contributor Author

snakefoot commented Apr 14, 2019 via email

@304NotModified
Copy link
Member

If we also add SetProperty, next to WithProperty, do you think that is OK? (from viewpoint API design)

@snakefoot
Copy link
Contributor Author

snakefoot commented Apr 14, 2019

@304NotModified Added a SetProperty method for updating the current Logger object-instance.

@304NotModified
Copy link
Member

Cool!

@304NotModified
Copy link
Member

Need to check it in depth, but I think I will merge it soon :)

@304NotModified 304NotModified self-assigned this Apr 14, 2019
@304NotModified 304NotModified self-requested a review April 14, 2019 18:06
@304NotModified 304NotModified changed the title Logger WithProperty Addede WithProperty and SetProperty on Logger Apr 14, 2019
src/NLog/Logger.cs Outdated Show resolved Hide resolved
src/NLog/Logger.cs Outdated Show resolved Hide resolved
src/NLog/Logger.cs Outdated Show resolved Hide resolved
src/NLog/Logger.cs Show resolved Hide resolved
src/NLog/LoggerImpl.cs Show resolved Hide resolved
@304NotModified 304NotModified changed the title Addede WithProperty and SetProperty on Logger Added WithProperty and SetProperty on Logger Apr 14, 2019
@304NotModified 304NotModified added this to the 4.6.3 milestone Apr 14, 2019
Copy link
Member

@304NotModified 304NotModified left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Is this safe for 4.6.3?

/// <summary>
/// Writes the specified diagnostic message.
/// </summary>
/// <param name="logEvent">Log event.</param>
public void Log(LogEventInfo logEvent)
{
if (IsEnabled(logEvent.Level))
var targetsForLevel = IsEnabled(logEvent.Level) ? GetTargetsForLevel(logEvent.Level) : null;
if (targetsForLevel != null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a performance tweak?

src/NLog/Logger.cs Show resolved Hide resolved
@304NotModified 304NotModified removed their assignment Apr 16, 2019
@snakefoot
Copy link
Contributor Author

snakefoot commented Apr 16, 2019 via email

@304NotModified
Copy link
Member

304NotModified commented Apr 30, 2019

@304NotModified 304NotModified added the documentation done all docs done (wiki, api docs, lists on nlog-project.org, xmldocs) label Apr 30, 2019
@snakefoot
Copy link
Contributor Author

snakefoot commented Apr 30, 2019

Updated the docs a little so the difference between WithProperty and SetProperty is clearer. Making the trap-door hidden in SetProperty a little more visible.

@304NotModified
Copy link
Member

304NotModified commented Apr 30, 2019

It's not a trap but a feature :)

@304NotModified
Copy link
Member

Updated it with a 2nd log. The set is really useful is some cases (e.g when you get more context along the way)

@snakefoot
Copy link
Contributor Author

snakefoot commented Apr 30, 2019

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)

@304NotModified
Copy link
Member

It's not suddenly? It's by (logger) name.

@304NotModified
Copy link
Member

Buy anyway, I'm curious when we will implement WithProperties and ClearProperty :p

(Maybe nice with default interfaces - c# 8)

@snakefoot
Copy link
Contributor Author

snakefoot commented Apr 30, 2019

It's not suddenly? It's by (logger) name.

Not sure I understand. There is no guarantee that two isolated locations doesn't call GetLogger() with the same name. When one of the isolated locations calls SetProperty then it will affect the other.

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 GetCurrentClassLogger). When one thread starts to call SetProperty then it will affect the Logger on the other controller-instance.

That is why I prefer WithProperty because it doesn't give you surprises.

@snakefoot snakefoot deleted the LoggerWithPropertyVer2 branch April 4, 2020 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation done all docs done (wiki, api docs, lists on nlog-project.org, xmldocs) feature needs documentation on wiki
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: define/set variable inside a logger
3 participants