Fix LogLevel: CLI phase suppression and case-insensitive "Default" config key #3203
+206
−41
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,6 +59,13 @@ public class FileSystemRuntimeConfigLoader : RuntimeConfigLoader | |
| /// </summary> | ||
| private readonly IFileSystem _fileSystem; | ||
|
|
||
| /// <summary> | ||
| /// Logger used to log all the events that occur inside of FileSystemRuntimeConfigLoader | ||
| /// </summary> | ||
| private ILogger<FileSystemRuntimeConfigLoader>? _logger; | ||
|
|
||
| private StartupLogBuffer? _logBuffer; | ||
|
|
||
|
Comment on lines
+62
to
+68
There was a problem hiding this comment.
|
||
| public const string CONFIGFILE_NAME = "dab-config"; | ||
| public const string CONFIG_EXTENSION = ".json"; | ||
| public const string ENVIRONMENT_PREFIX = "DAB_"; | ||
|
|
@@ -82,13 +89,17 @@ public FileSystemRuntimeConfigLoader( | |
| HotReloadEventHandler<HotReloadEventArgs>? handler = null, | ||
| string baseConfigFilePath = DEFAULT_CONFIG_FILE_NAME, | ||
| string? connectionString = null, | ||
| bool isCliLoader = false, | ||
| ILogger<FileSystemRuntimeConfigLoader>? logger = null, | ||
| StartupLogBuffer? logBuffer = null) | ||
| : base(handler, connectionString) | ||
| { | ||
| _fileSystem = fileSystem; | ||
| _baseConfigFilePath = baseConfigFilePath; | ||
| ConfigFilePath = GetFinalConfigFilePath(); | ||
| _isCliLoader = isCliLoader; | ||
| _logger = logger; | ||
| _logBuffer = logBuffer; | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -196,7 +207,14 @@ public bool TryLoadConfig( | |
| { | ||
| if (_fileSystem.File.Exists(path)) | ||
| { | ||
| if (_logger is null) | ||
| { | ||
| _logBuffer?.BufferLog(LogLevel.Information, $"Loading config file from {_fileSystem.Path.GetFullPath(path)}."); | ||
| } | ||
| else | ||
| { | ||
| _logger?.LogInformation($"Loading config file from {_fileSystem.Path.GetFullPath(path)}."); | ||
| } | ||
|
|
||
| // Use File.ReadAllText because DAB doesn't need write access to the file | ||
| // and ensures the file handle is released immediately after reading. | ||
|
|
@@ -215,7 +233,15 @@ public bool TryLoadConfig( | |
| } | ||
| catch (IOException ex) | ||
| { | ||
| if (_logger is null) | ||
| { | ||
| _logBuffer?.BufferLog(LogLevel.Warning, $"IO Exception, retrying due to {ex.Message}"); | ||
| } | ||
| else | ||
| { | ||
| _logger?.LogWarning($"IO Exception, retrying due to {ex.Message}"); | ||
| } | ||
|
|
||
| if (runCount == FileUtilities.RunLimit) | ||
| { | ||
| throw; | ||
|
|
@@ -238,8 +264,14 @@ public bool TryLoadConfig( | |
| { | ||
| if (TrySetupConfigFileWatcher()) | ||
| { | ||
| if (_logger is null) | ||
| { | ||
| _logBuffer?.BufferLog(LogLevel.Information, $"Monitoring config: {ConfigFilePath} for hot-reloading."); | ||
| } | ||
| else | ||
| { | ||
| _logger?.LogInformation($"Monitoring config: {ConfigFilePath} for hot-reloading."); | ||
| } | ||
| } | ||
|
|
||
| // When isDevMode is not null it means we are in a hot-reload scenario, and need to save the previous | ||
|
|
@@ -249,13 +281,13 @@ public bool TryLoadConfig( | |
| // Log error when the mode is changed during hot-reload. | ||
| if (isDevMode != this.RuntimeConfig.IsDevelopmentMode()) | ||
| { | ||
| if (_logger is null) | ||
| { | ||
| _logBuffer?.BufferLog(LogLevel.Error, "Hot-reload doesn't support switching mode. Please restart the service to switch the mode."); | ||
| } | ||
| else | ||
| { | ||
| _logger?.LogError("Hot-reload doesn't support switching mode. Please restart the service to switch the mode."); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -281,15 +313,14 @@ public bool TryLoadConfig( | |
| return false; | ||
| } | ||
|
|
||
| string errorMessage = $"Unable to find config file: {path} does not exist."; | ||
| if (_logger is null) | ||
| { | ||
| _logBuffer?.BufferLog(LogLevel.Error, errorMessage); | ||
| } | ||
| else | ||
| { | ||
| _logger?.LogError(message: errorMessage); | ||
| } | ||
|
Comment on lines
+316
to
324
There was a problem hiding this comment. When the config file is missing, this now only buffers/logs the error and no longer writes to |
||
|
|
||
| config = null; | ||
|
|
@@ -516,4 +547,17 @@ public void UpdateConfigFilePath(string filePath) | |
| _baseConfigFilePath = filePath; | ||
| ConfigFilePath = filePath; | ||
| } | ||
|
|
||
| public void SetLogger(ILogger<FileSystemRuntimeConfigLoader> logger) | ||
| { | ||
| _logger = logger; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Flush all logs from the buffer after the log level is set from the RuntimeConfig. | ||
| /// </summary> | ||
| public void FlushLogBuffer() | ||
| { | ||
| _logBuffer?.FlushToLogger(_logger); | ||
| } | ||
| } | ||
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| using System.Collections.Concurrent; | ||
| using Microsoft.Extensions.Logging; | ||
|
|
||
| namespace Azure.DataApiBuilder.Config | ||
| { | ||
| /// <summary> | ||
| /// A general-purpose log buffer that stores log entries before the final log level is determined. | ||
| /// Can be used across different components during startup to capture important early logs. | ||
| /// </summary> | ||
| public class StartupLogBuffer | ||
| { | ||
| private readonly ConcurrentQueue<(LogLevel LogLevel, string Message)> _logBuffer; | ||
| private readonly object _flushLock = new(); | ||
|
|
||
| public StartupLogBuffer() | ||
| { | ||
| _logBuffer = new(); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Buffers a log entry with a specific category name. | ||
| /// </summary> | ||
| public void BufferLog(LogLevel logLevel, string message) | ||
| { | ||
| _logBuffer.Enqueue((logLevel, message)); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Flushes all buffered logs to a single target logger. | ||
| /// </summary> | ||
| public void FlushToLogger(ILogger? targetLogger) | ||
| { | ||
| lock (_flushLock) | ||
| { | ||
| while (_logBuffer.TryDequeue(out (LogLevel LogLevel, string Message) entry)) | ||
| { | ||
| targetLogger?.Log(entry.LogLevel, message: entry.Message); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ namespace Azure.DataApiBuilder.Service | |
| public class Program | ||
| { | ||
| public static bool IsHttpsRedirectionDisabled { get; private set; } | ||
| public static DynamicLogLevelProvider LogLevelProvider = new(); | ||
|
|
||
|
Comment on lines
35
to
37
There was a problem hiding this comment.
|
||
| public static void Main(string[] args) | ||
| { | ||
|
|
@@ -59,7 +60,6 @@ public static void Main(string[] args) | |
|
|
||
| public static bool StartEngine(string[] args, bool runMcpStdio, string? mcpRole) | ||
| { | ||
| try | ||
| { | ||
| IHost host = CreateHostBuilder(args, runMcpStdio, mcpRole).Build(); | ||
|
|
@@ -107,9 +107,19 @@ public static IHostBuilder CreateHostBuilder(string[] args, bool runMcpStdio, st | |
| McpStdioHelper.ConfigureMcpStdio(builder, mcpRole); | ||
| } | ||
| }) | ||
| .ConfigureServices((context, services) => | ||
| { | ||
| services.AddSingleton(LogLevelProvider); | ||
| }) | ||
| .ConfigureLogging(logging => | ||
| { | ||
| logging.AddFilter("Microsoft", logLevel => LogLevelProvider.ShouldLog(logLevel)); | ||
| logging.AddFilter("Microsoft.Hosting.Lifetime", logLevel => LogLevelProvider.ShouldLog(logLevel)); | ||
| }) | ||
| .ConfigureWebHostDefaults(webBuilder => | ||
| { | ||
| Startup.MinimumLogLevel = GetLogLevelFromCommandLineArgs(args, out Startup.IsLogLevelOverriddenByCli); | ||
| LogLevelProvider.SetInitialLogLevel(Startup.MinimumLogLevel, Startup.IsLogLevelOverriddenByCli); | ||
| ILoggerFactory loggerFactory = GetLoggerFactoryForLogLevel(Startup.MinimumLogLevel, stdio: runMcpStdio); | ||
| ILogger<Startup> startupLogger = loggerFactory.CreateLogger<Startup>(); | ||
| DisableHttpsRedirectionIfNeeded(args); | ||
|
|
@@ -185,9 +195,9 @@ public static ILoggerFactory GetLoggerFactoryForLogLevel( | |
| // "Azure.DataApiBuilder.Service" | ||
| if (logLevelInitializer is null) | ||
| { | ||
| builder.AddFilter(category: "Microsoft", logLevel => LogLevelProvider.ShouldLog(logLevel)); | ||
| builder.AddFilter(category: "Azure", logLevel => LogLevelProvider.ShouldLog(logLevel)); | ||
| builder.AddFilter(category: "Default", logLevel => LogLevelProvider.ShouldLog(logLevel)); | ||
| } | ||
| else | ||
| { | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test asserts
Assert.IsFalse(process.HasExited)immediately after starting the process, which is race-prone: a failing process can still be running at the exact moment of the check, and a slow-starting process might not have fully initialized yet. Consider adding a small wait/poll window (e.g., wait a short duration and then assert it's still running, or wait for a known readiness signal when available) to make the test deterministic.