Skip to content

InternalLogger - Skip allocating params array when no exception#6142

Merged
snakefoot merged 1 commit into
NLog:devfrom
snakefoot:InternalLoggerStringFormat
Apr 4, 2026
Merged

InternalLogger - Skip allocating params array when no exception#6142
snakefoot merged 1 commit into
NLog:devfrom
snakefoot:InternalLoggerStringFormat

Conversation

@snakefoot
Copy link
Copy Markdown
Contributor

No description provided.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 4, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 87d610e5-9f12-4e9e-ad94-70b58023913b

📥 Commits

Reviewing files that changed from the base of the PR and between aed7494 and d77de73.

📒 Files selected for processing (4)
  • src/NLog/Common/InternalLogger.cs
  • src/NLog/Config/AssemblyExtensionLoader.cs
  • src/NLog/Config/XmlLoggingConfiguration.cs
  • src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs
✅ Files skipped from review due to trivial changes (3)
  • src/NLog/Config/XmlLoggingConfiguration.cs
  • src/NLog/Config/AssemblyExtensionLoader.cs
  • src/NLog/Common/InternalLogger.cs
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs

Walkthrough

Adjustments to internal logging and string handling: timestamp formatting in InternalLogger, explicit exception-branching in log message construction, replacement of "" with string.Empty in two configs, and allowing null to be passed for fileWildcard in archive-handler logs.

Changes

Cohort / File(s) Summary
Internal Logging Formatting
src/NLog/Common/InternalLogger.cs
Timestamp format updated to include a trailing space; exception message construction changed from conditional/inline expressions to explicit if (ex is null) ... else ..., causing the " Exception: " segment to be omitted when ex is null and appended with ex.ToString() when present.
String Literal Standardization
src/NLog/Config/AssemblyExtensionLoader.cs, src/NLog/Config/XmlLoggingConfiguration.cs
Replaced empty string literal ("") usage with string.Empty when composing log/exception messages; interpolation and conditional suffixing behavior unchanged.
Archive Handler Null Handling
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs
Removed null-coalescing for fileWildcard in a log call so fileWildcard may be passed as null to the formatter instead of an empty string.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 I hopped through code with a curious nose,
Tucked spaces in timestamps where bright light glows,
Empty strings neat, branches tidy and clear,
Nulls left honest so logs tell it sincere.
A little hop—now the logs whisper near.

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive No description was provided by the author, making it impossible to assess relevance to the changeset. Add a pull request description explaining the optimization goal and its impact on performance and memory allocation.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the main optimization: reducing params array allocation in InternalLogger when no exception is present.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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
Copy Markdown

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

🧹 Nitpick comments (1)
src/NLog/Common/InternalLogger.cs (1)

388-433: Indentation inconsistency in the else branch (lines 425-430).

The string.Concat call at lines 425-430 uses different indentation compared to its counterpart at lines 405-411. The arguments in the non-timestamp else branch are indented by 4 spaces relative to return, while the timestamp branch aligns arguments under the opening parenthesis.

💅 Proposed fix for consistent indentation
             else
             {
                 if (ex is null)
                 {
                     return string.Concat(
                         level.ToString(),
                         fieldSeparator,
                         fullMessage);
                 }
                 else
                 {
                     return string.Concat(
-                    level.ToString(),
-                    fieldSeparator,
-                    fullMessage,
-                    " Exception: ",
-                    ex.ToString());
+                        level.ToString(),
+                        fieldSeparator,
+                        fullMessage,
+                        " Exception: ",
+                        ex.ToString());
                 }
             }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/NLog/Common/InternalLogger.cs` around lines 388 - 433, The CreateLogLine
method has inconsistent indentation in the non-timestamp else branch for the
string.Concat call; align the argument indentation to match the timestamp branch
style so the arguments are vertically aligned under the opening parenthesis
(same as the block that returns TimeSource.Current.Time.ToString(...)). Update
the string.Concat block in the else branch (the branch where IncludeTimestamp is
false and ex is not null) so its arguments (level.ToString(), fieldSeparator,
fullMessage, " Exception: ", ex.ToString()) follow the same indentation pattern
as the other string.Concat usages in CreateLogLine.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/NLog/Common/InternalLogger.cs`:
- Around line 388-433: The CreateLogLine method has inconsistent indentation in
the non-timestamp else branch for the string.Concat call; align the argument
indentation to match the timestamp branch style so the arguments are vertically
aligned under the opening parenthesis (same as the block that returns
TimeSource.Current.Time.ToString(...)). Update the string.Concat block in the
else branch (the branch where IncludeTimestamp is false and ex is not null) so
its arguments (level.ToString(), fieldSeparator, fullMessage, " Exception: ",
ex.ToString()) follow the same indentation pattern as the other string.Concat
usages in CreateLogLine.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: d1781f5c-a895-41ac-acce-065159a38c91

📥 Commits

Reviewing files that changed from the base of the PR and between 8ba58a4 and aed7494.

📒 Files selected for processing (4)
  • src/NLog/Common/InternalLogger.cs
  • src/NLog/Config/AssemblyExtensionLoader.cs
  • src/NLog/Config/XmlLoggingConfiguration.cs
  • src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs

@snakefoot snakefoot closed this Apr 4, 2026
@snakefoot snakefoot reopened this Apr 4, 2026
@snakefoot snakefoot force-pushed the InternalLoggerStringFormat branch from aed7494 to d77de73 Compare April 4, 2026 09:19
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Apr 4, 2026

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Apr 4, 2026

@snakefoot snakefoot merged commit 8fa3c06 into NLog:dev Apr 4, 2026
6 of 7 checks passed
@snakefoot snakefoot added this to the 6.1.3 milestone May 4, 2026
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