Migrate to Microsoft.Extensions.Logging#1968
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request migrates ImageBuilder from a custom ad-hoc logging system (ILoggerService/Logger) to Microsoft.Extensions.Logging as part 1/2 of issue #1908 to support structured logging. The migration replaces all instances of ILoggerService with ILogger from Microsoft.Extensions.Logging, removes the custom Logger and LoggerService classes, and introduces StandaloneLoggerFactory for static code paths that cannot use dependency injection.
Changes:
- Replaced custom ILoggerService interface and implementation with Microsoft.Extensions.Logging.ILogger
- Added StandaloneLoggerFactory for static contexts where DI isn't available
- Updated all services, commands, and utilities to accept ILogger via constructor injection
- Migrated all logging calls from custom methods (WriteMessage, WriteError, WriteHeading) to standard ILogger methods (LogInformation, LogError)
- Updated test mocks from ILoggerService to ILogger
Reviewed changes
Copilot reviewed 80 out of 80 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| GlobalUsings.cs | Adds global using for Microsoft.Extensions.Logging |
| StandaloneLoggerFactory.cs | New factory providing logger access for static code paths |
| ImageBuilder.cs | Removes ILoggerService service registration |
| Logger.cs, LoggerService.cs, ILoggerService.cs | Removed custom logging infrastructure |
| RetryHelper.cs | Updated to use ILogger instead of ILoggerService |
| Program.cs | Enhanced error handling using ILoggerFactory |
| Services/*.cs | Migrated all services to use ILogger or ILoggerFactory |
| Commands/*.cs | Migrated all commands to use ILogger |
| Tests/*.cs | Updated all test mocks from ILoggerService to ILogger |
| } | ||
|
|
||
| protected ILoggerService LoggerService { get; } | ||
| protected ILogger LoggerService { get; } |
There was a problem hiding this comment.
According to the PR TODO, protected loggers should be changed to private. This protected ILogger LoggerService property should be private readonly ILogger _logger to follow the convention established in other migrated classes. Additionally, the property name should match the constructor parameter type (ILogger) rather than resembling the old ILoggerService.
| private readonly IDockerService _dockerService; | ||
| private readonly ILoggerService _loggerService; | ||
| private readonly ILogger<BuildCommand> _logger; | ||
| private ILogger Logger => _logger; |
There was a problem hiding this comment.
This property wrapper private ILogger Logger => _logger; is unnecessary. The _logger field can be used directly throughout the class, which would be more consistent with how other classes in this migration use loggers. This adds an extra layer of indirection without clear benefit.
| ILogger<MarImageIngestionReporter> logger, | ||
| ILoggerFactory loggerFactory, |
There was a problem hiding this comment.
The constructor accepts an unused logger parameter but never assigns it. The _loggerFactory field is used instead to create loggers later. Either remove the unused logger parameter or use it if it's needed.
| { | ||
| loggerService.WriteError(exception.ToString()); | ||
| LogRetryMessage(loggerService, timeToNextRetry, retryCount, maxRetries); | ||
| logger.LogError(exception.ToString()); |
There was a problem hiding this comment.
The LogError calls should use the structured logging overload that accepts an exception as the first parameter. Instead of logger.LogError(exception.ToString()), use logger.LogError(exception, "message") to properly capture exception information in structured logs.
| catch (InvalidOperationException ex) | ||
| { | ||
| loggerService.WriteError($"Failed to annotate EOL for digest '{digest}': {ex.Message}"); | ||
| logger.LogError($"Failed to annotate EOL for digest '{digest}': {ex.Message}"); |
There was a problem hiding this comment.
The LogError calls should use the structured logging overload that accepts an exception as the first parameter. Instead of logger.LogError($"Failed to annotate EOL for digest '{digest}': {ex.Message}"), use logger.LogError(ex, $"Failed to annotate EOL for digest '{digest}'") to properly capture exception information in structured logs.
| Logger.LogError($""" | ||
| Template parsing error in file {templatePath}:{startLine},{startColumn},{endLine},{endColumn} | ||
| Message: {e.Message} |
There was a problem hiding this comment.
The LogError call should use the structured logging overload that accepts an exception as the first parameter. Change this to use a proper structured logging pattern that captures the exception object rather than converting it to a string.
| private readonly IImageCacheService _imageCacheService; | ||
| private readonly ILoggerService _loggerService; | ||
| private readonly ILogger<GenerateBuildMatrixCommand> _logger; | ||
| private ILogger Logger => _logger; |
There was a problem hiding this comment.
This property wrapper private ILogger Logger => _logger; is unnecessary. The _logger field can be used directly throughout the class, which would be more consistent with how other classes in this migration use loggers. This adds an extra layer of indirection without clear benefit.
| string errorMsg = $"Importing Failure: {formattedDestinationImages}"; | ||
| errorMsg += Environment.NewLine + e.ToString(); | ||
|
|
||
| _logger.LogInformation(errorMsg); |
There was a problem hiding this comment.
The error message is being logged with LogInformation instead of LogError. This should be _logger.LogError(errorMsg) or better yet, use the exception overload: _logger.LogError(e, "Importing Failure: {DestinationImages}", formattedDestinationImages).
| private readonly ICopyImageService _copyImageService = copyImageService; | ||
|
|
||
| protected ILoggerService LoggerService { get; } = loggerService; | ||
| protected ILogger LoggerService { get; } = logger; |
There was a problem hiding this comment.
According to the PR TODO, protected loggers should be changed to private. This protected ILogger LoggerService property should be private readonly ILogger _logger to follow the convention established in other migrated classes. Additionally, the property name should match the constructor parameter type (ILogger) rather than resembling the old ILoggerService.
| private TimeSpan _artifactGenerationTime = TimeSpan.Zero; | ||
|
|
||
| protected GenerateArtifactsCommand(IEnvironmentService environmentService) : base() | ||
| protected ILogger Logger { get; } |
There was a problem hiding this comment.
According to the PR TODO, protected loggers should be changed to private. This protected ILogger Logger property should be private readonly ILogger _logger to follow the convention established in other migrated classes and to adhere to the least-exposure rule.
mthalman
left a comment
There was a problem hiding this comment.
Also follow up on Copilot comments.
Part 1/2 of #1908 - Migrate to M.E.Logging.
ILogger<T> loggerparameters and have a private_loggerfield.Everything is [Info] level right now. We can adjust later as we go.
TODO (before merging):