feature: Structured Logging Support #141
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces significant enhancements to the Arc4u.Diagnostics library, focusing on advanced logging capabilities and improved project organization. Key changes include the addition of OpenTelemetry package references for .NET 8.0 and 9.0, the creation of extension methods to integrate logging into ASP.NET Core applications, and the implementation of a robust Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant HostBuilder as IHostBuilder
participant LoggerContext as LoggerWrapperContext
participant Logger as LoggerWrapper
App->>HostBuilder: UseArc4uLogging()
HostBuilder-->>App: Configured Logging
App->>LoggerContext: Initialize(ServiceProvider)
LoggerContext-->>App: Context Initialized
App->>Logger: Technical/Business/Monitoring Log
Logger-->>App: Enriched Log Message
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (13)
src/Arc4u.sln (1)
101-106: Well-structured solution organization!The addition of "sample", "src", and "tests" solution folders follows best practices for .NET solution organization, providing clear separation of concerns between source code, tests, and examples.
src/Arc4u.Diagnostics/Logging/LoggerWrapper.cs (3)
13-14: Potential Concerns with Embedding Object in Dictionary
The field '_additionalFields' is declared as a Dictionary<string, object?>. You might eventually need more robust handling (e.g., concurrency) if multiple threads log concurrently and update fields simultaneously.
52-75: Leverage Named Templates for Structured Logging
Here, you define separate log templates (TechnicalLog, BusinessLog, MonitoringLog). This is good for structured logging, but consider adding more placeholders (e.g., methodName, className) or using parseable keys for easier indexing in log aggregators.
137-147: Dispose Pattern
The class implements Dispose but does not do any resource clean-up besides flipping '_disposed'. If the underlying '_logger' or related objects require disposing, ensure they're also handled here.src/Arc4u.Diagnostics/Logging/ApplicationBuilderExtensions.cs (1)
7-11: Initialization Timing
This extension method initializes the LoggerWrapperContext with the ASP.NET Core service provider. Ensure this call is placed at an appropriate point in the pipeline (often after required services are registered) so that loggers are fully functional.src/Arc4u.Diagnostics/MessageProperty.cs (1)
Line range hint
5-13: Null Handling
In AddIfNotExist(), you skip adding if 'value == null'. This helps avoid storing null, but some logs or systems may want explicit tracking of null. This design choice can be fine, but confirm it aligns with your usage.src/Arc4u.Diagnostics/Logging/HostBuilderExtensions.cs (1)
8-19: Validate Logging Configuration
Your 'UseArc4uLogging' extension method delegates to 'ConfigureLogging', optionally applying the user-provided delegate 'configure'. Ensure you handle conflicts if the user calls 'ConfigureLogging' multiple times.src/Arc4u.Diagnostics/Logging/LoggerWrapperContext.cs (1)
8-9: Consider null-safety improvementsThe Initialize method could benefit from additional validation and thread-safety.
Consider this implementation:
-public static void Initialize(IServiceProvider serviceProvider) - => _scopeFactory = serviceProvider.GetRequiredService<IServiceScopeFactory>(); +public static void Initialize(IServiceProvider serviceProvider) +{ + ArgumentNullException.ThrowIfNull(serviceProvider); + if (_scopeFactory != null) + throw new InvalidOperationException("LoggerContext already initialized"); + _scopeFactory = serviceProvider.GetRequiredService<IServiceScopeFactory>(); +}src/Arc4u.Diagnostics/Logging/ILoggerExtensions.cs (2)
8-15: Add XML documentation for public APIThese public extension methods would benefit from XML documentation describing their purpose and usage.
Add documentation for better IntelliSense support:
+/// <summary> +/// Creates a technical category logger wrapper for structured logging. +/// </summary> +/// <param name="logger">The logger instance.</param> +/// <param name="methodName">The calling method name (automatically populated).</param> +/// <returns>A new LoggerWrapper instance for technical logging.</returns> public static LoggerWrapper<T> Technical<T>(this ILogger logger, [CallerMemberName] string methodName = "")
8-24: Consider reducing code duplicationThe implementation is duplicated between ILogger and ILogger extensions. Consider refactoring to reduce duplication.
Consider implementing the ILogger extensions in terms of the ILogger ones:
public static LoggerWrapper<T> Technical<T>(this ILogger<T> logger, [CallerMemberName] string methodName = "") => - new(logger, MessageCategory.Technical, typeof(T), methodName); + Technical<T>((ILogger)logger, methodName);src/Arc4u.Diagnostics/Logging/LoggerWrapperExtensions.cs (3)
35-40: Consider consolidating duplicate methodsThe
AddOrReplacemethod is identical to theAddmethod in implementation. This duplication might be confusing for users as both methods perform the same operation (dictionary[key] = value always replaces existing values).Consider either:
- Removing
AddOrReplaceand documenting thatAddalways replaces existing values- Renaming
AddtoAddOrReplaceto be more explicit about the behavior
59-80: Enhance validation documentation and error messagesThe key validation logic could be improved in two ways:
- Add XML documentation to explain the reserved keys and their purpose
- Provide a more descriptive ArgumentNullException message
+/// <summary> +/// Validates that the provided key is not null, empty, or a reserved logging key. +/// </summary> +/// <param name="key">The key to validate</param> +/// <returns>The validated key</returns> +/// <exception cref="ArgumentNullException">Thrown when key is null or empty</exception> +/// <exception cref="ReservedLoggingKeyException">Thrown when key is a reserved logging key</exception> private static string ValidateKey(string key) { if (string.IsNullOrWhiteSpace(key)) { - throw new ArgumentNullException(nameof(key)); + throw new ArgumentNullException(nameof(key), "Logging key cannot be null or empty"); }
1-81: Well-structured implementation of fluent logging interfaceThe implementation provides a clean and consistent API for structured logging that aligns well with the PR objectives. The fluent interface allows for easy chaining of logging configurations, and the validation ensures proper usage of logging keys.
Some architectural considerations:
- The extension methods provide a good abstraction layer for logging configuration
- The fluent interface pattern is consistently applied
- Key validation helps prevent conflicts with system-reserved keys
Consider adding integration tests to verify the behavior of these extension methods with different logging providers (Serilog, NLog) to ensure compatibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/Arc4u.Diagnostics/Arc4u.Diagnostics.csproj(1 hunks)src/Arc4u.Diagnostics/Logging/ApplicationBuilderExtensions.cs(1 hunks)src/Arc4u.Diagnostics/Logging/HostBuilderExtensions.cs(1 hunks)src/Arc4u.Diagnostics/Logging/ILoggerExtensions.cs(1 hunks)src/Arc4u.Diagnostics/Logging/LoggerWrapper.cs(1 hunks)src/Arc4u.Diagnostics/Logging/LoggerWrapperContext.cs(1 hunks)src/Arc4u.Diagnostics/Logging/LoggerWrapperExtensions.cs(1 hunks)src/Arc4u.Diagnostics/MessageProperty.cs(2 hunks)src/Arc4u.UnitTest/Logging/SerilogTests.cs(1 hunks)src/Arc4u.sln(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Arc4u.UnitTest/Logging/SerilogTests.cs
🔇 Additional comments (9)
src/Arc4u.Diagnostics/Logging/LoggerWrapper.cs (4)
81-85: Avoid Short-Circuiting in Log Method
You return immediately if logging is disabled for the current level. Typically fine, but if you rely on any side-effects (e.g., metrics) prior to the check, you might skip them. Ensure you intend to skip all side-effects.
86-135: Exception Handling within Logging
Catching and re-logging exceptions could mask their original stack trace if done improperly. Currently, after catching the exception in line 130, you call '_logger.Log(...)' with the same level. This is typically okay, but ensure there's no duplication or unintentional infinite loop if the exception is triggered by the logger itself.
205-242: Check for Potential Recursion in GetAllExceptions()
When dealing with nested AggregateException or InnerExceptions, ensure there's no cyclical reference that might cause infinite recursion.
26-35: Ensure Environment.ProcessId Works on All Targeted Platforms
Accessing 'Environment.ProcessId' can throw on non-supported platforms, hence the try-catch. However, confirm you have test coverage or fallback logic for the scenarios where it returns -1.
✅ Verification successful
Based on the search results, I can generate the final response as I have enough context about how ProcessId is used throughout the codebase:
ProcessId Fallback is Properly Handled Across the Codebase
The -1 fallback value for ProcessId is consistently handled across the codebase. The value is used primarily for logging purposes in various formatters and sinks, where it's padded and displayed as part of log messages. No critical logic depends on the actual process ID value, making the fallback safe and appropriate.
Key observations:
- ConsoleTraceListener, FileLoggerTraceListener, and SimpleTextFormatter all handle ProcessId by just formatting it as a string
- LoggerMessage and LoggerWrapper both implement the same try-catch pattern for ProcessId
- The value is consistently treated as a display/diagnostic field rather than a critical identifier
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for usage or checks around 'ProcessId' usage to ensure fallback logic
rg -A 3 "ProcessId"
Length of output: 13664
src/Arc4u.Diagnostics/MessageProperty.cs (1)
15-18: Argument Checking
Throwing an ArgumentNullException for a null 'value' in AddOrReplace() ensures no null data is stored. If a user explicitly wants to store null, they'll be forced to code differently. Confirm this is intended.
src/Arc4u.Diagnostics/Arc4u.Diagnostics.csproj (2)
17-17: LGTM: Framework reference aligns with PR objectives
The addition of Microsoft.AspNetCore.App framework reference aligns with the PR's goal of enhancing logging capabilities and integration with Microsoft.Extensions.Logging.
9-9: Verify OpenTelemetry version compatibility
The OpenTelemetry package versions differ between frameworks (1.9.0 for net8.0 vs 1.10.0 for net9.0). While this follows the framework versioning pattern, we should verify compatibility.
Also applies to: 14-14
✅ Verification successful
OpenTelemetry package versions are appropriate for respective frameworks
The package versions are correctly aligned with their target frameworks:
- net8.0: OpenTelemetry.Extensions.Hosting 1.9.0 with Microsoft.Extensions.Hosting 8.0.1
- net9.0: OpenTelemetry.Extensions.Hosting 1.10.0 with Microsoft.Extensions.Hosting 9.0.0
No security vulnerabilities were found for these versions, and the versioning follows a consistent pattern with other framework dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any known compatibility issues or breaking changes between versions
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NUGET, package: "OpenTelemetry.Extensions.Hosting") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 367
Script:
#!/bin/bash
# Check for OpenTelemetry package references across the solution
rg -l "OpenTelemetry\.Extensions\.Hosting" --type csproj
# Check the target frameworks in the project file
cat src/Arc4u.Diagnostics/Arc4u.Diagnostics.csproj
Length of output: 1111
src/Arc4u.Diagnostics/Logging/LoggerWrapperContext.cs (1)
11-18: LGTM: Proper error handling for scope creation
The CreateScope method correctly handles the null case and throws an appropriate exception.
src/Arc4u.Diagnostics/Logging/LoggerWrapperExtensions.cs (1)
1-6: Verify namespace choice for architectural consistency
The extension methods are placed in the Microsoft.Extensions.DependencyInjection namespace while using Arc4u.Diagnostics types. While this is a common practice for extension methods to be discoverable, please verify if this aligns with the project's architectural guidelines.
✅ Verification successful
Namespace choice is architecturally consistent
The namespace choice of Microsoft.Extensions.DependencyInjection for LoggerWrapperExtensions is consistent with the project's architectural patterns. This is evidenced by:
- Multiple extension classes in Arc4u.Diagnostics namespace that extend Microsoft's functionality
- Similar extension patterns across the codebase where functionality is added to Microsoft's DI container
- The presence of numerous other files using Microsoft.Extensions.DependencyInjection, indicating tight integration with Microsoft's DI ecosystem
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for similar extension method patterns in the codebase
fd -e cs -x grep -l "namespace Microsoft.Extensions.DependencyInjection" {} \; | xargs grep -l "static class.*Extensions"
Length of output: 185
Script:
#!/bin/bash
# Let's check for other extension classes in Arc4u.Diagnostics namespace
fd -e cs -x grep -l "namespace Arc4u.Diagnostics" {} \; | xargs grep -l "static class.*Extensions"
# Also check if there are any other DI-related extensions in the codebase
fd -e cs -x grep -l "using Microsoft.Extensions.DependencyInjection" {} \;
Length of output: 6748
| GlobalSection(NestedProjects) = preSolution | ||
| {CCA589E1-100F-47A6-A34B-FD46FDFDAA2A} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {511B1EB2-F853-4C5C-80A7-0825AECF1BBE} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {B270FC8C-6B28-40FA-BA78-B73F720DB98D} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {705ADAFC-9C5C-4DCB-93C5-B71D49371074} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {C1048831-D985-481B-BA50-2840315EFFEA} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {29DA3E34-4769-457F-B583-8BCFF5958E41} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {531058F7-1DD4-4E51-8727-A2D038B57274} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {1AD8095D-CEF1-486F-912B-CB9C6DCE0A20} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {4AC131B6-9FF2-4F66-A2DE-BE66C3CA366B} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {42B7DC08-23E2-4854-A5DB-71E9FB3BAF6D} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {51D25BBB-E6E5-46AF-81A1-B854F715F2C0} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {150D036C-4B66-4A99-BAA9-28AC0A81E2D7} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {F333A9E4-52F7-473D-AFCE-46A4FCFE844C} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {9B9ABE45-A069-4E97-BAEF-5C273CF4C8AB} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {0F2EEF43-174D-436D-A5B1-DA92D3EF9B25} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {C10B3D6C-E5E4-4365-B5E5-826A2C7A44AE} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {62867DEB-C095-4D34-8C56-3F0C3A0FE3B7} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {39E14B92-11E6-433E-8B5C-082485F16DA9} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {F0D96654-0415-4540-A224-44C2585F825E} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {86290E0E-3450-4CFD-80B3-8EED35E2A121} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {64CBEAB5-41B7-47CE-95E7-62CE3ECFE2FC} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {F0EF3965-11F5-4F8E-8D8C-8A1BC32DFFE0} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {E954455E-AB54-4144-B784-1E68B6548482} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {1959B659-8A69-4D68-A198-C627464C2143} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {5157EBFA-D53F-49A3-BDC6-B4887D4F6E2F} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {FCFE7961-7B48-42BF-BD72-67A1C0858BB1} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {FACC7AF6-8AAF-41B9-A0EF-0E6285EE1CA7} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {F854BCF4-D7DF-46D3-809A-0EF53586EC00} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {F018AC7E-2799-48E3-BAC0-79C94452085A} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {46D4C073-FD32-4C23-B22A-306E69122BF6} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {30D7D397-A29D-4148-AD66-EF26495A5BC0} = {E8E0B4BA-5659-40E6-94C8-CA6D9F339101} | ||
| {EA273269-7720-48DE-8866-FC82E2C9490F} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {6ED0D79F-96D4-418A-9C75-69E165FA5682} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {FB64F43E-F22E-4202-9F33-840CB7F44AA1} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {EB7B7062-7366-4262-8686-3DDAA19141B4} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {B16F1734-B286-4898-8B15-EFA0D17DA912} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {06393D15-0DA9-40FA-84A1-9A822B627B52} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {1910732C-CF4C-4411-B29F-F75185DD2F9B} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {2B61C53F-ECFC-471D-AB98-0D23FF2D071A} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {6FD0A0B7-A94C-4DBD-AF62-CE3B5359F088} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {AE2BA9A5-AFCD-4D98-87DC-71D6DCBC2C21} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| {2C83BE21-3C11-46B2-9831-5B84E4912BFB} = {B9419F5B-9CF3-4247-9929-7AF2362FD8E3} | ||
| EndGlobalSection |
There was a problem hiding this comment.
💡 Codebase verification
Remove unused "sample" folder section from solution file
The "sample" folder exists only as an empty section in the solution file without any actual projects or files associated with it. Since this PR focuses on logging changes and there are no sample projects demonstrating these changes, the empty sample section should be removed from the solution file to maintain a clean project structure.
🔗 Analysis chain
Verify the need for the empty "sample" folder
While the project organization is good, with source code and test projects properly separated, the "sample" folder remains empty. Consider either:
- Adding sample projects demonstrating the new structured logging implementation, or
- Removing the "sample" folder if it's not needed for this PR.
This is particularly relevant as the PR introduces significant logging changes that could benefit from example implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any sample projects in the repository that should be included
fd -t f -e csproj . | grep -i "sample"
Length of output: 38
Script:
#!/bin/bash
# Let's check the solution file for any sample-related sections or folders
cat src/Arc4u.sln | grep -i "sample"
# Also check if there are any sample-related folders in the repository
fd -t d "sample"
Length of output: 166
| using var scope = LoggerWrapperContext.CreateScope(); | ||
| var providers = scope.ServiceProvider | ||
| .GetServices<IAddPropertiesToLog>(); | ||
|
|
||
| properties = new Dictionary<string, object?>(AdditionalFields); | ||
| var definedProperties = providers.Select(x => x.GetProperties()).SelectMany(x => x); | ||
| if (definedProperties != null) | ||
| { | ||
| foreach (var property in definedProperties) | ||
| { | ||
| if (property.Value != null) | ||
| { | ||
| this.AddIfNotExist(property.Key, property.Value); | ||
| } | ||
| } | ||
|
|
||
| return properties; | ||
| } | ||
|
|
||
| return new Dictionary<string, object?>(AdditionalFields); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.LogWarning(ex, "Error getting property providers. Logging without additional properties."); | ||
| return new Dictionary<string, object?>(AdditionalFields); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential Performance Implication of GetServices<IAddPropertiesToLog>()
You're calling 'GetServices()' every time you add additional properties, which can create overhead. Consider caching these services or retrieving them once in the constructor if the list of providers is static or changes infrequently.
| namespace Arc4u.Diagnostics.Logging; | ||
| internal class LoggerWrapperContext | ||
| { | ||
| private static IServiceScopeFactory? _scopeFactory; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider thread-safe initialization pattern
The static field _scopeFactory could benefit from a thread-safe initialization pattern to prevent potential race conditions in concurrent scenarios.
Consider using Lazy<T> for thread-safe initialization:
-private static IServiceScopeFactory? _scopeFactory;
+private static readonly Lazy<IServiceScopeFactory> _scopeFactory = new();Committable suggestion skipped: line range outside the PR's diff.
| public static LoggerWrapper<T> AddStackTrace<T>(this LoggerWrapper<T> logger) | ||
| { | ||
| logger.IncludeStackTrace = true; | ||
| return logger; | ||
| } |
There was a problem hiding this comment.
Add disposal check for consistency
The AddStackTrace method is missing the disposal check that other methods have.
public static LoggerWrapper<T> AddStackTrace<T>(this LoggerWrapper<T> logger)
{
+ logger.ThrowIfDisposed();
logger.IncludeStackTrace = true;
return logger;
}📝 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.
| public static LoggerWrapper<T> AddStackTrace<T>(this LoggerWrapper<T> logger) | |
| { | |
| logger.IncludeStackTrace = true; | |
| return logger; | |
| } | |
| public static LoggerWrapper<T> AddStackTrace<T>(this LoggerWrapper<T> logger) | |
| { | |
| logger.ThrowIfDisposed(); | |
| logger.IncludeStackTrace = true; | |
| return logger; | |
| } |
| public static LoggerWrapper<T> AddIfNotExist<T>(this LoggerWrapper<T> logger, string key, object? value) | ||
| { | ||
| if (value == null || logger.AdditionalFields.ContainsKey(key)) | ||
| { | ||
| return logger; | ||
| } | ||
|
|
||
| logger.AdditionalFields[ValidateKey(key)] = value; | ||
| return logger; | ||
| } |
There was a problem hiding this comment.
Add disposal check and optimize key validation
The AddIfNotExist method has several potential issues:
- Missing disposal check that other methods have
- Key validation happens after the null and existence checks
- Potential race condition between ContainsKey check and dictionary addition
Consider this safer implementation:
public static LoggerWrapper<T> AddIfNotExist<T>(this LoggerWrapper<T> logger, string key, object? value)
{
+ logger.ThrowIfDisposed();
+ var validKey = ValidateKey(key);
if (value == null || logger.AdditionalFields.ContainsKey(validKey))
{
return logger;
}
- logger.AdditionalFields[ValidateKey(key)] = value;
+ logger.AdditionalFields[validKey] = value;
return logger;
}📝 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.
| public static LoggerWrapper<T> AddIfNotExist<T>(this LoggerWrapper<T> logger, string key, object? value) | |
| { | |
| if (value == null || logger.AdditionalFields.ContainsKey(key)) | |
| { | |
| return logger; | |
| } | |
| logger.AdditionalFields[ValidateKey(key)] = value; | |
| return logger; | |
| } | |
| public static LoggerWrapper<T> AddIfNotExist<T>(this LoggerWrapper<T> logger, string key, object? value) | |
| { | |
| logger.ThrowIfDisposed(); | |
| var validKey = ValidateKey(key); | |
| if (value == null || logger.AdditionalFields.ContainsKey(validKey)) | |
| { | |
| return logger; | |
| } | |
| logger.AdditionalFields[validKey] = value; | |
| return logger; | |
| } |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/Arc4u.Diagnostics/Logging/LoggerWrapper.cs (3)
157-185: Simplify AddAdditionalProperties methodThe method has redundant checks and could be simplified for better maintainability.
private Dictionary<string, object?> AddAdditionalProperties() { - Dictionary<string, object?> properties; - try { - properties = new Dictionary<string, object?>(AdditionalFields); + var properties = new Dictionary<string, object?>(AdditionalFields); var definedProperties = _providers.Value.Select(x => x.GetProperties()).SelectMany(x => x); - if (definedProperties != null) - { - foreach (var property in definedProperties) - { - if (property.Value != null) - { - this.AddIfNotExist(property.Key, property.Value); - } - } - - return properties; - } - - return new Dictionary<string, object?>(AdditionalFields); + foreach (var property in definedProperties.Where(p => p.Value != null)) + { + properties.AddIfNotExist(property.Key, property.Value); + } + return properties; } catch (Exception ex) { _logger.LogWarning(ex, "Error getting property providers. Logging without additional properties."); return new Dictionary<string, object?>(AdditionalFields); } }
209-225: Add XML documentation for exception formatting methodsAdding XML documentation would improve maintainability and help other developers understand the formatting pattern.
+ /// <summary> + /// Formats an exception and its inner exceptions into a hierarchical string representation. + /// </summary> + /// <param name="exception">The exception to format.</param> + /// <returns>A formatted string containing the exception hierarchy.</returns> internal static string ToFormattedstring(this Exception exception)
9-9: Add XML documentation for public classPublic types should be documented according to .NET guidelines.
+/// <summary> +/// Provides structured logging capabilities with support for additional properties and exception formatting. +/// </summary> +/// <typeparam name="T">The type associated with the logger instance.</typeparam> public sealed class LoggerWrapper<T> : ILogger<T>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Arc4u.Diagnostics/Logging/LoggerWrapper.cs(1 hunks)src/Arc4u.Diagnostics/Logging/LoggerWrapperContext.cs(1 hunks)src/Arc4u.Diagnostics/Logging/LoggerWrapperExtensions.cs(1 hunks)src/Arc4u.Diagnostics/LoggingConstants.cs(1 hunks)src/Arc4u.sln(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Arc4u.Diagnostics/Logging/LoggerWrapperExtensions.cs
🔇 Additional comments (4)
src/Arc4u.Diagnostics/LoggingConstants.cs (1)
22-23: LGTM!
The new constant UnwrappedException follows the established naming convention and aligns with the structured logging objectives.
src/Arc4u.Diagnostics/Logging/LoggerWrapper.cs (1)
53-58: 🛠️ Refactor suggestion
Consider caching the scope during initialization
Creating a new scope for each provider retrieval could lead to resource leaks. Consider initializing providers during construction.
- _providers = new Lazy<IReadOnlyList<IAddPropertiesToLog>>(() =>
- {
- using var scope = LoggerWrapperContext.CreateScope();
- return scope.ServiceProvider.GetServices<IAddPropertiesToLog>().ToList();
- });
+ using var scope = LoggerWrapperContext.CreateScope();
+ _providers = new Lazy<IReadOnlyList<IAddPropertiesToLog>>(
+ () => scope.ServiceProvider.GetServices<IAddPropertiesToLog>().ToList());Likely invalid or redundant comment.
src/Arc4u.sln (2)
101-104: LGTM! Good solution organization.
The addition of "src" and "tests" solution folders improves the project structure by clearly separating source code from test code, following standard .NET solution organization practices.
963-1006: Project organization looks good, verify no orphaned projects.
The organization of projects under "src" and "tests" folders is logical and well-structured. Let's verify that all projects in the solution are properly organized.
✅ Verification successful
Project organization is correctly structured
All projects are properly organized in the solution structure:
- 39 projects are correctly placed under the "src" folder ({B9419F5B-9CF3-4247-9929-7AF2362FD8E3})
- 1 test project (Arc4u.UnitTest) is correctly placed under the "tests" folder ({E8E0B4BA-5659-40E6-94C8-CA6D9F339101})
- No orphaned projects found - all projects in the solution are assigned to a folder
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any projects that are not organized under solution folders
# Get all project GUIDs from the solution
echo "Projects in solution:"
rg -o '{[A-F0-9-]{36}}' src/Arc4u.sln | sort | uniq
echo -e "\nProjects organized in solution folders:"
rg -A 1 'NestedProjects = preSolution' -A 50 src/Arc4u.sln | rg '{[A-F0-9-]{36}}'
# Note: Compare the outputs manually to identify any projects that appear in the first list
# but not in the second list (these would be orphaned projects not organized in any folder)
Length of output: 874
Script:
#!/bin/bash
# Description: Check for any projects that are not organized under solution folders
# Get all project GUIDs from the solution
echo "Projects in solution:"
rg "\{[A-F0-9]{8}-[A-F0-9]{4}-[A-F0-9]{4}-[A-F0-9]{4}-[A-F0-9]{12}\}" src/Arc4u.sln | sort | uniq
echo -e "\nProjects organized in solution folders:"
rg "NestedProjects" -A 50 src/Arc4u.sln | rg "\{[A-F0-9]{8}-[A-F0-9]{4}-[A-F0-9]{4}-[A-F0-9]{4}-[A-F0-9]{12}\}"
Length of output: 77829
Script:
#!/bin/bash
# Get all project GUIDs and their organization
echo "Checking project organization..."
# Extract all project GUIDs from Project lines
echo "All projects in solution:"
rg "^Project.*= \"{.*\"" src/Arc4u.sln | cut -d'"' -f4
echo -e "\nProjects organized in folders:"
rg "^\t\t{.*} = {.*}$" src/Arc4u.sln
Length of output: 526
Script:
#!/bin/bash
# Get all project GUIDs and their organization
echo "Checking project organization..."
# Extract all project GUIDs from Project lines
echo "All projects in solution:"
rg "^Project.*= \".*\"" src/Arc4u.sln
echo -e "\nProjects organized in folders:"
rg "^\s+\{[A-F0-9-]+\}\s+=\s+\{[A-F0-9-]+\}" src/Arc4u.sln
Length of output: 11238
| if (_scopeFactory.IsValueCreated) | ||
| { | ||
| throw new InvalidOperationException("LoggerContext not initialized"); | ||
| } |
There was a problem hiding this comment.
Critical: Logic error in CreateScope method
The condition is reversed. The method throws when the factory IS created, but it should throw when it's NOT created.
Apply this fix:
- if (_scopeFactory.IsValueCreated)
+ if (!_scopeFactory.IsValueCreated)
{
throw new InvalidOperationException("LoggerContext not initialized");
}📝 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.
| if (_scopeFactory.IsValueCreated) | |
| { | |
| throw new InvalidOperationException("LoggerContext not initialized"); | |
| } | |
| if (!_scopeFactory.IsValueCreated) | |
| { | |
| throw new InvalidOperationException("LoggerContext not initialized"); | |
| } |
|
|
||
| public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func<TState, Exception?, string> formatter) | ||
| { | ||
| Console.WriteLine("Log<TState> is called."); |
There was a problem hiding this comment.
Remove debug Console.WriteLine statement
Debug statements should not be present in production code.
- Console.WriteLine("Log<TState> is called.");
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/Arc4u.Diagnostics/Logging/LoggerWrapper.cs (4)
18-18: Consider optimizing the Lazy initializationThe current initialization creates an empty list eagerly. Consider using a simpler initialization:
- private readonly Lazy<IReadOnlyList<IAddPropertiesToLog>> _providers = new(Enumerable.Empty<IAddPropertiesToLog>().ToList()); + private readonly Lazy<IReadOnlyList<IAddPropertiesToLog>> _providers = new();
23-36: Consider using a named constant for the fallback ProcessId valueDefine a constant for better maintainability and clarity of intent:
+ private const int FALLBACK_PROCESS_ID = -1; internal static int ProcessId { get { try { return Environment.ProcessId; } catch (PlatformNotSupportedException) { - return -1; + return FALLBACK_PROCESS_ID; } } }
114-120: Consider extracting timestamp format to configurationThe timestamp is currently hardcoded to UTC. Consider making the timestamp format configurable to support different timezone requirements:
+ private readonly DateTimeKind _timestampKind; var enrichedState = new { Caller = _caller, - TimeStamp = DateTime.UtcNow, + TimeStamp = _timestampKind == DateTimeKind.Utc ? DateTime.UtcNow : DateTime.Now, AdditionalFields = properties, Level = level };
204-243: Add XML documentation for exception handling utilitiesConsider adding XML documentation to improve maintainability:
+ /// <summary> + /// Provides utilities for formatting exceptions into human-readable strings. + /// </summary> internal static class DumpException { + /// <summary> + /// Formats an exception and its inner exceptions into a hierarchical string representation. + /// </summary> + /// <param name="exception">The exception to format.</param> + /// <returns>A formatted string containing the exception hierarchy.</returns> internal static string ToFormattedstring(this Exception exception)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Arc4u.Diagnostics/Logging/LoggerWrapper.cs(1 hunks)src/Arc4u.Diagnostics/Logging/LoggerWrapperContext.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Arc4u.Diagnostics/Logging/LoggerWrapperContext.cs
🔇 Additional comments (2)
src/Arc4u.Diagnostics/Logging/LoggerWrapper.cs (2)
38-83: Well-structured logging implementation!
The implementation follows Microsoft's recommended practices:
- Proper use of LoggerMessage.Define for high-performance logging
- Well-organized event IDs with clear categories
- Proper null validation in constructor
1-244: Verify test coverage for edge cases
While the implementation is solid, please ensure comprehensive test coverage for:
- Platform-specific ProcessId handling
- Exception formatting with deep hierarchies
- Memory usage with large numbers of additional properties
- Thread safety of the logging operations
|
@GFlisch any comments ? |
Move from Arc4u.Diagnostics to Arc4u.Diagnostics.Logger
Summary
This PR moves the logging implementation from the Arc4u.Diagnostics namespace to Arc4u.Diagnostics.Logger, providing significant performance improvements and better alignment with Microsoft.Extensions.Logging patterns.
Key Changes
Namespace Organization
Structured Logging Support
LoggerMessage.Define@operator)Example:
Performance Improvements
ILogger Integration
Breaking Changes
Migration Path
Benefits for Log Analysis
Future Improvements
Summary by CodeRabbit
Summary by CodeRabbit
New Features
UseArc4uLoggingmethods for both ASP.NET Core applications and host builders.Bug Fixes
Documentation
Chores