Change CLI flag --LogLevel to --log-level#3653
Conversation
RubenCerna2079
requested review from
Alekhya-Polavarapu,
Aniruddh25,
JerryNixon,
aaronburtle,
anushakolan,
rusamant,
sourabh1007,
souvikghosh04,
stuartpa and
vadeveka
as code owners
There was a problem hiding this comment.
Pull request overview
This PR standardizes the dab start logging flag to --log-level to align with the CLI’s kebab-case conventions, while retaining the legacy --LogLevel as a deprecated alias and updating internal documentation/tests accordingly.
Changes:
- Renamed the
dab startflag from--LogLevelto--log-leveland introduced a hidden legacy option (--LogLevel) with a deprecation warning. - Updated CLI/service log-level plumbing to pass
--log-levelthrough to the engine and refreshed comments/docs to match. - Updated unit/end-to-end tests to use
--log-level, adding coverage for the legacy flag still functioning.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Service/Startup.cs | Updates comments to reference the new --log-level startup argument. |
| src/Service/Program.cs | Updates service-side CLI arg parsing/docs to use --log-level for log level initialization. |
| src/Service.Tests/UnitTests/DynamicLogLevelProviderTests.cs | Updates test docs to reference --log-level. |
| src/Core/Telemetry/ILogLevelController.cs | Updates interface docs for precedence to reference --log-level. |
| src/Cli/Utils.cs | Updates CLI state/docs to reference --log-level. |
| src/Cli/Properties/launchSettings.json | Modifies CLI debug profile arguments (currently includes a machine-specific path). |
| src/Cli/Program.cs | Updates early flag parsing to detect --log-level before logger creation. |
| src/Cli/Exporter.cs | Updates StartOptions construction to include new legacy parameter. |
| src/Cli/CustomLoggerProvider.cs | Updates comments to reference --log-level behavior in MCP stdio mode. |
| src/Cli/ConfigGenerator.cs | Implements legacy --LogLevel handling with a deprecation warning and forwards --log-level to the engine. |
| src/Cli/Commands/StartOptions.cs | Adds --log-level option and a hidden legacy --LogLevel option. |
| src/Cli.Tests/EndToEndTests.cs | Updates E2E tests to use --log-level and adds legacy --LogLevel cases. |
| src/Cli.Tests/CustomLoggerTests.cs | Updates test docs to reference --log-level. |
| src/Azure.DataApiBuilder.Mcp/Core/McpStdioServer.cs | Updates MCP docs/comments to reference --log-level. |
Comments suppressed due to low confidence (1)
src/Cli/Program.cs:67
- ParseEarlyFlags only detects the new
--log-levelflag. If a user uses the legacy--LogLevelflag (which this PR intends to keep working), the early logging state (Utils.IsCliOverriding/Utils.CliLogLevel) won’t be set, which can keep CLI logs suppressed in MCP stdio mode despite the user explicitly opting in to logging.
else if (string.Equals(arg, "--log-level", StringComparison.OrdinalIgnoreCase) && i + 1 < args.Length)
{
Utils.IsCliOverriding = true;
if (Enum.TryParse<LogLevel>(args[i + 1], ignoreCase: true, out LogLevel cliLogLevel))
{
…cli-loglevel-flag' into dev/rubencerna/fix-cli-loglevel-flag
RubenCerna2079
changed the title
souvikghosh04
left a comment
souvikghosh04
left a comment
There was a problem hiding this comment.
Posting comments so far. rethink if we really need the legacy flag or should error out on unsupported format as we generally do and document these.
| int logLevelIndex = Array.FindIndex(args, a => | ||
| string.Equals(a, "--log-level", StringComparison.OrdinalIgnoreCase) || | ||
| string.Equals(a, "--LogLevel", StringComparison.OrdinalIgnoreCase)); |
There was a problem hiding this comment.
The deprecation warning only fires through the dab start's ConfigGenerator path. When the service host is invoked directly (e.g., dotnet Azure.DataApiBuilder.Service.dll --LogLevel Information, which is what containers and Aspire scenarios do), the legacy flag works but the user is never told it's deprecated. Either emit the same warning here, or document explicitly that the service host will keep both spellings indefinitely. Additionally, I also have commented about do we really need to handle deprecated scenario since the unsupported format ideally should error out and document in new release.
| public LogBuffer CliBuffer { get; } | ||
|
|
||
| public StartOptions(bool verbose, LogLevel? logLevel, bool isHttpsRedirectionDisabled, bool mcpStdio, string? mcpRole, string config) | ||
| public StartOptions(bool verbose, LogLevel? logLevel, LogLevel? logLevelLegacy, bool isHttpsRedirectionDisabled, bool mcpStdio, string? mcpRole, string config) |
There was a problem hiding this comment.
New parameters should be added at the end to avoid any existing callers breaking the sequenece of intended params passed in earlier format.
Why make this change?
--LogLevelshould be--loglevel(and case insensitive) like all the other CLI flags #3257What is this change?
We changed the
dab start--LogLevelflag to--log-levelin order to have it follow the conventions we already have. We also allow for the CLI to still use the previous--LogLevelflag but add a warning sign to ensure the user knows it is deprecated. Lastly, also changed all of the comments that mention the previous--LogLeveland updated it to use the new--log-level.How was this tested?
Changed the tests that were for
--LogLevelso they now use--log-leveland added a few extra cases to ensure the--LogLevelflag still worksSample Request(s)
dab start --log-level information
dab start --LogLevel warning