Log4JXmlEventLayout using Nlog XmlLayout#5774
Conversation
Mapped the Chainsaw to log4j layout Removed depenecy of log4j paramter class from System.xml Removed depenecy of log4j layoutclass from System.xml Fixed unit test for log4j paramter (had extra space). Added Cdata property in XmlElement class. Temp Unit test log4jxmllayout tests change
Added Invalid Xml Character Remover Abstarcted logic of inner renderer into xml helper methods. Added tests for cdata Marked Previous test case for bad xml as obsolete Added new unit test for invalid xml values.
Updated Log4JXmlEventLayout to improve XML formatting and rendering. Key changes include: - Added `ThrowableEncode` property for XML escaping control. - Simplified `ScopeNestedSeparator` handling. - Renamed `WrapInCData` to `CDataEncode`. - Removed deprecated methods from `XmlHelper`. - Updated test suite with new tests and removed obsolete ones.
[WIP] Log4JXmlEventLayout using XmlLayout
|
Thanks for opening this pull request! |
WalkthroughThis pull request refactors the XML layout rendering for Log4j events by removing the dedicated Changes
Sequence Diagram(s)sequenceDiagram
participant Logger
participant ChainsawTarget
participant Layout as Log4JXmlEventLayout
participant InnerXml as XmlLayout
participant Encoder as XmlEncodeLayoutRendererWrapper
participant Helper as XmlHelper
Logger->>ChainsawTarget: Send log event
ChainsawTarget->>Layout: Process event
Layout->>InnerXml: Build XML structure
InnerXml->>Encoder: Render XML elements
Encoder->>Helper: Check config (RemoveInvalidXmlIfNeeded / EscapeCDataIfNeeded)
Helper-->>Encoder: Return sanitized XML
Encoder-->>InnerXml: Return final XML output
InnerXml-->>Layout: Provide rendered XML
Layout-->>ChainsawTarget: Return formatted log output
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/NLog/Layouts/XML/XmlElement.cs (1)
109-116: Consider renaming property for consistency.
This property is namedRemoveInvalidXmlChar(singular), while related methods are plural (RemoveInvalidXmlChars). Renaming it toRemoveInvalidXmlCharscould reduce confusion.Proposed diff:
- public bool RemoveInvalidXmlChar + public bool RemoveInvalidXmlCharssrc/NLog/LayoutRenderers/Wrappers/XmlEncodeLayoutRendererWrapper.cs (1)
65-69: Fix the minor typo in the XML-doc comment.The summary line says "Remove Imvalid Xml Characters" instead of "Remove Invalid Xml Characters."
-/// Remove Imvalid Xml Characters from inner +/// Remove Invalid Xml Characters from inner
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/NLog.Targets.Network/Layouts/Log4JXmlEventLayout.cs(3 hunks)src/NLog.Targets.Network/Layouts/Log4JXmlEventLayoutRenderer.cs(0 hunks)src/NLog.Targets.Network/Layouts/Log4JXmlEventParameter.cs(1 hunks)src/NLog.Targets.Network/Targets/ChainsawTarget.cs(11 hunks)src/NLog/Config/ConfigurationItemFactory.cs(0 hunks)src/NLog/Internal/XmlHelper.cs(1 hunks)src/NLog/LayoutRenderers/Wrappers/XmlEncodeLayoutRendererWrapper.cs(3 hunks)src/NLog/Layouts/XML/XmlElement.cs(1 hunks)tests/NLog.Targets.Network.Tests/Log4JXmlTests.cs(7 hunks)
💤 Files with no reviewable changes (2)
- src/NLog.Targets.Network/Layouts/Log4JXmlEventLayoutRenderer.cs
- src/NLog/Config/ConfigurationItemFactory.cs
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/NLog.Targets.Network.Tests/Log4JXmlTests.cs (2)
src/NLog/ScopeContext.cs (1)
ScopeContext(52-887)src/NLog/Internal/ScopeContextAsyncState.cs (1)
PushNestedState(250-263)
src/NLog.Targets.Network/Layouts/Log4JXmlEventLayout.cs (1)
src/NLog.Targets.Network/Layouts/Log4JXmlEventParameter.cs (1)
Log4JXmlEventParameter(48-50)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: WIP
- GitHub Check: WIP
🔇 Additional comments (22)
src/NLog.Targets.Network/Layouts/Log4JXmlEventParameter.cs (1)
57-57: Validate intentional removal of XML sanitization.
Previously, assigningNamesanitized invalid XML characters. Now, invalid characters can slip through, potentially causing malformed XML. Please confirm if you intend to remove sanitization or reintroduce it for safety.If reintroducing is desired, consider this quick fix:
- public string Name { get => _name; set => _name = value; } + public string Name { get => _name; set => _name = XmlHelper.RemoveInvalidXmlChars(value); }src/NLog/Layouts/XML/XmlElement.cs (1)
99-107: Prevent potential conflicts between Encode and CDataEncode.
When bothEncodeandCDataEncodeare enabled, it might lead to double-escape or unexpected output. Please verify whether these properties should be mutually exclusive or if the current logic handles both safely.src/NLog/Internal/XmlHelper.cs (1)
449-454: Check for valid orgLength before trimming and appending.
IforgLengthis out of range,ToString(orgLength, ...)may throw an exception. Consider adding boundary checks to prevent potential errors.src/NLog/LayoutRenderers/Wrappers/XmlEncodeLayoutRendererWrapper.cs (3)
60-64: Add a new property for CDATA encoding.Introducing the
CDataEncodeproperty helps simplify toggling between standard XML encoding and CDATA wrapping.
81-91: Logical ordering of removal and CDATA wrapping.Removing invalid XML characters first and then optionally wrapping the result in CDATA looks functionally consistent.
92-95: Skipping XML-escaping when using CDATA.Using an
else ifensures XML-escaping is bypassed when CDATA is active, which aligns with the expected configuration flow.tests/NLog.Targets.Network.Tests/Log4JXmlTests.cs (12)
66-82: Configuration block for the Log4JXmlEventLayout.Defining attributes like
includeCallSite="true"andincludeEventProperties="true"in the test config matches the intended testing scenario.
86-91: Verifying ScopeContext usage.Clearing the context and pushing test properties/states validates that they are not included when
includeScopeNested="false".
97-97: Chained exception instantiation.Creating an inner exception ("Goodbye Exception") ensures deeper stack traces can be tested.
174-174: Validating primary and inner exception text.Checks that both "Hello Exception" and "Goodbye Exception" appear in the final XML snippet.
184-185: AppInfo substring extraction.Extracting the dynamic portion ensures consistent checking of the domain name and process ID.
241-241: Setting custom AppInfo.Assigning
"MyApp"ensures user-defined application info is reflected in the final log4j XML.
254-254: Double braces in escaped message.The test verifies message layout with
<,>symbols escaped and double braces for placeholders. Confirm that this meets your formatting expectations.
258-309: Test for CDATA-wrapped exception.
WriteThrowableCData = trueensures exceptions appear inside a<![CDATA[...]]>block, preventing XML-escape quirks.
314-333: Test for proper XML-escaping when CDATA is off.Checks to ensure
<,>characters in the exception are encoded in standard XML form.
335-336: Skipping obsolete test.Marking the old renderer-based test with
[Obsolete]and skipping is appropriate given the refactored XML logic.
405-453: Verifying removal of invalid XML characters.Covers edge cases by enumerating a wide range of characters and ensuring they are sanitized.
455-477: Test for raw throwable output.Turning off both
WriteThrowableCDataandThrowableEncodeensures raw unescaped content is produced.src/NLog.Targets.Network/Targets/ChainsawTarget.cs (1)
96-240: Refactor to delegate property access to_log4JLayout.These pass-through properties (e.g.,
IncludeNdc,IncludeMdc,NdcItemSeparator) directly map to_log4JLayoutfields, which is consistent with the new layout-based architecture.src/NLog.Targets.Network/Layouts/Log4JXmlEventLayout.cs (3)
55-147: Switching to a dedicatedInnerXmllayout.
InitializeLayout()populatesInnerXmlwith attributes and elements, eliminating the old separate renderer. This design fosters clearer, more maintainable XML generation.
153-306: Parameter handling and additional flags.Collecting custom parameters in
_parametersand adding boolean flags (e.g.,IncludeScopeNested,WriteThrowableCData) centralizes the log4j XML configuration.
312-312: InnerXml-based rendering.
RenderFormattedMessagenow callsInnerXml.Render, removing the duplication from a separate renderer class.
snakefoot
added
breaking change
src/NLog/LayoutRenderers/Wrappers/XmlEncodeLayoutRendererWrapper.cs
Outdated
Show resolved
Hide resolved
src/NLog/LayoutRenderers/Wrappers/XmlEncodeLayoutRendererWrapper.cs
Outdated
Show resolved
Hide resolved
src/NLog/LayoutRenderers/Wrappers/XmlEncodeLayoutRendererWrapper.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/NLog.Targets.Network/Layouts/Log4JXmlEventLayout.cs (2)
56-57: PreferDateTimeKind.Utcfor the epoch baseline
log4jDateBaseis created with an unspecifiedKind. When later subtractingevt.TimeStamp.ToUniversalTime()the framework silently ignores the mismatch, but the intent is clearly “milliseconds since the Unix epoch (UTC)”. Declaring the baseline explicitly as UTC removes any ambiguity and makes static analysis tools happier.-private static readonly DateTime log4jDateBase = new DateTime(1970, 1, 1); +private static readonly DateTime log4jDateBase = new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc);
80-82:WriteThrowableCDatachanges after initialization are ignored
CDataEncodeis assigned only once duringInitializeLayout.
If the user togglesWriteThrowableCDataat runtime (for example after loading an XML config and then adjusting programmatically), the layout will continue to use the old value becauseInitializeLayout()is not re-executed.Consider either:
- Making
WriteThrowableCDataareadonly/init-only property, or- Re-evaluating the flag inside a small wrapper layout, e.g.
-InnerXml.Elements.Add(new XmlElement("log4j:throwable", "${exception:format=ToString}") -{ - CDataEncode = WriteThrowableCData, -}); +InnerXml.Elements.Add( + new XmlElement("log4j:throwable", "${exception:format=ToString}") + { + CDataEncode = Layout.FromMethod(_ => WriteThrowableCData.ToString(), LayoutRenderOptions.ThreadAgnostic) + });so the value is checked for every event.
(Any other dynamic mechanism provided byXmlElementwould work as well.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/NLog.Targets.Network/Layouts/Log4JXmlEventLayout.cs(3 hunks)src/NLog/Internal/XmlHelper.cs(1 hunks)src/NLog/LayoutRenderers/Wrappers/XmlEncodeLayoutRendererWrapper.cs(2 hunks)src/NLog/Layouts/XML/XmlElement.cs(1 hunks)tests/NLog.Targets.Network.Tests/Log4JXmlTests.cs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/NLog/Layouts/XML/XmlElement.cs
- src/NLog/LayoutRenderers/Wrappers/XmlEncodeLayoutRendererWrapper.cs
- tests/NLog.Targets.Network.Tests/Log4JXmlTests.cs
- src/NLog/Internal/XmlHelper.cs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: WIP
- GitHub Check: WIP
- GitHub Check: WIP
src/NLog/LayoutRenderers/Wrappers/XmlEncodeLayoutRendererWrapper.cs
Outdated
Show resolved
Hide resolved
|
Think the pull-request is super close to be ready for merge. Thank you for being patient with all my comments. I hope my last suggestions are acceptable. |
No worries at all! Thanks a ton for all the guidance — your feedback really helped me improve the code a lot. Appreciate you being patient too! |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/NLog.Targets.Network/Layouts/Log4JXmlEventLayout.cs (1)
120-129:parameter.Layoutmay be null – guard to preventNullReferenceExceptionThis exact issue was raised in an earlier review and still applies: if a user mis-configures a
<parameter>element without alayout,new XmlAttribute("value", parameter.Layout)will throw.Consider failing fast with a clear configuration message or skipping the element when the layout is missing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/NLog.Targets.Network/Layouts/Log4JXmlEventLayout.cs(3 hunks)src/NLog/Internal/XmlHelper.cs(1 hunks)src/NLog/LayoutRenderers/Wrappers/XmlEncodeLayoutRendererWrapper.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/NLog/LayoutRenderers/Wrappers/XmlEncodeLayoutRendererWrapper.cs
- src/NLog/Internal/XmlHelper.cs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: WIP
- GitHub Check: WIP
- GitHub Check: WIP
| public class Log4JXmlEventLayout : Layout | ||
| { | ||
|
|
||
| private static readonly DateTime log4jDateBase = new DateTime(1970, 1, 1); |
There was a problem hiding this comment.
Specify DateTimeKind.Utc for the epoch constant to avoid time-kind mismatch
log4jDateBase is created with DateTimeKind.Unspecified, yet it’s subtracted from evt.TimeStamp.ToUniversalTime() (kind = UTC). Mixing kinds works but triggers an implicit conversion that can bite (e.g., DateTime assertions using Kind, daylight-saving corrections in older frameworks, or analyzers warning about mismatched kinds).
- private static readonly DateTime log4jDateBase = new DateTime(1970, 1, 1);
+ private static readonly DateTime log4jDateBase = new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc);A one-liner fix keeps the arithmetic purely UTC and future-proofs the calculation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static readonly DateTime log4jDateBase = new DateTime(1970, 1, 1); | |
| private static readonly DateTime log4jDateBase = new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc); |
| var appProperty = new XmlElement("log4j:data", null); | ||
| appProperty.Attributes.Add(new XmlAttribute("name", "log4japp")); | ||
| appProperty.Attributes.Add(new XmlAttribute("value", AppInfo)); | ||
| dataProperties.Elements.Add(appProperty); | ||
|
|
||
| var machineProperty = new XmlElement("log4j:data", null); | ||
| machineProperty.Attributes.Add(new XmlAttribute("name", "log4jmachinename")); | ||
| machineProperty.Attributes.Add(new XmlAttribute("value", "${machinename}")); | ||
| dataProperties.Elements.Add(machineProperty); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle optional AppInfo to avoid crashes and honour defaults
AppInfo has no default value and can be left null. Adding it unconditionally causes a runtime exception inside XmlAttribute.
- var appProperty = new XmlElement("log4j:data", null);
- appProperty.Attributes.Add(new XmlAttribute("name", "log4japp"));
- appProperty.Attributes.Add(new XmlAttribute("value", AppInfo));
- dataProperties.Elements.Add(appProperty);
+ if (AppInfo is null)
+ {
+ // Fallback to a sensible default (old behaviour was `${appdomain:friendlyname}`)
+ AppInfo = "${appdomain:friendlyname}";
+ }
+
+ var appProperty = new XmlElement("log4j:data", null);
+ appProperty.Attributes.Add(new XmlAttribute("name", "log4japp"));
+ appProperty.Attributes.Add(new XmlAttribute("value", AppInfo));
+ dataProperties.Elements.Add(appProperty);This keeps the layout resilient and preserves backward compatibility.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var appProperty = new XmlElement("log4j:data", null); | |
| appProperty.Attributes.Add(new XmlAttribute("name", "log4japp")); | |
| appProperty.Attributes.Add(new XmlAttribute("value", AppInfo)); | |
| dataProperties.Elements.Add(appProperty); | |
| var machineProperty = new XmlElement("log4j:data", null); | |
| machineProperty.Attributes.Add(new XmlAttribute("name", "log4jmachinename")); | |
| machineProperty.Attributes.Add(new XmlAttribute("value", "${machinename}")); | |
| dataProperties.Elements.Add(machineProperty); | |
| // Ensure AppInfo is never null (fallback to old default if unset) | |
| if (AppInfo is null) | |
| { | |
| // Fallback to a sensible default (old behaviour was `${appdomain:friendlyname}`) | |
| AppInfo = "${appdomain:friendlyname}"; | |
| } | |
| var appProperty = new XmlElement("log4j:data", null); | |
| appProperty.Attributes.Add(new XmlAttribute("name", "log4japp")); | |
| appProperty.Attributes.Add(new XmlAttribute("value", AppInfo)); | |
| dataProperties.Elements.Add(appProperty); | |
| var machineProperty = new XmlElement("log4j:data", null); | |
| machineProperty.Attributes.Add(new XmlAttribute("name", "log4jmachinename")); | |
| machineProperty.Attributes.Add(new XmlAttribute("value", "${machinename}")); | |
| dataProperties.Elements.Add(machineProperty); |
|
I think the code looks great, and super happy that the NLog XmlLayout was able to lift the task. And thanks for adding the new feature Do you have any more changes planned? Or should I just merge when the build-servers approves? (Then I can fiddle with getting the separator-option working again) |
|
Yup, nothing more from my side! Merge away once the build is happy. |
|
Closing and opening pull-request to tickle the build-servers |
|
ana1250
changed the title
|
Hooray your first pull request is merged! Thanks for the contribution! Looking for more? 👼 Please check the up-for-grabs issues - thanks! |
2 participants
This PR replaces the usage of System.Xml with NLog’s native XML layout infrastructure (XmlLayout, XmlElement, etc.) to construct Log4j-compatible XML logs. #5747
System.Xml Removal: Removed System.Xml dependencies from both the Log4JXmlLayout and Log4JXmlEventLayoutParameter classes.
Renderer Updates:
New Features:
Tests:
TODO:
Fix Scopenested rendering
Remove XML helper from Network.Target and dependent obsolete unit test