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

NdlcLayoutRenderer - Nested Diagnostics Logical Context #2110

Merged
merged 1 commit into from
May 17, 2017

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented May 16, 2017

Make it easier to support Microsoft ILogger BeginScope (And maybe resolve #1615 and #1181)

Maybe rename to Nested Diagnostics Logical Context (NDLC) ?

@codecov
Copy link

codecov bot commented May 16, 2017

Codecov Report

Merging #2110 into master will increase coverage by <1%.
The diff coverage is 95%.

@@           Coverage Diff           @@
##           master   #2110    +/-   ##
=======================================
+ Coverage      81%     81%   +<1%     
=======================================
  Files         289     291     +2     
  Lines       19957   20069   +112     
  Branches     2360    2376    +16     
=======================================
+ Hits        16176   16323   +147     
+ Misses       3175    3140    -35     
  Partials      606     606

@304NotModified
Copy link
Member

304NotModified commented May 16, 2017

Cool! Great!

I propose:

  • the class name for the context: NestedDiagnosticsLogicalContext (consistent with NestedDiagnosticsContext , MappedDiagnosticsContext and MappedDiagnosticsLogicalContext)

  • the class name and renderer name for the layout renderer: NdlcLayoutRender ``${NDLC} (consistent with NdcLayoutRenderer/${NDC}, MDC, MDLC etc)

@snakefoot snakefoot changed the title NlcLayoutRenderer - Nested Logical Context NdlcLayoutRenderer - Nested Logical Context May 16, 2017
@snakefoot snakefoot changed the title NdlcLayoutRenderer - Nested Logical Context NdlcLayoutRenderer - Nested Diagnostics Logical Context May 16, 2017
@snakefoot
Copy link
Contributor Author

I propose:

@304NotModified Accepted

@304NotModified 304NotModified merged commit 36854b0 into NLog:master May 17, 2017
@304NotModified
Copy link
Member

thanks!

/// <returns>The top message which is no longer on the stack.</returns>
public static object Pop()
{
var current = GetThreadLocal();
Copy link
Member

Choose a reason for hiding this comment

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

@snakefoot

see it now in the code. It this really a Pop (should remove it from the stack)? looks like a Top method

Copy link
Contributor Author

@snakefoot snakefoot May 18, 2017

Choose a reason for hiding this comment

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

Sorry not sure I understand the Pop-method modifies the stack by removing the top-element and returns it.

@304NotModified 304NotModified mentioned this pull request May 25, 2017
3 tasks
@304NotModified
Copy link
Member

@snakefoot could you please help me by adding some docs to the wiki? Thanks!

@snakefoot
Copy link
Contributor Author

@304NotModified
Copy link
Member

thanks!!

@furier
Copy link

furier commented Apr 6, 2018

Could the documentation possibly be included to explain what is the purpose of NDLC and NDC and maybe how it differs from MDLC and MDC? With NLog 4.5 structured logging MDLC seems to add metadata properties that are part of the structured data being logged but not part of the message rendered, correct? What does NDLC do then? append the pushed value to the rendered message or prepended, does it also support structured data, etc...?

@304NotModified
Copy link
Member

@furier please create a new github issue. Closed issues are difficult to track. Thanks!

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

3 participants