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 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.
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.
…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).

Copy link
Contributor

@JerryNixon JerryNixon left a comment

Choose a reason for hiding this comment

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

Good pragmatic fix: it aligns user expectations (--LogLevel None should be quiet) and makes config more forgiving (Default vs default). The approach (buffer early logs + introduce a provider for dynamic filtering) is a bit more involved than a one-liner, but it looks coherent and test-backed, and it removes the remaining Console.WriteLine that was bypassing log level entirely.


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

Choose a reason for hiding this comment

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

Consider SingleOrDefault() for the strange but possible scenario where Default and default are both in the configuration. Single ensures there is only one, while OrDefault allows it to be missing.

public void UpdateFromRuntimeConfig(RuntimeConfig runtimeConfig)
{
// Only update if CLI didn't override
if (!IsCliOverridden)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If Cli overrides, when do we set the CurrentL:ogLevel with the value that the CLI provides?

if (runtimeConfigProvider.TryGetConfig(out RuntimeConfig? runtimeConfig))
{
// Configure Application Insights Telemetry
// Configure Telemetry
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this configuring telemetry?

DynamicLogLevelProvider logLevelProvider = app.ApplicationServices.GetRequiredService<DynamicLogLevelProvider>();
logLevelProvider.UpdateFromRuntimeConfig(runtimeConfig);
FileSystemRuntimeConfigLoader configLoader = app.ApplicationServices.GetRequiredService<FileSystemRuntimeConfigLoader>();
configLoader.SetLogger(app.ApplicationServices.GetRequiredService<ILogger<FileSystemRuntimeConfigLoader>>());
Copy link
Collaborator

Choose a reason for hiding this comment

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

How are the effects of DynamicLogLevelProvider setting the log level consumed by the FileSystemRuntimeConfigLoader's Logger?

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

6 participants