Cherry-Pick LogLevel & Health Checks Bug Fix#3249
Open
RubenCerna2079 wants to merge 2 commits intorelease/1.7from
Open
Cherry-Pick LogLevel & Health Checks Bug Fix#3249RubenCerna2079 wants to merge 2 commits intorelease/1.7from
RubenCerna2079 wants to merge 2 commits intorelease/1.7from
Conversation
…nfig key (#3203) ## Why make this change? - #3201 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? **All logs follow the LogLevel configuration** - Added more specific configuration for logs in the host level, `Program.cs` which where outputing some logs that where not following the LogLevel configuration from CLI and from configuration file. - Add `DynamicLogLevelProvider` class which allows the loggers in the host level to be updated after the RuntimeConfig is parsed and we know the LogLevel. - Add `StartupLogBuffer` class which saves the logs that are created before we know the LogLevel, and sends them to their respective logger after the RuntimeConfig is parsed. **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`. ```json // 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 - [x] Unit Tests - `ValidStringLogLevelFilters`: added `"Default"` (capital D) data row to cover the case-insensitive validation fix. ## Sample Request(s) ```bash # Suppress all output dab start --LogLevel None # Config key now case-insensitive # dab-config.json: # "telemetry": { "log-level": { "Default": "none" } } dab start ``` --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: RubenCerna2079 <32799214+RubenCerna2079@users.noreply.github.com> Co-authored-by: Ruben Cerna <rcernaserna@microsoft.com> Co-authored-by: Aniruddh Munde <anmunde@microsoft.com> Co-authored-by: Anusha Kolan <anushakolan10@gmail.com>
RubenCerna2079
requested review from
Aniruddh25,
aaronburtle,
anushakolan,
neeraj-sharma2592,
sourabh1007 and
souvikghosh04
as code owners
RubenCerna2079
requested review from
Alekhya-Polavarapu,
rusamant,
stuartpa and
vadeveka
as code owners
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
Aniruddh25
approved these changes
Mar 13, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Ports the LogLevel fixes from #3203 into this branch to ensure startup/host logging respects the configured/CLI log level and to make the telemetry.log-level "Default" key case-insensitive.
Changes:
- Adds a
DynamicLogLevelProviderand wires host-level logging filters to it so early/runtime log suppression works (e.g.,--LogLevel None). - Adds a
StartupLogBufferand updatesFileSystemRuntimeConfigLoaderto buffer/emit startup logs viaILoggerinstead of writing directly to the console. - Updates log-level validation/lookup to be case-insensitive for
"default"/"Default"and extends unit test coverage for the"Default"key.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Service/Telemetry/DynamicLogLevelProvider.cs | New dynamic provider used by host logging filters and updated after runtime config is available. |
| src/Service/Program.cs | Wires host logging filters to DynamicLogLevelProvider; removes direct console startup message. |
| src/Service/Startup.cs | Registers log buffer/provider; passes buffer to config loader; flushes buffered config-loader logs after log level is finalized. |
| src/Config/StartupLogBuffer.cs | New concurrent buffer for early startup log messages. |
| src/Config/FileSystemRuntimeConfigLoader.cs | Replaces console writes with buffered/logger-based logging; adds logger/buffer plumbing and flush support. |
| src/Core/Configurations/RuntimeConfigValidator.cs | Makes logger filter validation case-insensitive. |
| src/Config/ObjectModel/RuntimeConfig.cs | Updates "default" log level lookup logic to be case-insensitive. |
| src/Service.Tests/Configuration/ConfigurationTests.cs | Adds test data row covering "Default" (capital D) key handling. |
| src/Cli.Tests/EndToEndTests.cs | Adjusts expectations to match removal of the console startup message. |
You can also share your feedback on Copilot code review. Take the survey.
## Why make this change? Closes #2977 Health check endpoint was returning results for stored procedures. Stored procedures should be excluded because: 1. They require parameters not configurable via health settings 2. They are not deterministic, making health checks unreliable ## What is this change? Added filter in `HealthCheckHelper.UpdateEntityHealthCheckResultsAsync()` to exclude entities with `EntitySourceType.StoredProcedure`: ```csharp // Before .Where(e => e.Value.IsEntityHealthEnabled) // After .Where(e => e.Value.IsEntityHealthEnabled && e.Value.Source.Type != EntitySourceType.StoredProcedure) ``` Only tables and views are now included in entity health checks. ## How was this tested? - [ ] Integration Tests - [x] Unit Tests Added `HealthChecks_ExcludeStoredProcedures()` unit test that creates a `RuntimeConfig` with both table and stored procedure entities, then applies the same filter used in `HealthCheckHelper.UpdateEntityHealthCheckResultsAsync` to verify stored procedures are excluded while tables are included. ## Sample Request(s) Health check response after fix (stored procedure `GetSeriesActors` no longer appears): ```json { "status": "Healthy", "checks": [ { "name": "MSSQL", "tags": ["data-source"] }, { "name": "Book", "tags": ["rest", "endpoint"] } ] } ``` <!-- START COPILOT CODING AGENT SUFFIX --> <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>[Bug]: Health erroneously checks Stored Procedures</issue_title> > <issue_description>## What? > > Health check returns check results for stored procs. It should ONLY include tables and views. > > ## Health output sample > > ```json > { > "status": "Healthy", > "version": "1.7.81", > "app-name": "dab_oss_1.7.81", > "timestamp": "2025-11-17T20:33:42.2752261Z", > "configuration": { > "rest": true, > "graphql": true, > "mcp": true, > "caching": true, > "telemetry": false, > "mode": "Development" > }, > "checks": [ > { > "status": "Healthy", > "name": "MSSQL", > "tags": [ > "data-source" > ], > "data": { > "response-ms": 3, > "threshold-ms": 1000 > } > }, > { > "status": "Healthy", > "name": "GetSeriesActors", // stored procedure > "tags": [ > "graphql", > "endpoint" > ], > "data": { > "response-ms": 1, > "threshold-ms": 1000 > } > }, > { > "status": "Healthy", > "name": "GetSeriesActors", // stored procedure > "tags": [ > "rest", > "endpoint" > ], > "data": { > "response-ms": 5, > "threshold-ms": 1000 > } > } > ] > } > ```</issue_description> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > <comment_new><author>@souvikghosh04</author><body> > @JerryNixon / @Aniruddh25 should stored procedures and functions be discarded from health checks permanently?</body></comment_new> > <comment_new><author>@JerryNixon</author><body> > The entity checks in the Health endpoint check every table and view type entity with a user-configurable select with a first compared against a user-configurable threshold. We do not check stored procedures, and cannot check stored procedures, as we do not have any mechanism to take parameters as Health configuration values. Also stored procedures are not guaranteed to be deterministic, making checks that would call them potentially be unreliable. So, yes, stored procedures should be ignored. </body></comment_new> > </comments> > </details> - Fixes #2982 <!-- START COPILOT CODING AGENT TIPS --> --- 💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey). --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Aniruddh25 <3513779+Aniruddh25@users.noreply.github.com> Co-authored-by: JerryNixon <1749983+JerryNixon@users.noreply.github.com>
RubenCerna2079
changed the title
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
Aniruddh25
approved these changes
Mar 13, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Why make this change?
This change ports the fix for the log-level bugs and the fix for the health checks bug
What is this change?
Cherry-picked changes:
How was this tested?
These changes were already tested in the original PR