LoggerImpl - Merge WriteToTargetWithFilterChain into Write by snakefoot · Pull Request #6008 · NLog/NLog

snakefoot · 2025-09-27T19:54:57Z

No description provided.

coderabbitai · 2025-09-27T19:55:07Z

Walkthrough

Inlines per-target filter handling into LoggerImpl, centralizes call-site capture via a new CaptureCallSiteInfo, changes error handling for capture failures to warn, and removes call-site/stack-trace helper methods from TargetWithFilterChain.

Changes

Cohort / File(s)	Summary of Changes
Logger implementation update `src/NLog/LoggerImpl.cs`	Inlined per-target filter handling into the main write loop (removed `WriteToTargetWithFilterChain` usage). Now evaluates filters per target in-place, writes asynchronously on non-Ignore results, logs debug when filters reject, and stops on `LogFinal`/`IgnoreFinal`. Replaced prior caller-class/stack-trace branching with a new `CaptureCallSiteInfo(Type loggerType, TargetWithFilterChain targetsForLevel, LogEventInfo logEvent, LogFactory logFactory)` that attempts call-site optimization, then guarded stack-trace capture; capture failures now log via `InternalLogger.Warn` and rethrow only under DEBUG or when `ThrowExceptions` dictates. Added internal helpers `TryCallSiteClassNameOptimization` and `MustCaptureStackTrace` locally.
Filter-chain helper removal `src/NLog/Internal/TargetWithFilterChain.cs`	Removed two helper methods related to stack-trace/call-site optimization (`TryCallSiteClassNameOptimization` and `MustCaptureStackTrace`) and eliminated related logic from this file; no other control-flow changes in this file.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Logger
    participant Capture as CaptureCallSiteInfo
    participant Filters as PerTargetFilters
    participant Target

    Logger->>Capture: CaptureCallSiteInfo(loggerType, targetsForLevel, logEvent, logFactory)
    alt Call-site optimization succeeds
        Capture-->>Logger: set CallerClassName (no stack-trace)
    else Need stack-trace
        Capture->>Capture: try capture StackTrace (guarded)
        alt Capture success
            Capture-->>Logger: set CallerClassName
        else Capture failed
            Capture-->>Logger: InternalLogger.Warn(...)
            Note right of Logger: rethrow only if DEBUG or ThrowExceptions
        end
    end

    Logger->>Filters: evaluate filters for each target (in-place)
    alt Result == Log or LogFinal
        Logger->>Target: Write(logEvent) [async]
        alt Result == LogFinal
            Note right of Logger: stop processing further targets
        end
    else Result == Ignore or IgnoreFinal
        Logger->>Logger: InternalLogger.Debug("Rejected by filter")
        alt Result == IgnoreFinal
            Note right of Logger: stop processing further targets
        end
    end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

I hopped through filters, soft and neat,
sniffed call-sites with eager feet,
captured traces when I must,
warned when tumble turned to dust.
A rabbit's patch — efficient feat. 🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The pull request description is empty and does not provide any information about the changes, leaving reviewers without context about the introduced refactoring.	Please add a concise description summarizing the refactoring of CaptureCallSiteInfo and filter handling in LoggerImpl, including the removal of WriteToTargetWithFilterChain, so reviewers understand the purpose and scope of the changes.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title clearly and concisely describes the primary refactoring change—merging the WriteToTargetWithFilterChain logic into the Write method of LoggerImpl—which aligns with the main modifications in the changeset.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

✨ Finishing touches

📝 Generate Docstrings

🧪 Generate unit tests

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 3ea23c4 and 0652f36.

📒 Files selected for processing (2)

src/NLog/Internal/TargetWithFilterChain.cs (0 hunks)
src/NLog/LoggerImpl.cs (2 hunks)

💤 Files with no reviewable changes (1)

src/NLog/Internal/TargetWithFilterChain.cs

🧰 Additional context used

🧬 Code graph analysis (1)

src/NLog/LoggerImpl.cs (4)

src/NLog/Internal/TargetWithFilterChain.cs (9)

StackTraceUsage (98-118)

TargetWithFilterChain (47-388)

TargetWithFilterChain (51-51)

TargetWithFilterChain (61-66)

TargetWithFilterChain (120-128)

TargetWithFilterChain (204-215)

TargetWithFilterChain (295-312)

TryLookupCallSiteClassName (334-347)

TryRememberCallSiteClassName (314-332)

src/NLog/Targets/Target.cs (1)

WriteAsyncLogEvent (308-341)

src/NLog/LogEventInfo.cs (15)

LogEventInfo (51-844)

LogEventInfo (79-85)

LogEventInfo (93-96)

LogEventInfo (105-119)

LogEventInfo (129-134)

LogEventInfo (145-158)

LogEventInfo (169-185)

LogEventInfo (196-199)

LogEventInfo (210-219)

LogEventInfo (487-490)

LogEventInfo (499-502)

LogEventInfo (527-539)

LogEventInfo (550-553)

SetStackTrace (594-597)

CallSiteInformation (251-251)

src/NLog/Internal/CallSiteInformation.cs (2)

SetStackTrace (106-122)

CallSiteInformation (41-328)

⏰ 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)

src/NLog/LoggerImpl.cs (2)
73-96: Filter-result cache ignores FilterDefaultAction.

