LNK-3066: Update Logging in DataAcquisition.PatientCensusService #536
Conversation
Also, removed string interpolation from TransientExceptionHandler.
|
Warning Rate limit exceeded@kissalk has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 11 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes made in this pull request focus on enhancing error logging and exception handling in two classes: Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
DotNet/DataAcquisition/Listeners/PatientCensusScheduledListener.cs (1)
Line range hint
56-74: Consider enhancing error context with structured loggingWhile the logging improvements are good, consider adding more context to help with debugging and monitoring. You could include relevant information such as facilityId and operation metadata using structured logging.
Example enhancement:
- Logger.LogError(ex, "Error occurred while processing the message"); + Logger.LogError(ex, "Error occurred while processing the message for facility {FacilityId}", facilityId);Similarly for the producer error:
- Logger.LogError(ex, "Error occurred while attempting to produce a message"); + Logger.LogError(ex, "Error occurred while attempting to produce message for facility {FacilityId} to topic {Topic}", + facilityId, KafkaTopic.PatientIDsAcquired);DotNet/Shared/Application/Error/Handlers/TransientExceptionHandler.cs (1)
69-69: Consider including exception details in the log message.While the structured logging format is good, consider including the exception message in the log for better debugging context:
-Logger.LogError(ex, "{Name}: Failed to process {ServiceName} Event.", GetType().Name, ServiceName); +Logger.LogError(ex, "{Name}: Failed to process {ServiceName} Event. Error: {ErrorMessage}", GetType().Name, ServiceName, ex.Message);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
DotNet/DataAcquisition/Listeners/PatientCensusScheduledListener.cs(2 hunks)DotNet/Shared/Application/Error/Handlers/TransientExceptionHandler.cs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
DotNet/DataAcquisition/Listeners/PatientCensusScheduledListener.cs (1)
Pattern **: PRs that have "TECH-DEBT" in the title should only contain changes related to typos, unused code, linter/IDE suggestions, swagger specification updates, and logging improvements. These TECH-DEBT PRs should not affect core functionality.
DotNet/Shared/Application/Error/Handlers/TransientExceptionHandler.cs (1)
Pattern **: PRs that have "TECH-DEBT" in the title should only contain changes related to typos, unused code, linter/IDE suggestions, swagger specification updates, and logging improvements. These TECH-DEBT PRs should not affect core functionality.
🔇 Additional comments (1)
DotNet/Shared/Application/Error/Handlers/TransientExceptionHandler.cs (1)
65-67: LGTM! Improved logging format.
The change from string interpolation to structured logging with named parameters improves log parsing and maintainability.
| @@ -70,6 +70,7 @@ protected override async Task ExecuteListenerAsync(ConsumeResult<string, Patient | |||
| } | |||
| catch (Exception ex) | |||
| { | |||
| Logger.LogError(ex, "Error occurred while attempting to produce a message: {1}", ex.Message); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix the logging statement format
Similar to the previous logging statement, this one has incorrect placeholder indexing and redundant message logging.
Apply this change for better structured logging:
- Logger.LogError(ex, "Error occurred while attempting to produce a message: {1}", ex.Message);
+ Logger.LogError(ex, "Error occurred while attempting to produce a message");📝 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.
| Logger.LogError(ex, "Error occurred while attempting to produce a message: {1}", ex.Message); | |
| Logger.LogError(ex, "Error occurred while attempting to produce a message"); |
| @@ -53,7 +53,7 @@ | |||
| } | |||
| catch (Exception ex) | |||
| { | |||
| Logger.LogError(ex, "Error occurred while processing the message."); | |||
| Logger.LogError(ex, "Error occurred while processing the message: {1}", ex.Message); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve the logging statement format
The current logging statement has incorrect placeholder indexing and redundant message logging. The exception message is already included when passing the exception object as the first parameter.
Apply this change for better structured logging:
- Logger.LogError(ex, "Error occurred while processing the message: {1}", ex.Message);
+ Logger.LogError(ex, "Error occurred while processing the message");📝 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.
| Logger.LogError(ex, "Error occurred while processing the message: {1}", ex.Message); | |
| Logger.LogError(ex, "Error occurred while processing the message"); |
|
|
||
| ProduceRetryScheduledEvent(consumeResult.Message.Key, consumeResult.Message.Value, | ||
| consumeResult.Message.Headers, facilityId, ex.Message, ex.StackTrace ?? string.Empty); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| Logger.LogError(e, $"Error in {GetType().Name}.HandleException: " + e.Message); | ||
| Logger.LogError(e, "Error in {Name}.HandleException: " + e.Message, GetType().Name); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fully adopt structured logging format.
The current implementation mixes structured logging with string concatenation. Consider using structured logging consistently:
-Logger.LogError(e, "Error in {Name}.HandleException: " + e.Message, GetType().Name);
+Logger.LogError(e, "Error in {Name}.HandleException: {ErrorMessage}", GetType().Name, e.Message);📝 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.
| Logger.LogError(e, "Error in {Name}.HandleException: " + e.Message, GetType().Name); | |
| Logger.LogError(e, "Error in {Name}.HandleException: {ErrorMessage}", GetType().Name, e.Message); |
Description of Changes
Updates to bubble up sub error messages in logging. Also, removed string interpolation from TransientExceptionHandler.
Please provide a high-level overview of the changes included in this PR.
🧪 Testing Performed
Performed testing against my local environment.
Summary by CodeRabbit
Bug Fixes
PatientCensusScheduledListenerto include specific exception messages for better context.TransientExceptionHandlerfor clearer and more consistent error messages.Documentation