Skip to content

Fix LogLevel: CLI phase suppression and case-insensitive "Default" config key#3203

Open
Copilot wants to merge 10 commits intomainfrom
copilot/fix-loglevel-incorrect-behavior
Open

Fix LogLevel: CLI phase suppression and case-insensitive "Default" config key#3203
Copilot wants to merge 10 commits intomainfrom
copilot/fix-loglevel-incorrect-behavior

Conversation

Copy link
Contributor

Copilot AI commented Mar 7, 2026

Why make this change?

Two LogLevel bugs: Using the None LogLevel still emits some Information messages (version, config path, etc.), and using "Default" (capital D) as a key in telemetry.log-level config crashes startup with NotSupportedException.

What is this change?

CLI logger respects --LogLevel

  • CustomConsoleLogger had a hardcoded _minimumLogLevel = LogLevel.Information, making it ignore the --LogLevel flag entirely.
  • Added LogLevel parameter to CustomLoggerProvider / CustomConsoleLogger.
  • Added Program.PreParseLogLevel() — scans raw args[] for --LogLevel before CommandLine.Parser runs — so the logger factory is configured at the right level before any CLI-phase output is emitted.
  • Updated Utils.GetLoggerFactoryForCli(LogLevel) to wire this through.

Case-insensitive "Default" key in config

  • IsLoggerFilterValid used string.Equals (ordinal), so "Default" failed against the registered "default" filter.
  • GetConfiguredLogLevel used TryGetValue("default") (case-sensitive), silently ignoring "Default" keys.
  • Both fixed with StringComparison.OrdinalIgnoreCase / LINQ FirstOrDefault.
// Now works — previously threw NotSupportedException
"telemetry": {
  "log-level": {
    "Default": "warning"
  }
}
# Now silent — previously emitted "Information: Microsoft.DataApiBuilder ..."
dab start --LogLevel None

How was this tested?

  • Integration Tests
  • Unit Tests
    • TestEngineStartUpWithHighLogLevelOptions (new): Warning/Error/Critical/None — asserts process starts without checking stdout.
    • ValidStringLogLevelFilters: added "Default" (capital D) data row to cover the case-insensitive validation fix.

Sample Request(s)

# Suppress all output
dab start --LogLevel None

# Config key now case-insensitive
# dab-config.json:
# "telemetry": { "log-level": { "Default": "none" } }
dab start

…ive Default key

Co-authored-by: RubenCerna2079 <32799214+RubenCerna2079@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix incorrect behavior of LogLevel Fix LogLevel: CLI phase suppression and case-insensitive "Default" config key Mar 7, 2026
@RubenCerna2079 RubenCerna2079 linked an issue Mar 9, 2026 that may be closed by this pull request
@Aniruddh25 Aniruddh25 added the 2.0 label Mar 10, 2026
@RubenCerna2079
Copy link
Contributor

/azp run

@RubenCerna2079 RubenCerna2079 marked this pull request as ready for review March 12, 2026 00:24
Copilot AI review requested due to automatic review settings March 12, 2026 00:24
@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Addresses two logging-related startup issues: (1) honoring --LogLevel during early/CLI-phase output, and (2) treating the telemetry.log-level config key "default" case-insensitively so "Default" doesn’t crash startup.

Changes:

  • Updates runtime-config log-level validation and lookup to be case-insensitive for "default"/"Default".
  • Refactors service startup/config-loader logging to route messages through logging infrastructure (with buffering intended before final log level is known).
  • Adjusts CLI E2E tests around --LogLevel behavior, splitting Information-and-below vs Warning-and-above scenarios.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
src/Service/Startup.cs Adds log buffering + runtime log-level update wiring during startup (and config-loader logger wiring).
src/Service/Program.cs Adds dynamic filtering hooks intended to suppress early messages based on effective log level.
src/Config/FileSystemRuntimeConfigLoader.cs Replaces direct console writes with logger/buffered logging and adds logger setter/flush.
src/Config/ObjectModel/RuntimeConfig.cs Makes "default" log-level lookup case-insensitive in GetConfiguredLogLevel.
src/Core/Configurations/RuntimeConfigValidator.cs Makes logger-filter validation case-insensitive.
src/Service.Tests/Configuration/ConfigurationTests.cs Adds a test row covering "Default" log-level key validation.
src/Cli/Program.cs Keeps CLI logger factory usage and loader creation (but currently still doesn’t wire args into log level).
src/Cli.Tests/EndToEndTests.cs Updates E2E coverage for --LogLevel expectations and adds a Warning+ startup test.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines 35 to 37
public static bool IsHttpsRedirectionDisabled { get; private set; }
public static DynamicLogLevelProvider LogLevelProvider = new();

Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DynamicLogLevelProvider is referenced/instantiated here, but the type does not exist in the repo (no matches found). As-is, this change will fail compilation; the PR likely needs to include the new provider implementation (or remove the reference).

Copilot uses AI. Check for mistakes.
Comment on lines +62 to +68
/// <summary>
/// Logger used to log all the events that occur inside of FileSystemRuntimeConfigLoader
/// </summary>
private ILogger<FileSystemRuntimeConfigLoader>? _logger;

private StartupLogBuffer? _logBuffer;

Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StartupLogBuffer is introduced as a dependency here, but there is no implementation for this type in the repo. This will fail compilation unless the buffer class is added (and ideally unit-tested for thread safety since config loading/hot-reload can be concurrent).

Copilot uses AI. Check for mistakes.
Comment on lines +316 to 324
string errorMessage = $"Unable to find config file: {path} does not exist.";
if (_logger is null)
{
string errorMessage = $"Unable to find config file: {path} does not exist.";
Console.Error.WriteLine(errorMessage);
_logBuffer?.BufferLog(LogLevel.Error, errorMessage);
}
else
{
string errorMessage = "Unable to find config file: {path} does not exist.";
logger.LogError(message: errorMessage, path);
_logger?.LogError(message: errorMessage);
}
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the config file is missing, this now only buffers/logs the error and no longer writes to Console.Error. If _logger is null and no buffer is provided (e.g. CLI loader construction currently passes neither), the user may get no error output at all. Consider falling back to Console.Error.WriteLine(...) when both _logger and _logBuffer are null, so failures are always visible.

Copilot uses AI. Check for mistakes.
Comment on lines 774 to 792
foreach (KeyValuePair<string, LogLevel?> logger in Runtime!.Telemetry!.LoggerLevel!)
{
// Checks if the new key that is valid has more priority than the current key
if (logger.Key.Length > max && loggerFilter.StartsWith(logger.Key))
{
max = logger.Key.Length;
currentFilter = logger.Key;
}
}

Runtime!.Telemetry!.LoggerLevel!.TryGetValue(currentFilter, out LogLevel? value);
if (value is not null)
{
return (LogLevel)value;
}

Runtime!.Telemetry!.LoggerLevel!.TryGetValue("default", out value);
value = Runtime!.Telemetry!.LoggerLevel!
.FirstOrDefault(kvp => kvp.Key.Equals("default", StringComparison.OrdinalIgnoreCase)).Value;
if (value is not null)
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IsLoggerFilterValid now accepts filter keys case-insensitively, and the default lookup below is case-insensitive, but the earlier loggerFilter.StartsWith(logger.Key) match is still case-sensitive. That can cause valid config keys like "azure" or "Azure.DataApiBuilder" (with different casing) to never match and fall back to default. Consider using StartsWith(..., StringComparison.OrdinalIgnoreCase) for consistency with validation/default lookup.

Copilot uses AI. Check for mistakes.
Comment on lines +707 to +708
DynamicLogLevelProvider logLevelProvider = app.ApplicationServices.GetRequiredService<DynamicLogLevelProvider>();
logLevelProvider.UpdateFromRuntimeConfig(runtimeConfig);
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DynamicLogLevelProvider is used here, but there is no definition for it anywhere in the repository (searching for the type returns no matches). This will not compile unless the provider type is added to the PR or the code is updated to use an existing log-level component.

Suggested change
DynamicLogLevelProvider logLevelProvider = app.ApplicationServices.GetRequiredService<DynamicLogLevelProvider>();
logLevelProvider.UpdateFromRuntimeConfig(runtimeConfig);

Copilot uses AI. Check for mistakes.
Comment on lines 201 to 264
public bool TryLoadConfig(
string path,
[NotNullWhen(true)] out RuntimeConfig? config,
ILogger? logger = null,
bool? isDevMode = null,
DeserializationVariableReplacementSettings? replacementSettings = null)
{
if (_fileSystem.File.Exists(path))
{
Console.WriteLine($"Loading config file from {_fileSystem.Path.GetFullPath(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.
// Previous usage of File.Open may cause file locking issues when
// actively using hot-reload and modifying the config file in a text editor.
// Includes an exponential back-off retry mechanism to accommodate
// circumstances where the file may be in use by another process.
int runCount = 1;
string json = string.Empty;
while (runCount <= FileUtilities.RunLimit)
{
try
{
json = _fileSystem.File.ReadAllText(path);
break;
}
catch (IOException ex)
{
Console.WriteLine($"IO Exception, retrying due to {ex.Message}");
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;
}

Thread.Sleep(TimeSpan.FromSeconds(Math.Pow(FileUtilities.ExponentialRetryBase, runCount)));
runCount++;
}
}

// Use default replacement settings if none provided
replacementSettings ??= new DeserializationVariableReplacementSettings();

if (!string.IsNullOrEmpty(json) && TryParseConfig(
json,
out RuntimeConfig,
replacementSettings,
logger: null,
connectionString: _connectionString))
{
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TryLoadConfig still accepts an optional ILogger logger parameter, but the method no longer uses it (all logging uses the _logger field instead). This breaks existing call sites (e.g. hot-reload passes a logger) and also forces TryParseConfig(..., logger: null) which changes where parse failures are emitted. Consider using an effectiveLogger = logger ?? _logger for all log writes (and pass it through to TryParseConfig), or remove the parameter and update all callers accordingly.

Copilot uses AI. Check for mistakes.
Comment on lines 29 to 40
// Logger setup and configuration
ILoggerFactory loggerFactory = Utils.LoggerFactoryForCli;
ILogger<Program> cliLogger = loggerFactory.CreateLogger<Program>();
ILogger<ConfigGenerator> configGeneratorLogger = loggerFactory.CreateLogger<ConfigGenerator>();
ILogger<Utils> cliUtilsLogger = loggerFactory.CreateLogger<Utils>();
ConfigGenerator.SetLoggerForCliConfigGenerator(configGeneratorLogger);
Utils.SetCliUtilsLogger(cliUtilsLogger);

// Sets up the filesystem used for reading and writing runtime configuration files.
IFileSystem fileSystem = new FileSystem();
FileSystemRuntimeConfigLoader loader = new(fileSystem, handler: null, isCliLoader: true);

FileSystemRuntimeConfigLoader loader = new(fileSystem, handler: null, isCliLoader: true);
return Execute(args, cliLogger, fileSystem, loader);
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description says CLI-phase logging now respects --LogLevel via pre-parsing args and wiring the level into CustomConsoleLogger, but this code still creates the logger factory via Utils.LoggerFactoryForCli (which currently does not take args and uses CustomLoggerProvider with a hardcoded minimum of Information). As written, dab start --LogLevel None will still emit CLI-phase Information messages. Either update the CLI logger factory initialization to depend on a pre-parsed log level (or update CustomLoggerProvider accordingly), or adjust the PR description/tests.

Copilot uses AI. Check for mistakes.
IFileSystem fileSystem = new FileSystem();
FileSystemRuntimeConfigLoader loader = new(fileSystem, handler: null, isCliLoader: true);

FileSystemRuntimeConfigLoader loader = new(fileSystem, handler: null, isCliLoader: true);
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is trailing whitespace at the end of this line (...isCliLoader: true); ). This can cause formatting checks to fail (and is typically auto-fixed by dotnet format).

Suggested change
FileSystemRuntimeConfigLoader loader = new(fileSystem, handler: null, isCliLoader: true);
FileSystemRuntimeConfigLoader loader = new(fileSystem, handler: null, isCliLoader: true);

Copilot uses AI. Check for mistakes.
Comment on lines +878 to +879
// Verify the engine started (process not immediately exited) then clean up.
Assert.IsFalse(process.HasExited);
Copy link

Copilot AI Mar 12, 2026

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.

Suggested change
// Verify the engine started (process not immediately exited) then clean up.
Assert.IsFalse(process.HasExited);
// Verify the engine did not exit within a short timeout, then clean up.
bool exitedWithinTimeout = process.WaitForExit(1000);
Assert.IsFalse(exitedWithinTimeout);

Copilot uses AI. Check for mistakes.
public static AzureLogAnalyticsOptions AzureLogAnalyticsOptions = new();
public static FileSinkOptions FileSinkOptions = new();
public const string NO_HTTPS_REDIRECT_FLAG = "--no-https-redirect";
private StartupLogBuffer _logBuffer = new();
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StartupLogBuffer is referenced here but no such type exists in the repo (searching for the class name returns no matches). This will fail the build unless the new type is added (or the code updated to use an existing buffer/logging mechanism).

Suggested change
private StartupLogBuffer _logBuffer = new();

Copilot uses AI. Check for mistakes.
…l-incorrect-behavior' into copilot/fix-loglevel-incorrect-behavior
@RubenCerna2079
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@RubenCerna2079
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@RubenCerna2079
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: LogLevel incorrect behavior and failing

4 participants