[ADMINAPI-1316] Fix log4net initialization, cross-platform compatibility, and consistent logging#363
Conversation
Co-authored-by: stephenfuqua <9324390+stephenfuqua@users.noreply.github.com>
| </appender> | ||
| <appender name="FileAppender" type="log4net.Appender.RollingFileAppender"> | ||
| <file value="${ProgramData}\Ed-Fi-ODS-AdminAPI\Log.txt" /> | ||
| <file value="logs/Ed-Fi-ODS-AdminAPI-Log.txt" /> |
There was a problem hiding this comment.
This is truly a better default option.
| var builder = WebApplication.CreateBuilder(args); | ||
|
|
||
| // Initialize log4net early so we can use it in Program.cs | ||
| var log4netConfigPath = Path.Combine(AppContext.BaseDirectory, "log4net", "log4net.config"); |
There was a problem hiding this comment.
@copilot the log4netConfigPath should be read from the appsettings file, which has Log4NetCore.Log4NetConfigFileName in it.
There was a problem hiding this comment.
Updated to read the log4net config path from Log4NetCore.Log4NetConfigFileName in appsettings.json instead of hardcoding it. The change ensures consistency with the existing configuration pattern used by the Microsoft.Extensions.Logging.Log4Net.AspNetCore integration. Commit: 0b49c6f
|
@copilot also change |
Co-authored-by: stephenfuqua <9324390+stephenfuqua@users.noreply.github.com>
Co-authored-by: stephenfuqua <9324390+stephenfuqua@users.noreply.github.com>
Updated RequestLoggingMiddleware to use log4net's |
| } | ||
|
|
||
| public async Task Invoke(HttpContext context, ILogger<RequestLoggingMiddleware> logger) | ||
| public async Task Invoke(HttpContext context) |
There was a problem hiding this comment.
This did not use dependency injection, but that's OK in this case, since we're not unit testing logging.
|
This output is much better! Still needs a developer to test the code change thoroughly locally - I only started it up and confirmed the root endpoint works. |
Test Results238 tests 237 ✅ 10s ⏱️ Results for commit 8d47e0b. |
Problem
The AdminAPI application was not using log4net properly, causing several issues:
Program.cs(e.g., "Starting Admin API", "Configuration - ApiMode: V2, Engine: SqlServer") were not appearing at application startupRoot Causes
Program.cs, so early logging calls were lostappsettings.jsonconfiguration used backslash path separators (log4net\\log4net.config) which don't work on Linux/UnixTraceAppenderinstead ofConsoleAppender, making console output invisibleSolution
This PR implements minimal, surgical fixes to resolve all identified issues:
1. Early Log4net Initialization
Added proper log4net initialization in
Program.csbefore any logging calls, reading the configuration path from appsettings:2. Cross-Platform Path Compatibility
Fixed path separator in
appsettings.jsonfromlog4net\\log4net.configtolog4net/log4net.configfor cross-platform compatibility.3. Visible Console Logging
Updated
log4net.configto useConsoleAppenderinstead ofTraceAppenderand changed the file path to a relative path that works across platforms.4. Configuration Consistency
Updated the early log4net initialization to read the configuration file path from the
Log4NetCore.Log4NetConfigFileNamesetting, ensuring consistency with the Microsoft.Extensions.Logging.Log4Net.AspNetCore integration.5. Consistent Log4net Usage
Updated
RequestLoggingMiddlewareto use log4net'sILoginterface instead of Microsoft.Extensions.LoggingILogger<T>, ensuring all logging in the application uses log4net consistently:Verification
Impact
This fix ensures that log4net works properly across all supported platforms while maintaining full backward compatibility. The application now has consistent logging throughout, with all components using log4net's ILog interface. The default configuration correctly logs to both console and file as intended, and early startup messages are visible for debugging and monitoring purposes.
Fixes #1316
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.