Log4JXmlEventLayout - Fixed bug with empty nlog:properties. Log4JXmlEventLayoutRenderer : added IncludeAllProperties option. Log4JXmlEventLayout: added IncludeAllProperties, IncludeMdlc and IncludeMdc option #2090
Conversation
snakefoot
force-pushed
the
Log4JXmlEventLayoutNLogProperties
branch
2 times, most recently
from
bcf200b
to
99b2137
Compare
Codecov Report
@@ Coverage Diff @@
## master #2090 +/- ##
=======================================
- Coverage 81% 81% -<1%
=======================================
Files 289 289
Lines 19924 19946 +22
Branches 2356 2360 +4
=======================================
+ Hits 16210 16217 +7
- Misses 3115 3124 +9
- Partials 599 605 +6 |
snakefoot
force-pushed
the
Log4JXmlEventLayoutNLogProperties
branch
4 times, most recently
from
39d9dc2
to
15ec055
Compare
|
changes without newlines: |
| /// Gets or sets a value indicating whether to include contents of the <see cref="MappedDiagnosticsContext"/> dictionary. | ||
| /// </summary> | ||
| /// <docgen category='Payload Options' order='10' /> | ||
| public bool IncludeMdc { get { return Renderer.IncludeMdc; } set { Renderer.IncludeMdc = value; } } |
There was a problem hiding this comment.
maybe we should add an interface that dictates the properties shared by Log4JXmlEventLayout.cs and Log4JXmlEventLayoutRenderer.cs ?
Then it's more clear those are coupled. What do you think?
There was a problem hiding this comment.
No idea. Just thought it was weird that the Layout didn't have access to all the fancy options on the layout-renderer.
There was a problem hiding this comment.
OK, added for the feature #2091
| continue; | ||
|
|
||
| xtw.WriteStartElement("log4j", "data", dummyNamespace); | ||
| xtw.WriteAttributeSafeString("name", propertyKey); |
There was a problem hiding this comment.
in the feature we should unduplicate this cdeo
There was a problem hiding this comment.
Removed the duplicate code.
| @@ -329,6 +332,28 @@ protected override void Append(StringBuilder builder, LogEventInfo logEvent) | |||
| } | |||
| #endif | |||
|
|
|||
| if (this.IncludeAllProperties) | |||
There was a problem hiding this comment.
this a new feature, isn't it?
src/NLog/MappedDiagnosticsContext.cs
Outdated
| private static IDictionary<string, object> ThreadDictionary | ||
| private static readonly IDictionary<string, object> EmptyDefaultDictionary = new SortHelpers.ReadOnlySingleBucketDictionary<string, object>(); | ||
|
|
||
| private static IDictionary<string, object> GetThreadDictionary(bool create = true) |
There was a problem hiding this comment.
could you please document when we should set create to false?
There was a problem hiding this comment.
Updated XML-documentation
304NotModified
changed the title
snakefoot
force-pushed
the
Log4JXmlEventLayoutNLogProperties
branch
from
15ec055
to
2cf9922
Compare
snakefoot
force-pushed
the
Log4JXmlEventLayoutNLogProperties
branch
from
2cf9922
to
2f5e3cc
Compare
| @@ -156,6 +156,10 @@ public void Log4JXmlTest() | |||
| break; | |||
| #endif | |||
|
|
|||
| case "nlogPropertyKey": | |||
There was a problem hiding this comment.
isn't it strange we have now "nlogPropertyKey" in "log4j" and "nlog" (https://github.com/snakefoot/NLog/blob/2f5e3cced2fb1e56846e68e260c28c7d237bccfb/tests/NLog.UnitTests/LayoutRenderers/Log4JXmlTests.cs#L192)
There was a problem hiding this comment.
Not sure I understand. It is now also part of the standard log4j-properties because of IncludeAllProperties=true. The non-standard nlog-properties can only be handled by custom parsers. Not my idea to initially place the LogEvent-properties in a nlog-xml-tag not recognized by the log4j-standard.
|
Could you please update the docs? Thanks! |
304NotModified
changed the title
304NotModified
changed the title
304NotModified
changed the title
|
Updated the following wiki pages: https://github.com/NLog/NLog/wiki/Log4JXMLEvent-Layout-Renderer |
snakefoot
added
network/NLogViewer-target
log4jxml-layout
Fixes #2088