Add Query Store performance diagnostics - #5723
Draft
Mikael Weaver (mikaelweave) wants to merge 21 commits into
Draft
Add Query Store performance diagnostics#5723Mikael Weaver (mikaelweave) wants to merge 21 commits into
Mikael Weaver (mikaelweave) wants to merge 21 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add schema version 117 with bounded Query Store and statistics procedures, least-privilege grants, and SQL-backed integration coverage.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move the required Category and OwningTeam traits to the test class so assembly validation recognizes them.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Terminate the LogEvent statement before the bare THROW so incremental schema upgrades can compile GetQueryStorePlanDiagnostics.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use Azure SQL diagnostic settings for wait statistics, reduce plan sanitization and audit plumbing, and keep the SQL interface focused on runtime metrics, sanitized plans, and statistics health.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Return Query Store waits with slow-query results so direct SQL and future Geneva callers can retrieve the complete diagnostic payload without joining Log Analytics.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5723 +/- ##
=======================================
Coverage ? 78.92%
=======================================
Files ? 1027
Lines ? 38322
Branches ? 5823
=======================================
Hits ? 30247
Misses ? 6636
Partials ? 1439 🚀 New features to boost your workflow:
|
Add section headers and focused rationale for validation, aggregation, edge cases, plan sanitization, and error handling without changing executable SQL.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replaces the stored-procedure design with an opt-in background watchdog that runs inside the FHIR server on its existing SQL identity and pushes diagnostics out as IMetricsNotification messages. The previous design exposed three stored procedures behind a new FhirDiagnosticsReader execute-only role, to be called by an external operational principal. That introduced an entirely new inbound permission model the service does not otherwise have. This version inverts the direction: nothing connects in, the server emits out. Changes: - QueryStoreDiagnosticsWatchdog reads Query Store on a configurable period using the existing Watchdog<T> lease, so exactly one instance runs per database. - Detect-only Query Store handling: the watchdog reports state and skips its cycle when unavailable, and never issues ALTER DATABASE. - Plan sanitization moved from 236 lines of T-SQL into C# (QueryPlanSanitizer), which is more reliable and testable. - Emits SlowQueryNotification, QueryPlanNotification and StatisticsHealthNotification. - Disabled by default. Enabled via FhirServer:Watchdog:QueryStoreDiagnostics and gated at runtime by the dbo.Parameters row, matching DefragWatchdog. No schema change: all SQL is inline, so schema version stays at V116 and the migration, role script and sprocs are removed. Tests: 9 sanitizer unit tests, plus 3 integration tests that execute every inline statement against a live SQL Server and assert all three notifications publish. AB#186447 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Bind every diagnostics read to the primary. The reads passed isReadOnly: true, which lets ReplicaHandler route them to a read-only secondary. Query Store reports READ_ONLY there, so the desired_state gate returned early and the feature would silently emit nothing. Make failure observable rather than silent: - the missing-Query-Store path (error 208) logs Warning, not Debug - wait statistics failure is surfaced on the notification through a new WaitStatisticsStatus property instead of being flattened into zeros - each completed tick logs its window and counts, including zeros, so a watchdog that runs but collects nothing is distinguishable from one that is not running at all - sanitization failure logs Warning with the plan id and status - readonly_reason is decoded from its bitmask into text Harden the PHI boundary. Post-sanitization verification walked the serialized string, so any plan whose own StatementText contained the literal "ParameterList" was discarded as a false positive. It now walks the element tree by local name, which is both accurate and namespace- agnostic. Make QueryPlanSanitizationResult unable to hold a contradictory state: private constructor, four named factories, and Truncated derived from the payload rather than passed alongside it. Also warn when configuration disables collection, document the WHY behind each SQL trap, and correct the failure-containment claim in the design doc. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Round two of review findings, all verified against the source. Three statements were claiming more than the code delivers: - the design doc said a missing Query Store row is logged "naming the state and readonly_reason", which that branch cannot do because there is no row; the two branches are now described separately - it said wait capture being off yields Failed with a warning, when only a caught SqlException does; the ordinary empty result is Unavailable and silent - both the doc and the XML doc on QueryPlanSanitizationResult said "not verified but populated" is unconstructable. The success factory accepts any non-null string, so the guarantee that actually holds is narrower: no failure status can carry a payload, success refuses null, and Truncated is derived. The claim now matches that and the type is unchanged. Make two log lines carry the information they implied: - the missing-view handler spans the whole collection, so the aborting read can be the last one and slow queries may already have been published. It no longer says "no diagnostics can be collected". - the completion log's plan count returned the slow-query count unconditionally, so it always equalled its neighbour. It now counts plans that actually carried sanitized XML. DescribeReadonlyReason dropped unrecognized bits whenever a documented bit was set alongside them, so a state flag from a newer SQL Server would vanish. Unknown bits are now reported with the raw value. Remove a DTO that mirrored StatisticsHealthNotification property for property, and a truncation helper class serving one call site. A non-positive SlowQueryCount now skips the read rather than issuing TOP (0). Tests: the probe asserted Assert.Single over notifications grouped by plan_id while the capture poll summed across plans, so a recompile between the two probe executions would have produced two plans and an intermittent red build; it now sums. Statistics health asserted only NotEmpty over twelve positional reads, leaving column order unpinned; it now pins ordinals against deterministic full-scan statistics. The CPU lower bound moves off zero, and a new unit test drives the wait read to throw and asserts slow queries still publish with WaitStatisticsStatus of Failed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| Assert.NotNull(probeIndexStatistics.Rows); | ||
| Assert.NotNull(probeIndexStatistics.RowsSampled); | ||
| Assert.NotNull(probeIndexStatistics.ModificationCounter); | ||
| Assert.Equal((long)ProbeTableRowCount, probeIndexStatistics.Rows.Value); |
| Assert.NotNull(probeIndexStatistics.RowsSampled); | ||
| Assert.NotNull(probeIndexStatistics.ModificationCounter); | ||
| Assert.Equal((long)ProbeTableRowCount, probeIndexStatistics.Rows.Value); | ||
| Assert.Equal((long)ProbeTableRowCount, probeIndexStatistics.RowsSampled.Value); |
| Assert.NotNull(probeIndexStatistics.ModificationCounter); | ||
| Assert.Equal((long)ProbeTableRowCount, probeIndexStatistics.Rows.Value); | ||
| Assert.Equal((long)ProbeTableRowCount, probeIndexStatistics.RowsSampled.Value); | ||
| Assert.Equal((long)ProbeTableModificationCount, probeIndexStatistics.ModificationCounter.Value); |
| Assert.Equal((long)ProbeTableRowCount, probeIndexStatistics.RowsSampled.Value); | ||
| Assert.Equal((long)ProbeTableModificationCount, probeIndexStatistics.ModificationCounter.Value); | ||
| Assert.NotNull(probeIndexStatistics.ModificationPercent); | ||
| Assert.Equal(ProbeTableModificationCount * 100.0 / ProbeTableRowCount, probeIndexStatistics.ModificationPercent.Value, 6); |
Reviewing the configuration surface turned up two ways an operator can set something and get no effect and no error. dbo.Parameters declares its primary key WITH (IGNORE_DUP_KEY = ON), so the seeding INSERT in Watchdog.InitParamsAsync is a silent no-op on a database that already holds the row, and the following line reads the stored value back over the configured one. Changing the configured PeriodSec on an initialized database therefore does nothing, silently, for both the tick interval and the lookback window. This is shared framework behaviour, so rather than change it, the watchdog now logs a warning at initialization when the stored period differs from the configured one. The runtime gate has no configuration binding at all: it is seeded to 0 and can only be armed with an UPDATE against dbo.Parameters. Setting only FhirServer:Watchdog:QueryStoreDiagnostics:Enabled collects nothing and logs "is not enabled". The doc now says so and gives the statement, and records that the remaining six settings bind through IOptions and so take effect on restart rather than reloading in place. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Round-3 review fixes for the Query Store diagnostics watchdog. Resilience: - Reject a non-positive or non-finite configured PeriodSec in the constructor and fall back to the class default with a warning. PeriodSec flows to PeriodicTimer via the shared framework, which throws on such values; that would fault this watchdog's task and WatchdogsBackgroundService cancels the token shared by every watchdog, so an off-by-default diagnostics feature could take the transaction and cleanup watchdogs down with it. - Track the effective configured period separately so a rejected value is not also reported as overridden by the stored dbo.Parameters row. Observability: - Warn when the lookback clamp decouples from the unclamped tick interval, naming the unexamined window or the overlap. - Promote the IsEnabled gate to a warning carrying the remedy statement; the row has no config binding, so reaching it always means an explicit opt-in that was never armed. - Warn on a negative MinDurationMilliseconds. PHI boundary: - Fail a rootless plan document closed. It is the one condition that would otherwise skip removal and satisfy verification, emitting the document verbatim. XDocument.Load makes it unreachable today; a PHI boundary should have no path where sanitization is skipped and verification reports success. Simplification and tests: - Derive Truncated in the constructor rather than at four call sites, and replace the single-use QueryPlanResult DTO with a named tuple. - Add hostile-XML DOCTYPE coverage and QueryStoreDiagnosticsPeriodTests for the guard, both divergence directions, and both clamp directions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
RunStartDate and RunEndDate bound the period during which collection happens. Both are DateTimeOffset? defaulting to null, so behaviour is unchanged when neither is set: an unset start collects from the first tick and an unset end collects indefinitely. The start is inclusive and the end exclusive, so adjacent windows tile without overlapping. The window is evaluated after both enablement gates, so a deployment outside its window still gets the warning that the dbo.Parameters runtime gate was never armed. The watchdog keeps ticking past RunEndDate rather than shutting down. Completing or faulting a watchdog task makes WatchdogsBackgroundService cancel the token shared by every watchdog, so an off-by-default diagnostics feature ending its own timer would take the transaction and cleanup watchdogs with it. An hourly clock comparison costs nothing. Window state is logged only on change. At the default hourly period a window opening in a month would otherwise emit ~720 identical skip lines. The state a process starts in is always logged once, so the reason for silence is available immediately after a restart. A start that is not before the end is an empty window and is warned about at initialization, since nothing downstream would report it. When either bound is set the effective window is logged converted to UTC: a value without an explicit offset binds in the host's local timezone, which is invisible in the configured text and rarely intended. Verified against the real configuration type that a DateTimeOffset? binds from an environment variable, that an offset-less value resolves to local time, and that a malformed value is rejected by the binder exactly as the existing typed settings are. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The watchdog no longer reads or writes dbo.Parameters. Arming the feature previously required an operator UPDATE against that table, and the base class read its period back over the configured value, so configuration was not authoritative. Both are the pattern the team is moving away from: a diagnostics feature should not require writing to the database. Watchdog<T>.InitParamsAsync is private and non-virtual and is called unconditionally from ExecuteAsync, so there was no override hook. This watchdog therefore schedules itself, owning a FhirTimer and a WatchdogLease directly and reproducing the lease-holder gate, the capped randomized stagger and the per-tick timing line. The lease is kept deliberately: it is runtime coordination rather than configuration, and without it every replica would collect and emit the same diagnostics each period. WatchdogLease<T> was constrained to T : Watchdog<T> while using its type argument only for typeof(T).Name. The constraint restricted nothing the class used and is relaxed here so a self-scheduling component can elect a single replica; every existing caller passes a Watchdog<T> and is unaffected. The alternative, an abstract type existing only to satisfy the constraint, would have reintroduced a Watchdog<T> subclass into the feature that exists to leave it. The constructor period guard is retained and still load-bearing: WatchdogsBackgroundService cancels the token shared by every watchdog as soon as one task completes, so a bad period would still take the transaction and cleanup watchdogs down. Adds ADR-2608 recording why collection runs as an in-process job rather than through an external caller granted rights on the data plane, directly or fronted by Geneva Actions. The deciding argument is that the job introduces no new access path: it runs on the identity the server already holds, pushes results through the existing notification pipeline, and is enabled through the existing configuration surface. Integration coverage now asserts against a live database that the watchdog creates no dbo.Parameters rows, and a unit test asserts the type declares no dbo.Parameters literal and derives from object, so re-deriving would fail rather than silently restore the seeding insert. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Mikael Weaver (mikaelweave)
force-pushed
the
personal/mikaelw/query-store-performance-diagnostics
branch
from
August 22, 2026 03:17
3173045 to
bad2ec2
Compare
Applies the author's edits and reworks the wording to read less formally and to sit closer to the other ADRs in docs/arch. Halves the em-dash count, breaks the dense compound sentences in the decision into separate paragraphs, and drops rhetorical scaffolding. Restores the configuration-only decision to the Decision section. The adverse effects referred to keeping configuration out of dbo.Parameters while the decision itself no longer stated it, so a reader met the cost of a choice that had not been recorded. Context now says why Query Store data is a PHI concern specifically: plans capture compiled and runtime parameter values, so a plan can carry patient data even when no resource table was queried. That is the reason sanitization exists and is worth stating where the risk is introduced. Also records the lease include and exclude patterns as an adverse effect. They live in dbo.Parameters, belong to the shared lease rather than to this feature, and can leave the feature enabled and silent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
StatisticsHealthCount looked like an arbitrary truncation of a set that could be much larger. Records what it actually is and what it costs. Each reported row is published as its own notification, so the setting multiplies emission volume per collection, per database, per host. The schema defines roughly a hundred index-backed statistics before SQL Server adds auto-created column statistics, so reporting everything would make one collection several hundred notifications. ADR-2605 records the consequence of that pattern: a shared metric account was throttled and monitoring degraded for both FHIR and DICOM. Also records a bias in the ordering. Ranking is by modification ratio, so a small heavily-churned table outranks a large one that has drifted less proportionally, and small busy tables can fill the report while a consequential stale statistic on a large table falls below the cut. Noted rather than changed, since which definition of "worst" is right depends on what is being chased. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The payload was never metric shaped. QueryText is unbounded high-cardinality text, SanitizedQueryPlan is an XML document, and TopWaitCategory is a high-cardinality string. None of those work as metric dimensions, and metric events are charged on receipt. adr-2605-metric-emission-rate-limiting.md records what volume on that pipeline costs: a shared metric account was throttled and monitoring degraded for both FHIR and DICOM. The three payload types no longer implement IMetricsNotification, and the watchdog no longer takes IMediator at all. Since the types are no longer a cross-assembly contract they move out of Core into the watchdog folder as internal types and lose the FhirOperation and ResourceType members that existed only to satisfy the metrics interface. Leaving public INotification types in Core that nothing publishes would have been misleading. They are new in this PR and have never shipped, so nothing depended on them. Slow queries and plans are emitted one structured line each, with named properties so each field stays queryable as a column. Statistics rows are batched into a JSON array, StatisticsHealthBatchSize rows per line, each line carrying its page number, page count and total row count so a partial final page is distinguishable from a set cut short. Batching suits these rows because they are small, uniform and free of free text; plan XML could not make that promise. Batch size is clamped to 64. Batching trades record count for record size, and an unbounded batch would rebuild the single oversized record that is the reason plan XML is not batched at all. A serialized row is a little under 400 bytes, so 64 keeps a page inside the 32 KB budget already applied to other large fields. Clamping pages the rows rather than dropping them. Sanitization and truncation are unchanged. The same bytes are emitted; only the destination differs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…y-store-performance-diagnostics
The Watchdogs folder followed an implicit one-file-per-watchdog convention. This feature had added six flat files to it, becoming ~40% of the folder and burying the six sibling watchdogs. Move the feature's files into Features/Watchdogs/QueryStoreDiagnostics/, with the three log payload shapes under a Models/ subfolder, and mirror the same structure in the unit test project. Namespaces follow the folder layout, matching the convention used by Schema/Model, Search/Expressions, Storage/TvpRowGeneration and Operations/Import. All moves are pure relocations - no behaviour changes. Both projects are SDK-style with automatic globbing, so no csproj edits are needed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tative WatchdogLease<T> had its `where T : Watchdog<T>` constraint relaxed so the Query Store diagnostics watchdog could elect a single replica while scheduling itself. Revert that shared file to origin/main and derive from Watchdog<T> like every other watchdog. Deriving from the base class reintroduces the two dbo.Parameters rows it seeds, which is acceptable provided configuration can still set them. By default it cannot: dbo.Parameters is declared WITH (IGNORE_DUP_KEY = ON), so on a database that already holds the rows the seeding INSERT is a silent no-op, and InitParamsAsync then reads the stale stored value back over the configured one. Verified against SQL Server: seeding 3600 then 60 for the same Id reports "Duplicate key was ignored. (0 rows affected)" and leaves 3600 in place. A fresh database honours the environment variable while an upgraded one quietly does not. Override InitAdditionalParamsAsync -- the one initialization step the base class makes virtual, which runs after that read-back and before the timer is built -- to UPDATE both rows to the configured values and re-assign the properties. UPDATE rather than INSERT precisely because IGNORE_DUP_KEY would make a re-INSERT a no-op. Configuration now wins on every database and the rows are a readable mirror of it rather than an input to it. The UPDATE is wrapped in a catch that logs and continues: it runs outside FhirTimer's per-tick catch, where a throw would fault this watchdog's task and cause WatchdogsBackgroundService to cancel every other watchdog with it. The property assignments sit outside the try, so the functional guarantee does not depend on the cosmetic one. LeasePeriodSec becomes a real configuration setting rather than a hard-coded const, since the base class stores it and every stored value must be settable from configuration. The unit test that pinned the old invariant is rewritten to pin the new one, and the integration test that asserted zero parameter rows now pre-seeds stale rows and asserts they are reconciled. Confirmed to be a real guard by mutation: removing the UPDATE fails it with 999999 vs 1. WatchdogLease.cs, Watchdog.cs and FhirTimer.cs are byte-identical to origin/main. Solution builds 0 warnings; 98 unit and 3 integration tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
+406
to
+423
| catch (Exception exception) | ||
| { | ||
| // Reported and swallowed rather than propagated, and deliberately so. This runs inside | ||
| // Watchdog<T>.ExecuteAsync's initialization, which is before and outside FhirTimer's per-tick catch, | ||
| // so a throw here faults this watchdog's task — and WatchdogsBackgroundService cancels the token | ||
| // shared by EVERY watchdog as soon as one task completes. Letting an off-by-default diagnostics | ||
| // feature fail the transaction and cleanup watchdogs over a cosmetic row update is the wrong trade. | ||
| // Nothing about the collection depends on the update succeeding: the rows are a mirror of | ||
| // configuration, not an input to it, and the assignments below make configuration authoritative in | ||
| // this process whether or not the mirror was written. The cost of failure is a stale pair of rows | ||
| // that disagree with the running configuration, which is what this warning names. | ||
| _logger.LogWarning( | ||
| exception, | ||
| "{WatchdogName}: could not reconcile the {PeriodSecId} and {LeasePeriodSecId} rows in dbo.Parameters to the configured values. Collection is unaffected and continues to run at the configured period, but those rows may now disagree with configuration and should not be read as the values in use.", | ||
| Name, | ||
| PeriodSecId, | ||
| LeasePeriodSecId); | ||
| } |
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.
Description
Adds opt-in Query Store performance diagnostics to the FHIR server as a background watchdog that emits telemetry, so slow-query analysis can be done without anyone connecting to the customer database.
How it works
QueryStoreDiagnosticsWatchdoguses the repo's existingWatchdog<T>lease, so exactly one instance runs per database. On each tick it reads Query Store on the server's existing SQL identity and publishes:SlowQueryNotificationQueryPlanNotificationStatisticsHealthNotificationDisabled by default. Enabled via
FhirServer:Watchdog:QueryStoreDiagnosticsand gated at runtime by thedbo.Parametersrow, matching theDefragWatchdogoperational-switch convention — so it can be turned off live without a deployment.Notable decisions
ALTER DATABASE.QueryPlanSanitizer— more reliable, and directly unit-testable. Plans are stripped of literals and verified before truncation; the XML reader is XXE-hardened.Trade-offs taken for simplicity, and the work deliberately deferred, are listed in the doc under "Simplifications and deferred work".
Testing
QueryPlanSanitizer(literal removal, namespace variants, verification, truncation reporting, malformed/hostile XML).The integration tests matter more than usual here: because the SQL is inline
conststrings, nothing in the C# build parses it. These tests are what execute it. They already caught two defects that compiled cleanly —statisticsused as a table alias (a reserved T-SQL keyword) and areadonly_reasonint/bigintcast mismatch that would have shipped the feature completely non-functional.Related
docs/QueryStorePerformanceDiagnostics.mdFHIR Team Checklist