The reuse path only keys on FilterChain reference. If two TargetWithFilterChain instances share the same chain but have different FilterDefaultAction, a neutral evaluation will reuse the wrong action and mis-route the log event. Please include the default action in the cache key/state before reusing prevFilterResult.
-            IList<Filter> prevFilterChain = ArrayHelper.Empty<Filter>();
-            FilterResult prevFilterResult = FilterResult.Neutral;
+            IList<Filter> prevFilterChain = ArrayHelper.Empty<Filter>();
+            FilterResult prevFilterResult = FilterResult.Neutral;
+            FilterResult prevFilterDefaultAction = FilterResult.Neutral;
@@
-                FilterResult result = ReferenceEquals(prevFilterChain, currentFilterChain) ?
-                    prevFilterResult : GetFilterResult(currentFilterChain, logEvent, t.FilterDefaultAction);
+                FilterResult result = (ReferenceEquals(prevFilterChain, currentFilterChain) && prevFilterDefaultAction == t.FilterDefaultAction) ?
+                    prevFilterResult : GetFilterResult(currentFilterChain, logEvent, t.FilterDefaultAction);
@@
                 prevFilterResult = result;  // Cache the result, and reuse it for the next target, if it comes from the same logging-rule
                 prevFilterChain = currentFilterChain;
+                prevFilterDefaultAction = t.FilterDefaultAction;
55-58: Callsite capture still ignores filter StackTraceUsage.

targetsForLevel.StackTraceUsage only reflects the aggregated target chain. TargetWithFilterChain.PrecalculateStackTraceUsage() never inspects FilterChain, so filters that render ${callsite} (e.g., LayoutBasedFilter) no longer trigger CaptureCallSiteInfo, regressing callsite rendering. Please fold the filter-chain stack-trace requirements into the gating (either update PrecalculateStackTraceUsage() to OR the filter needs or compute the filter usage locally before skipping capture) so filters continue to get the callsite data they demand.

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.}

coderabbitai

Actionable comments posted: 2

🧹 Nitpick comments (1)

src/NLog/LoggerImpl.cs (1)

89-89: Include filter result in debug for quicker diagnosis

Logging the actual FilterResult helps triage why it stopped/continued.

-                    InternalLogger.Debug("{0} [{1}] Rejecting message because of a filter.", logEvent.LoggerName, logEvent.Level);
+                    InternalLogger.Debug("{0} [{1}] Rejecting message because of filter result: {2}.", logEvent.LoggerName, logEvent.Level, result);

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 68e9a96 and 3ea23c4.

📒 Files selected for processing (1)

src/NLog/LoggerImpl.cs (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/NLog/LoggerImpl.cs (5)

src/NLog/Internal/StackTraceUsageUtils.cs (1)

StackTraceUsage (50-68)

src/NLog/Internal/TargetWithFilterChain.cs (11)

StackTraceUsage (98-118)

TargetWithFilterChain (47-416)

TargetWithFilterChain (51-51)

TargetWithFilterChain (61-66)

TargetWithFilterChain (120-128)

TargetWithFilterChain (204-215)

TargetWithFilterChain (295-312)

TryCallSiteClassNameOptimization (314-326)

TryLookupCallSiteClassName (362-375)

MustCaptureStackTrace (328-340)

TryRememberCallSiteClassName (342-360)

src/NLog/Targets/Target.cs (1)

WriteAsyncLogEvent (308-341)

src/NLog/LogEventInfo.cs (14)

LogEventInfo (51-844)

LogEventInfo (79-85)

LogEventInfo (93-96)

LogEventInfo (105-119)

LogEventInfo (129-134)

LogEventInfo (145-158)

LogEventInfo (169-185)

LogEventInfo (196-199)

LogEventInfo (210-219)

LogEventInfo (487-490)

LogEventInfo (499-502)

LogEventInfo (527-539)

LogEventInfo (550-553)

SetStackTrace (594-597)

src/NLog/Internal/CallSiteInformation.cs (1)

SetStackTrace (106-122)

🔇 Additional comments (2)

src/NLog/LoggerImpl.cs (2)

81-86: Early-termination semantics look correct

Handling LogFinal/IgnoreFinal by breaking the loop matches expected behavior and avoids unnecessary target writes.

Also applies to: 89-91

99-133: Confirm log level and rethrow policy for call-site capture

Catch now emits InternalLogger.Warn instead of Error on capture failures and rethrows only when ThrowExceptions/LogManager.ThrowExceptions are set. Please confirm this change is intended and won’t mask important diagnostics on platforms with partial stack-trace support.

Verified removal of WriteToTargetWithFilterChain and that all CaptureCallSiteInfo calls use the new four-parameter signature.

src/NLog/LoggerImpl.cs

sonarqubecloud · 2025-09-27T20:59:56Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
89.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

snakefoot added the refactoring label Sep 27, 2025

pull-request-size bot added the size/M label Sep 27, 2025

snakefoot force-pushed the dev branch 2 times, most recently from 654a31e to 3ea23c4 Compare September 27, 2025 20:09

coderabbitai bot reviewed Sep 27, 2025

View reviewed changes

src/NLog/LoggerImpl.cs Show resolved Hide resolved

src/NLog/LoggerImpl.cs Show resolved Hide resolved

LoggerImpl - Merge WriteToTargetWithFilterChain into Write

0652f36

snakefoot force-pushed the dev branch from 3ea23c4 to 0652f36 Compare September 27, 2025 20:38

pull-request-size bot added size/L and removed size/M labels Sep 27, 2025

snakefoot merged commit b98723f into NLog:dev Sep 27, 2025
5 of 6 checks passed

snakefoot added this to the 6.0.5 milestone Sep 30, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LoggerImpl - Merge WriteToTargetWithFilterChain into Write#6008

LoggerImpl - Merge WriteToTargetWithFilterChain into Write#6008
snakefoot merged 1 commit intoNLog:devfrom
snakefoot:dev

snakefoot commented Sep 27, 2025

Uh oh!

coderabbitai bot commented Sep 27, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented Sep 27, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

snakefoot commented Sep 27, 2025

Uh oh!

coderabbitai bot commented Sep 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented Sep 27, 2025

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai bot commented Sep 27, 2025 •

edited

Loading