Skip to content

Conversation

@snakefoot
Copy link
Contributor

No description provided.

@snakefoot snakefoot added this to the 6.0.7 milestone Dec 7, 2025
@coderabbitai
Copy link

coderabbitai bot commented Dec 7, 2025

Walkthrough

Added the AppDomainFixedOutput attribute to WhenEmptyLayoutRendererWrapper and updated a unit test to exclude wrapper base types from an attribute-enforcement check.

Changes

Cohort / File(s) Change Summary
Attribute addition
src/NLog/LayoutRenderers/Wrappers/WhenEmptyLayoutRendererWrapper.cs
Added the [AppDomainFixedOutput] attribute to the public class WhenEmptyLayoutRendererWrapper.
Test adjustment
tests/NLog.UnitTests/ApiTests.cs
Modified IStringValueRenderer_AppDomainFixedOutput_Attribute_NotRequired to exclude types assignable from NLog.LayoutRenderers.Wrappers.WrapperLayoutRendererBase when asserting IStringValueRenderer implementations.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Pay attention to the attribute placement and any reflection- or discovery-based logic that relies on AppDomainFixedOutput.
  • Verify the updated test exclusion correctly targets wrapper base types and doesn't mask other missing attributes.

Poem

🐰 I hopped through code with ears on alert,
A tiny tag placed where wrappers insert,
Tests learned to skip the wrapper's embrace,
One gentle change — a neat, tidy trace,
I nibble a carrot and bound from the place. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check ❓ Inconclusive No pull request description was provided by the author. While a description would be helpful, the absence of one alone does not constitute a violation since the title and code changes are self-explanatory. Consider adding a brief description explaining what the AppDomainFixedOutput attribute does and why it benefits the WhenEmpty wrapper to improve reviewer context.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title 'WhenEmpty - Support AppDomainFixedOutput optimization' clearly and specifically describes the main change: adding AppDomainFixedOutput support to the WhenEmptyLayoutRendererWrapper class for performance optimization.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8edcd10 and ec980bb.

📒 Files selected for processing (2)
  • src/NLog/LayoutRenderers/Wrappers/WhenEmptyLayoutRendererWrapper.cs (1 hunks)
  • tests/NLog.UnitTests/ApiTests.cs (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/NLog.UnitTests/ApiTests.cs (1)
src/NLog/LayoutRenderers/Wrappers/WrapperLayoutRendererBase.cs (1)
  • WrapperLayoutRendererBase (49-122)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: task-list-completed
🔇 Additional comments (2)
tests/NLog.UnitTests/ApiTests.cs (1)

193-193: Test exclusion aligns with wrapper optimization.

The exclusion of WrapperLayoutRendererBase-derived types is appropriate and consistent with the addition of [AppDomainFixedOutput] to WhenEmptyLayoutRendererWrapper. Wrapper types can legitimately implement IStringValueRenderer for optimization while also being marked as AppDomainFixedOutput when their dependencies are fixed.

src/NLog/LayoutRenderers/Wrappers/WhenEmptyLayoutRendererWrapper.cs (1)

47-47: Clarify whether NLog validates AppDomainFixedOutput dependencies in wrapper classes.

WhenEmptyLayoutRendererWrapper conditionally outputs either Inner or WhenEmpty based on runtime evaluation (line 85-93). The AppDomainFixedOutput attribute is only semantically correct if NLog's infrastructure validates that both the Inner and WhenEmpty layout renderers are themselves marked AppDomainFixedOutput before applying any caching or optimizations.

Without this validation, incorrect output caching could occur if the dependencies do not provide fixed output. Confirm that this validation is in place, or add explicit runtime checks to ensure correctness.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c7abc83 and 8edcd10.

📒 Files selected for processing (1)
  • src/NLog/LayoutRenderers/Wrappers/WhenEmptyLayoutRendererWrapper.cs (1 hunks)

/// </summary>
[LayoutRenderer("whenEmpty")]
[AmbientProperty(nameof(WhenEmpty))]
[AppDomainFixedOutput]
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find AppDomainFixedOutput attribute definition
echo "=== AppDomainFixedOutput definition ==="
fd -e cs . src/ --exec rg -l "AppDomainFixedOutput" {} \; | head -20

echo -e "\n=== AppDomainFixedOutput attribute definition ==="
rg -A10 "class AppDomainFixedOutputAttribute" --type=cs

Repository: NLog/NLog

Length of output: 162


🏁 Script executed:

#!/bin/bash
# Check all wrapper classes and their attributes
echo "=== Wrapper classes with attributes ==="
cd src/NLog/LayoutRenderers/Wrappers
for file in *.cs; do
  echo "=== $file ==="
  head -60 "$file" | rg -B2 "^\s*\[.*\]|class.*Wrapper"
done

Repository: NLog/NLog

Length of output: 162


🏁 Script executed:

#!/bin/bash
# Check WhenEmptyLayoutRendererWrapper in detail
echo "=== WhenEmptyLayoutRendererWrapper class structure ==="
cat -n src/NLog/LayoutRenderers/Wrappers/WhenEmptyLayoutRendererWrapper.cs | head -130

Repository: NLog/NLog

Length of output: 162


🌐 Web query:

NLog AppDomainFixedOutput attribute semantics layout renderer

💡 Result:

Short answer — AppDomainFixedOutputAttribute marks a Layout or LayoutRenderer whose rendered value is constant for the lifetime of the current AppDomain (so it never changes between log events). NLog can then evaluate/cache that value once (at init/config time or when capturing state) instead of rendering it per-log-event on the logging thread. Without this attribute NLog will render the layout on the main/logging thread every time. [1]

Notes:

  • It’s different from ThreadAgnostic/ThreadAgnosticImmutable (which say the renderer is safe to run on any thread); AppDomainFixedOutput specifically signals the value is fixed for the AppDomain lifetime and may be precomputed/cached. [1]

Source: NLog API docs (AppDomainFixedOutputAttribute description). [1]

References
[1] NLog API documentation — NLog.Config namespace (AppDomainFixedOutputAttribute).


🌐 Web query:

NLog WhenEmptyLayoutRendererWrapper optimization performance

💡 Result:

Short answer: NLog already added a specific optimization for WhenEmptyLayoutRendererWrapper (IStringValueRenderer logic) in recent releases — upgrade to a current NLog 5.x/5.5+ to get that improvement. Also follow general layout-renderer performance guidance (mark thread-agnostic where possible, avoid heavy renderers like callsite/stacktrace, and use Async/Buffering wrappers). [1][2][3]

Sources

  • NLog 5.5.0 release notes (calls out "WhenEmptyLayoutRendererWrapper - Optimize IStringValueRenderer Logic"). [1]
  • NLog docs: WhenEmptyLayoutRendererWrapper reference. [2]
  • NLog performance guidance (layout-renderer optimization, ThreadAgnostic notes). [3]

🌐 Web query:

NLog layout renderer wrapper AppDomainFixedOutput conditional output

💡 Result:

You can conditionally render the AppDomain (or any layout) using NLog's wrapper layout renderers (e.g. ${when}, ${onexception}, ${whenEmpty}). The AppDomain renderer is ${appdomain}. Example — render short AppDomain name only when logger == "MyLogger":

${when:when=logger=='MyLogger':inner=${appdomain:format=Short}}

Notes:

  • Use ${onexception:inner=...} to output only on exceptions.
  • Use ${whenEmpty:inner=...:else=...} to provide fallback text when inner is empty.
    Docs: NLog wrapper renderers list, AppDomain renderer, and When wrapper. [1][2][3]

Sources
[1] NLog.LayoutRenderers.Wrappers (wrapper list).
[2] AppDomainLayoutRenderer documentation.
[3] WhenLayoutRendererWrapper (conditional wrapper).


Remove AppDomainFixedOutput attribute from WhenEmptyLayoutRendererWrapper.

The AppDomainFixedOutput attribute indicates that a layout's rendered value is constant for the AppDomain lifetime and can be cached. However, WhenEmptyLayoutRendererWrapper conditionally outputs either the Inner or WhenEmpty layout based on runtime evaluation of each LogEventInfo. The choice between these two layouts is determined per log event (whether Inner produces empty output), making the output not fixed within the AppDomain. This attribute is semantically incorrect for this wrapper and may cause NLog to incorrectly cache or optimize the output, leading to wrong results.

🤖 Prompt for AI Agents
In src/NLog/LayoutRenderers/Wrappers/WhenEmptyLayoutRendererWrapper.cs around
line 47, remove the AppDomainFixedOutput attribute because the wrapper's output
depends on each LogEventInfo (it conditionally renders Inner or WhenEmpty per
event) and therefore is not constant for the AppDomain; edit the file to delete
the [AppDomainFixedOutput] annotation so the wrapper won't be treated as
cacheable across the AppDomain and will be evaluated per log event.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 7, 2025

@snakefoot snakefoot merged commit 155b158 into NLog:dev Dec 7, 2025
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant