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

TargetWithContext - Easier to capture snapshot of MDLC and NDLC context #2467

Merged
merged 2 commits into from
Feb 12, 2018

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Dec 16, 2017

Attempt to resolve #2466 and #2465

Guess one could also extend it to enable capture of GDC and Callsite.

Maybe implement a helper method than returns all context properties (IncludeAllProperties, IncludeMdlc, IncludeMdc, IncludeGdc).

Suddenly also become a bugfix for correct precalculation of async data for custom NLog-Layouts. Before it would be impossible for custom NLog-Layouts to correctly capture the async-state. Resolves #254

And a bugfix that allows custom NLog-Layouts to work correctly, when they override Layout.Precalculate and are used with Target.OptimizeBufferReuse = true.

@codecov
Copy link

codecov bot commented Dec 17, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@29704d6). Click here to learn what that means.
The diff coverage is 56%.

@@           Coverage Diff            @@
##             master   #2467   +/-   ##
========================================
  Coverage          ?     81%           
========================================
  Files             ?     325           
  Lines             ?   23908           
  Branches          ?    3020           
========================================
  Hits              ?   19380           
  Misses            ?    3713           
  Partials          ?     815

@snakefoot snakefoot force-pushed the TargetWithContext branch 2 times, most recently from 5f87959 to af0aa3e Compare December 17, 2017 08:08
@snakefoot
Copy link
Contributor Author

snakefoot commented Dec 17, 2017

An alternative approach would be to implement a different async-target-wrapper using ThreadPool.QueueUserWorkItem (or Tasks). Then the async-state will automatically flow with the LogEventInfo (This will not help MDC + NDC but maybe they are also on their way out).

But such async-wrapper will not be able to perform batch-handling, so not that super interesting. And it would also require a new buffering-target-wrapper.

@snakefoot snakefoot force-pushed the TargetWithContext branch 10 times, most recently from 855dfc2 to 32d3576 Compare December 19, 2017 20:10
@snakefoot
Copy link
Contributor Author

snakefoot commented Dec 19, 2017

@304NotModified Implementing an easier approach for handling batching of log-events, became too messy.

But I like this direction for making it easier for custom targets to make use of MDLC and NDLC without forcing them to use JsonLayout.

@304NotModified
Copy link
Member

But I like this direction for making it easier for custom targets to make use of MDLC and NDLC without forcing them to use JsonLayout.

Indeed, that would be a nice improvement!

Will try to review the PR this weekend.

@snakefoot
Copy link
Contributor Author

Maybe consider to change the default value of the parameter IncludeMdlc = true.

@304NotModified 304NotModified added this to the 4.5 rc4 milestone Jan 3, 2018
@snakefoot snakefoot force-pushed the TargetWithContext branch 2 times, most recently from c90daff to 6086aa4 Compare January 4, 2018 18:55
@snakefoot snakefoot force-pushed the TargetWithContext branch 5 times, most recently from 84db6b3 to 7fefb7b Compare January 20, 2018 15:02
@snakefoot
Copy link
Contributor Author

snakefoot commented Jan 20, 2018

@304NotModified Now removed the automatic forwarding of properties to the inner-layout, think it was more confusing than helping.

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.

It code looks great!

I tried to implement my own target and tried to understand the properties/method. I proposed some small renames that make things easier (IMO).

src/NLog/Targets/TargetWithContextProperty.cs Outdated Show resolved Hide resolved
src/NLog/Targets/TargetWithContext.cs Outdated Show resolved Hide resolved
src/NLog/Targets/TargetWithContext.cs Outdated Show resolved Hide resolved
src/NLog/Targets/TargetWithContext.cs Outdated Show resolved Hide resolved
/// </summary>
/// <param name="logEvent"></param>
/// <returns>True if properties should be included</returns>
protected bool HasCombinedProperties(LogEventInfo logEvent)
Copy link
Member

Choose a reason for hiding this comment

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

proposal to rename (after renames below) -> ShouldIncludeProperties

I prefer removing the term "combined properties", if possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@304NotModified
Copy link
Member

304NotModified commented Feb 6, 2018

oops sorry for breaking the build

edit: can't see what's wrong. Works locally

@snakefoot
Copy link
Contributor Author

edit: can't see what's wrong. Works locally

There is a conflict with my optimizations of the NetworkTarget and this PR. I have now merged with master, and resolved the conflict.

@304NotModified
Copy link
Member

merged :)

@snakefoot snakefoot mentioned this pull request Feb 21, 2018
@snakefoot snakefoot deleted the TargetWithContext branch April 4, 2020 17:38
@snakefoot snakefoot modified the milestones: 4.5 rc5, 4.5 Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants