Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

NLog GlobalDiagnosticContext should be explictly added using ContextProperties #183

Merged
merged 1 commit into from
Apr 30, 2018

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Apr 29, 2018

This is a refactoring of #152 by @jeff-zohrab

It is a bad idea to include all NLog GDC-items, as they are often used a global variables across all NLog-targets. This means all kind of random junk will suddenly be sent to the ApplicationInsightsTarget.

Instead this PR allows one to add the wanted global items explicitly using the new ContextProperties:

<nlog>
   <extensions>
      <add assembly="Microsoft.ApplicationInsights.NLogTarget" />
   </extensions>
   <targets>
      <target type="ApplicationInsightsTarget" name="aiTarget" />
           <contextProperty name="appid" layout="${gdc:item=appid}" />
           <contextProperty name="threadid" layout="${threadid}" />
      </target>
   </targets>
   <rules>
      <logger name="*" minlevel="Trace" writeTo="aiTarget" />
   </rules>
</nlog>

One can also add other random stuff like threadid, trace-correlationid, etc.

@snakefoot snakefoot changed the title GlobalDiagnosticContext must be explictly added using ContextProperties NLog GlobalDiagnosticContext must be explictly added using ContextProperties Apr 29, 2018
@snakefoot snakefoot force-pushed the develop branch 3 times, most recently from 1a8ca04 to 6bff99f Compare April 29, 2018 22:06
@snakefoot
Copy link
Contributor Author

snakefoot commented Apr 29, 2018

One might consider removing this code:

https://github.com/Microsoft/ApplicationInsights-dotnet-logging/blob/388ff8e884711ffdc6a7ef8cab16a0b136b6c2ac/src/NLogTarget/ApplicationInsightsTarget.cs#L75-L79

As it can be replaced by this config (Property names copied from log4net appender):

      <target type="ApplicationInsightsTarget" name="aiTarget" />
           <contextProperty name="ClassName" layout="${callsite:className=true:fileName=false:methodName=false}" />
           <contextProperty name="FileName" layout="${callsite:className=false:fileName=true:methodName=false}" />
           <contextProperty name="MethodName" layout="${callsite:className=false:fileName=false:methodName=true}" />
           <contextProperty name="LineNumber" layout="${callsite-linenumber}" />
      </target>

And it will actually ensure that NLog will capture a useful callsite (As UserStackFrame is not guranteed unless another target correctly requests callsite info).

@snakefoot snakefoot changed the title NLog GlobalDiagnosticContext must be explictly added using ContextProperties NLog GlobalDiagnosticContext should be explictly added using ContextProperties Apr 29, 2018
@SergeyKanzhelev
Copy link
Contributor

@snakefoot thanks for another good contribution!

@jeff-zohrab are you around to provide feedback?

Copy link
Contributor

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

LGTM. Can you please update the branch? Also I'll wait a day to give chance for additional feedback

@snakefoot
Copy link
Contributor Author

snakefoot commented Apr 30, 2018

@SergeyKanzhelev Have updated the PR-branch from origin

@SergeyKanzhelev
Copy link
Contributor

@jeff-zohrab thank you for review!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants