Fix CLI overrides LogLevel value from config file#3426
Fix CLI overrides LogLevel value from config file#3426RubenCerna2079 wants to merge 5 commits intomainfrom
Conversation
RubenCerna2079
requested review from
Alekhya-Polavarapu,
Aniruddh25,
JerryNixon,
aaronburtle,
anushakolan,
sourabh1007,
souvikghosh04 and
vadeveka
as code owners
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes dab start so the CLI only overrides the minimum LogLevel when --LogLevel is explicitly provided, allowing config-file log levels to take effect otherwise.
Changes:
- Move
--LogLevelargument injection to only occur whenStartOptions.LogLevelis set. - Improve logging to differentiate whether the log level came from config’s
defaultnamespace vs host-mode fallback. - Add an end-to-end test validating
Startup.IsLogLevelOverriddenByClibehavior with/without the--LogLevelflag.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Cli/ConfigGenerator.cs | Stops always appending --LogLevel args; adds logic to detect config-provided default log level for logging. |
| src/Cli.Tests/EndToEndTests.cs | Adds an E2E test for CLI log level override behavior by asserting Startup.IsLogLevelOverriddenByCli. |
| .SingleOrDefault(kvp => kvp.Key.Equals("default", StringComparison.OrdinalIgnoreCase)).Value != null; | ||
|
|
||
| _logger.LogInformation($"Setting default minimum LogLevel: {minimumLogLevel} for {hostModeType} mode.", minimumLogLevel, hostModeType); | ||
| // Check if we set the minimum LogLevel from the config file or if it is based on the host mode. |
There was a problem hiding this comment.
we are not using the hostModeType at all to make the decision of what the level should be set as
| { | ||
| minimumLogLevel = deserializedRuntimeConfig.GetConfiguredLogLevel(); | ||
| HostMode hostModeType = deserializedRuntimeConfig.IsDevelopmentMode() ? HostMode.Development : HostMode.Production; | ||
| bool isValueFromDefault = !deserializedRuntimeConfig.IsLogLevelNull() && deserializedRuntimeConfig.Runtime!.Telemetry!.LoggerLevel! |
There was a problem hiding this comment.
how is the function GetConfiguredLogLevel getting the minimumLogLevel?
isnt it from the config file?
if it is obtained in a similar manner, isValueFromDefault should be a by product of the GetConfiguredLevel function.
| } | ||
| else | ||
| { | ||
| _logger.LogInformation("Setting default minimum LogLevel: {minimumLogLevel} for {hostModeType} mode.", minimumLogLevel, hostModeType); |
There was a problem hiding this comment.
What are we doing in the else part to set the minimum log level? We do a log message here but what is setting the loglevel in the LogLevel provider?
| }, | ||
| ""telemetry"": { | ||
| ""log-level"": { | ||
| ""default"": ""Warning"" |
There was a problem hiding this comment.
Have you tested whether different namespaces within DAB honor the different log levels? when the loglevel from CLI is not overriding the level?
This config is only testing the default namespace scenario,
|
|
||
| _logger.LogInformation($"Setting default minimum LogLevel: {minimumLogLevel} for {hostModeType} mode.", minimumLogLevel, hostModeType); | ||
| // Check if we set the minimum LogLevel from the config file or if it is based on the host mode. | ||
| if (isValueFromDefault) |
There was a problem hiding this comment.
what is the use of figuring out isValueFromDefault?
Aniruddh25
left a comment
Aniruddh25
left a comment
There was a problem hiding this comment.
Needs answers to some questions, and more testing
github-project-automation
bot
moved this from Todo
to Review In Progress
in Data API builder
Why make this change?
Whenever we use the
dab startcommand the CLI overrides the minimum LogLevel from the configuration file. This is acceptable only if we use the--LogLevelflag. If we do not use that flag, the LogLevel for each namespace should be decided by the configuration file.What is this change?
This change moves the
args.Addmethods inside theConfigGenerator.csso that they are only applied when we use the--LogLevelflag. Having these arguments causes DAB to not allow any changes to the LogLevel in the loggers, since those arguments were always being added, DAB always assumed the CLI was overriding the LogLevel when it was not expect.How was this tested?
Sample Request(s)
dab start --LogLevel information
dab start (With config file default value debug)