This repository was archived by the owner on Oct 12, 2022. It is now read-only.
NLog GlobalDiagnosticContext should be explictly added using ContextProperties#183
Merged
SergeyKanzhelev merged 1 commit intomicrosoft:developfrom Apr 30, 2018
Merged
Conversation
1a8ca04 to
6bff99f
Compare
Contributor
Author
|
One might consider removing this code: 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 |
Contributor
|
@snakefoot thanks for another good contribution! @jeff-zohrab are you around to provide feedback? |
SergeyKanzhelev
approved these changes
Apr 30, 2018
Contributor
SergeyKanzhelev
left a comment
There was a problem hiding this comment.
LGTM. Can you please update the branch? Also I'll wait a day to give chance for additional feedback
…ictly added using ContextProperties
Contributor
Author
|
@SergeyKanzhelev Have updated the PR-branch from origin |
Contributor
|
@jeff-zohrab thank you for review! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
One can also add other random stuff like threadid, trace-correlationid, etc.