Sonar Bucket B (part 1): structured logging templates (CA2254/S2629)#190
Merged
Conversation
Convert 69 interpolated logging calls ($"...{x}...") to static message
templates with named placeholders + args across core, all transports, and the
integration-test notification helper. Output preserved exactly (Environment.NewLine
passed as an arg where it was interpolated). Clears CA2254 and its Sonar mirror S2629.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR converts logging across core queue, worker, scheduler, transport, and test code from string interpolation to structured message templates with explicit arguments. It also adds tests that verify template rendering matches equivalent interpolated output. ChangesStructured Logging Conversion
Estimated code review effort: 2 (Simple) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Source/DotNetWorkQueue/Logging/Decorator/IResetHeartBeatDecorator.cs (1)
58-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTypo: "where" should be "were".
The message text reads "records that where outside of the heartbeat window" — "where" should be "were". This is pre-existing but now part of the changed line, so it's a trivial fix while here.
✏️ Proposed fix
- "Reset the status of {Count} records that where outside of the heartbeat window of {HeartBeatSeconds} seconds", count.Count, _configuration.HeartBeat.Time.TotalSeconds); + "Reset the status of {Count} records that were outside of the heartbeat window of {HeartBeatSeconds} seconds", count.Count, _configuration.HeartBeat.Time.TotalSeconds);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/DotNetWorkQueue/Logging/Decorator/IResetHeartBeatDecorator.cs` at line 58, The log message in IResetHeartBeatDecorator contains a typo in the text passed to the reset-status message. Update the string in the logging call that references Count and HeartBeatSeconds so it says “records that were outside of the heartbeat window” instead of “where,” keeping the rest of the message unchanged.Source/DotNetWorkQueue/TaskScheduling/TaskScheduler.cs (1)
146-149: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider consistent placeholder names for the same semantic value.
The overall task count is logged as
{Queue}inIncrementGroup/DecrementGroup(lines 149, 187) but as{Current}inIncrementCounter/DecrementCounter(lines 161, 172). Meanwhile,{Current}is used for the group-level count inIncrementGroup/DecrementGroup(lines 146, 184) but for the overall count inIncrementCounter/DecrementCounter. This means the same placeholder name maps to different values depending on the log source, which complicates structured log queries.Using distinct, consistent names (e.g.,
{GroupTaskCount}for group-level and{TaskCount}for overall) would improve log searchability.Also applies to: 161-161, 172-172, 184-187
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/DotNetWorkQueue/TaskScheduling/TaskScheduler.cs` around lines 146 - 149, The logging in TaskScheduler currently reuses placeholder names for different meanings across IncrementGroup, DecrementGroup, IncrementCounter, and DecrementCounter, which makes structured log queries ambiguous. Update the Log calls in these methods to use distinct, consistent names for the group-level count versus the overall task count, and apply the same naming scheme everywhere the counts are logged so the semantic value of each placeholder is always the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Source/DotNetWorkQueue/Logging/Decorator/IResetHeartBeatDecorator.cs`:
- Line 58: The log message in IResetHeartBeatDecorator contains a typo in the
text passed to the reset-status message. Update the string in the logging call
that references Count and HeartBeatSeconds so it says “records that were outside
of the heartbeat window” instead of “where,” keeping the rest of the message
unchanged.
In `@Source/DotNetWorkQueue/TaskScheduling/TaskScheduler.cs`:
- Around line 146-149: The logging in TaskScheduler currently reuses placeholder
names for different meanings across IncrementGroup, DecrementGroup,
IncrementCounter, and DecrementCounter, which makes structured log queries
ambiguous. Update the Log calls in these methods to use distinct, consistent
names for the group-level count versus the overall task count, and apply the
same naming scheme everywhere the counts are logged so the semantic value of
each placeholder is always the same.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 47822fdd-8269-4d84-9176-7dd4943b0e1f
📒 Files selected for processing (39)
Source/DotNetWorkQueue.IntegrationTests.Shared/CreateNotifications.csSource/DotNetWorkQueue.IntegrationTests.Shared/OsDetectionHelper.csSource/DotNetWorkQueue.Transport.LiteDB/Basic/DatabaseExists.csSource/DotNetWorkQueue.Transport.LiteDB/Basic/LiteDbQueueReceiveMessages.csSource/DotNetWorkQueue.Transport.PostgreSQL/Basic/RemoveMessage.csSource/DotNetWorkQueue.Transport.PostgreSQL/Basic/RetrySQLPolicyCreation.csSource/DotNetWorkQueue.Transport.Redis/Basic/Logging/Decorator/DelayedProcessingActionDecorator.csSource/DotNetWorkQueue.Transport.Redis/Basic/Logging/Decorator/ReceiveMessageQueryDecorator.csSource/DotNetWorkQueue.Transport.Redis/Basic/RedisQueueReceiveMessagesError.csSource/DotNetWorkQueue.Transport.SQLite/Basic/RetryTransactionPolicyCreation.csSource/DotNetWorkQueue.Transport.SQLite/Basic/SqLiteMessageQueueReceive.csSource/DotNetWorkQueue.Transport.SQLite/Decorator/FindExpiredRecordsToDeleteQueryHandlerErrorDecorator.csSource/DotNetWorkQueue.Transport.SQLite/Decorator/FindRecordsToResetByHeartBeatDecorator.csSource/DotNetWorkQueue.Transport.Shared/Basic/ReceiveErrorMessage.csSource/DotNetWorkQueue.Transport.SqlServer/Basic/Message/RollbackMessage.csSource/DotNetWorkQueue.Transport.SqlServer/Basic/RemoveMessage.csSource/DotNetWorkQueue.Transport.SqlServer/Basic/RetrySqlPolicyCreation.csSource/DotNetWorkQueue/JobScheduler/JobScheduler.csSource/DotNetWorkQueue/JobScheduler/ScheduledJob.csSource/DotNetWorkQueue/Logging/Decorator/IClearErrorMessagesDecorator.csSource/DotNetWorkQueue/Logging/Decorator/IClearExpiredMessagesDecorator.csSource/DotNetWorkQueue/Logging/Decorator/IReceivePoisonMessageDecorator.csSource/DotNetWorkQueue/Logging/Decorator/IResetHeartBeatDecorator.csSource/DotNetWorkQueue/Logging/Decorator/IRollbackMessageDecorator.csSource/DotNetWorkQueue/Messages/ReceivedMessage.csSource/DotNetWorkQueue/Queue/BaseMonitor.csSource/DotNetWorkQueue/Queue/HeartBeatScheduler.csSource/DotNetWorkQueue/Queue/HeartBeatWorker.csSource/DotNetWorkQueue/Queue/MessageExceptionHandler.csSource/DotNetWorkQueue/Queue/MessageProcessing.csSource/DotNetWorkQueue/Queue/MessageProcessingAsync.csSource/DotNetWorkQueue/Queue/PrimaryWorker.csSource/DotNetWorkQueue/Queue/ProducerQueue.csSource/DotNetWorkQueue/Queue/StopWorkers.csSource/DotNetWorkQueue/Queue/Worker.csSource/DotNetWorkQueue/Queue/WorkerCollection.csSource/DotNetWorkQueue/TaskScheduling/TaskScheduler.csSource/DotNetWorkQueue/Time/BaseTime.csSource/DotNetWorkQueue/Transport/Memory/Basic/ReceiveErrorMessage.cs
Add LogMessageTemplateTests: renders representative converted templates (single/multi placeholder, repeated Environment.NewLine, exception arg, retry template) through a capturing ILogger and asserts the output equals the original string-interpolation. Guards against placeholder/argument count mismatch, which the logging framework renders silently rather than throwing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/DotNetWorkQueue.Tests/Logging/LogMessageTemplateTests.cs`:
- Around line 1-5: Add the missing LGPL-2.1 license header to this new C# test
file so it matches the repository’s source-file policy. Place the standard
header at the top of LogMessageTemplateTests.cs before the using statements, and
keep the rest of the file unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff97d1c1-b090-42f0-b5a1-face819a446c
📒 Files selected for processing (1)
Source/DotNetWorkQueue.Tests/Logging/LogMessageTemplateTests.cs
Comment on lines
+1
to
+5
| using System; | ||
| using Microsoft.Extensions.Logging; | ||
| using Microsoft.VisualStudio.TestTools.UnitTesting; | ||
|
|
||
| namespace DotNetWorkQueue.Tests.Logging |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add LGPL-2.1 license header.
Per coding guidelines, all .cs source files must include an LGPL-2.1 license header. This new file is missing it.
As per coding guidelines: **/*.cs: All source files must include LGPL-2.1 license headers.
📄 Add license header
+// Copyright (c) DotNetWorkQueue contributors
+// Licensed under the LGPL-2.1 license. See LICENSE file in the project root for full license information.
+
using System;
using Microsoft.Extensions.Logging;
using Microsoft.VisualStudio.TestTools.UnitTesting;📝 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.
Suggested change
| using System; | |
| using Microsoft.Extensions.Logging; | |
| using Microsoft.VisualStudio.TestTools.UnitTesting; | |
| namespace DotNetWorkQueue.Tests.Logging | |
| // Copyright (c) DotNetWorkQueue contributors | |
| // Licensed under the LGPL-2.1 license. See LICENSE file in the project root for full license information. | |
| using System; | |
| using Microsoft.Extensions.Logging; | |
| using Microsoft.VisualStudio.TestTools.UnitTesting; | |
| namespace DotNetWorkQueue.Tests.Logging |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Source/DotNetWorkQueue.Tests/Logging/LogMessageTemplateTests.cs` around lines
1 - 5, Add the missing LGPL-2.1 license header to this new C# test file so it
matches the repository’s source-file policy. Place the standard header at the
top of LogMessageTemplateTests.cs before the using statements, and keep the rest
of the file unchanged.
Source: Coding guidelines
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #190 +/- ##
==========================================
- Coverage 87.54% 87.51% -0.04%
==========================================
Files 1023 1023
Lines 33614 33614
Branches 2803 2803
==========================================
- Hits 29429 29418 -11
- Misses 3324 3332 +8
- Partials 861 864 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fifth SonarCloud sweep (Bucket B, part 1) — structured logging, following #184, #186, #188, #189.
What changed
Converted 69 interpolated logging calls (
$"...{x}...") to static message templates with named placeholders + arguments:Across core, all transports (SqlServer/PostgreSQL/SQLite/Redis/LiteDB/Shared/Memory), and the integration-test notification helper. Clears CA2254 and its Sonar mirror S2629 (~62 shared + 7 test sites).
Output preserved exactly — where
Environment.NewLinewas interpolated it's passed as an argument (rather than dropped or hardcoded), so multi-line log formatting is unchanged.Not in this PR
CA1873 ("argument may be expensive if logging is disabled", 37) is a different fix (
IsEnabledguards /LoggerMessage). ~25 of its sites are the same calls converted here, so templating clears the cheap-arg ones automatically; the residual (Dashboard services + a few method-call args) is a focused follow-up.Verification
Log*($"...")calls remainNo changelog/version bump (matches the #186/#188/#189 mechanical-cleanup precedent).
🤖 Generated with Claude Code
Summary by CodeRabbit