Serve Query Store regressions (#2484) - #2507
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| lookback: it is everything collected before the window, so a longer hours_back makes | ||
| the recent window bigger AND the baseline shorter. | ||
| */ | ||
| baseline_is = "every Query Store capture collected BEFORE the recent window", |
There was a problem hiding this comment.
Lite/Darling parity drift: Darling's get_query_store_regressions response includes recent_window_start and recent_window_end (see DarlingMcpQueryStoreRegressionTools.cs, right after database_name):
recent_window_start = start.ToString("o"),
recent_window_end = end.ToString("o"),
baseline_is = "every Query Store capture collected BEFORE recent_window_start",Lite's response drops both fields entirely — baseline_is here just says "the recent window" without giving its bounds anywhere in the payload. Since the PR's own "Deliberate differences from the viewer" section only calls out the row-cap parameter and the query-text-source fallback as intentional Lite/Darling differences, this looks like an unintentional gap rather than a deliberate one. A client (or an agent) switching between the two SKUs for the same tool would see a different JSON shape and lose the ability to state which literal timestamps the "recent" window covered on Lite. Worth adding recent_window_start/recent_window_end here (using the startTime/endTime from GetTimeRange above) to match Darling.
| resolved.ServerId, hours_back, limit + 1, databases); | ||
|
|
||
| if (rows.Count == 0) | ||
| return await EmptyRegressionsAsync(dataService, resolved.ServerId, resolved.ServerName, hours_back); |
There was a problem hiding this comment.
Minor: EmptyRegressionsAsync is passed only hours_back, not the start/end already computed a few lines above for the main query. Inside it, GetQueryStoreRegressionCoverageAsync calls GetTimeRange(hoursBack, null, null) again, which does a fresh DateTime.UtcNow.AddHours(-hoursBack) / DateTime.UtcNow — a different (later) window boundary than the one the main query just ran with.
Darling's equivalent (DarlingMcpQueryStoreRegressionTools.EmptyAsync) avoids this by reusing the exact same start/end it already computed for the main read, so the coverage probe is guaranteed to describe the same window the empty result came from. Here the two calls could disagree by however long the DB round-trip took, so in a narrow race a row collected between the two calls could flip has_recent/has_baseline relative to what the (already-returned) empty main query saw. Given collectors run on minute-scale cadences this is unlikely to matter in practice, but it's a real deviation from the "one probe answers exactly what the read just saw" invariant the file-header comment describes, and from Darling's implementation.
|
Reviewed the diff (Darling Postgres reader/tools + Lite DuckDB service/tool + shared web panel, plus both test suites). Overall this is careful, well-tested work — the SQL is genuinely copied verbatim between Darling/Lite/the viewer's TVF (dedup partition keys, gate, ranking, severity bands all match), the four empty-state branches are distinct and correctly ordered, Left two inline comments on the Lite side, both around
Nothing else stood out as a correctness, security, or performance issue — |
| /// two booleans, run only on this path.</para> | ||
| /// </summary> | ||
| private static async Task<string> EmptyAsync( | ||
| NpgsqlDataSource postgres, string serverName, int serverId, DateTime start, DateTime end, int hours_back) |
There was a problem hiding this comment.
Coverage probe ignores database_name, so the four-states diagnosis can be wrong for the exact case this feature exists to get right.
GetQueryStoreRegressionsAsync (line 60-61) is filtered to database_name when the caller supplies it, but EmptyAsync here — and DarlingQueryStoreRegressionReader.GetCoverageAsync/RegressionCoverageSql that it calls — never receive database_name at all. The coverage probe checks baseline/recent existence across the whole server, not the requested database.
Concrete failure: server has Prod with a full baseline and recent window with zero regressions, and Marketing, which has never had Query Store collected. A caller asks for get_query_store_regressions(database_name: "Marketing"). The main query correctly returns 0 rows for Marketing, but EmptyAsync sees hasBaseline=true, hasRecent=true (from Prod) and returns:
"No query on {server} regressed in the last N hour(s) ... this IS the all-clear for this read."
That's a confident wrong answer for Marketing — exactly the failure mode the PR description calls out as the reason this read exists ("a confident wrong answer rather than a missing one"). The same gap exists in the "never collected" and "no baseline" branches too, since none of them are scoped to the database either.
Fix: add the same $4::text[] IS NULL OR database_name = ANY($4) predicate to RegressionCoverageSql, and thread database_name through GetCoverageAsync → EmptyAsync (called from line 64). The Lite twin (Lite/Mcp/McpQueryTools.cs EmptyRegressionsAsync / LocalDataService.GetQueryStoreRegressionCoverageAsync) has the identical gap.
Untested today — none of the new tests pass database_name through the four-states walk, so this wouldn't be caught by the current suite.
| /// no BEFORE, so it can never show a regression however badly it regressed, and answering "no | ||
| /// regressions" there is a confident wrong answer rather than a missing one.</para> | ||
| /// </summary> | ||
| private static async Task<string> EmptyRegressionsAsync( |
There was a problem hiding this comment.
Same gap as the Darling twin (DarlingMcpQueryStoreRegressionTools.EmptyAsync): database_name is filtered on the main read (line 295-297 above) but EmptyRegressionsAsync → LocalDataService.GetQueryStoreRegressionCoverageAsync checks baseline/recent coverage for the whole server, ignoring database_name. A caller who scopes to a database with no Query Store history at all — while other databases on the server have full baseline+recent coverage — gets the "all-clear" (or wrong "no baseline"/"never collected") message instead of the truth for the database they asked about. Needs the same database_name filter that BuildDbInClause already applies to the main query, threaded into the coverage SQL/method.
Review summaryReviewed the new Correctness
Lite/Darling parity
Security: all inputs are bound via parameterized queries ( Style: T-SQL style rules don't apply directly (this SQL is Postgres/DuckDB), and the No missing-index DMV recommendations to flag (none present). |
The last of the #2484 list I am taking, and the only tab in the per-server page that was entirely unreachable rather than merely reduced. Every other Query Store read answers "what is expensive". This answers "what got WORSE", and the second is not derivable from the first: the costliest query on a server is usually the one that has always been the costliest, and the one that changed last Tuesday sits well down that list. The SQL is the viewer's, which is the Dashboard TVF's, copied rather than re-derived. The gate (average CPU worse by more than 25%), the ranking (execution-count-weighted extra duration -- a 5 ms regression run a million times outranks a 5-second one run twice) and the severity bands are what decide WHICH queries a user is shown, and a browser that disagreed with the desktop viewer about that would be worse than one that showed nothing. The dedup is correctness here, not performance, and more so than on any sibling read. Query Store rows are cumulative per-interval snapshots that the collector re-fetches while an interval stays open. The baseline arm is UNBOUNDED -- potentially months -- while the recent arm is a short window, so the two sides being compared have systematically different re-collection density per interval. Un-deduped, that alone moves the averages the percent is computed from and the 25% gate, manufacturing and hiding regressions for reasons that have nothing to do with the query. Zero rows is FOUR states, and only one is good news. Never collected. No BASELINE, because every capture falls inside the requested window. Nothing collected IN the window. And a genuine all-clear. One probe, two booleans, run only on the empty path. The second of those is the reason the branching is worth the code. A server whose entire history sits inside the window has no BEFORE, so it cannot show a regression however badly it regressed -- and "no regressions" there is a confident wrong answer, not a missing one. Its advice also runs the other way: widening the window makes the baseline SHORTER, so that branch says shorten, and the tests pin that it never says widen. One deliberate change from the viewer: the row cap is a bound parameter rather than the hardcoded 50, so a caller can ask for fewer or more. The gate and the ranking are untouched, so the first 50 rows of any call are the viewer's 50. Lite has the columns but not collect.query_store_text, so its text sample comes from the fact rows only -- the same text, one hop closer. That is a difference in where the text comes from, not in which rows the read returns. Census 103 -> 104 tools, 78 -> 79 shared, Darling-unique unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…to a corner The no-recent branch was asserted after a row had already been planted 30 minutes ago, and no window a caller can legally ask for excludes a row that recent -- so that state was unreachable and the read correctly answered all-clear instead. My assertion was wrong, not the read. Seeding order now runs baseline-only first, so the empty-window branch is reachable, and the no-baseline branch is reached from the SAME rows by widening hours_back until every one of them falls inside the window. Four states, one fixed set of rows, no deletes between assertions. Both SKUs had the identical flaw, which is what porting a test alongside a tool is supposed to catch and did not: I ported the mistake too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three tools landed on dev while this branch was open, and the rebase merged the census sentence without knowing it is arithmetic. 105/80 was the number for the dev I branched from; the real inventory is 106/81. Derived with the same scan the pin uses rather than incremented by hand -- the sentence is prose an LLM plans against, and a stale one is worse than an absent one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
06b8763 to
1f60583
Compare
Both sides of the conflict were counts -- 106/81 here, 107/82 on dev -- and after the merge NEITHER is right, because each described a tree without the other's tools in it. Taking either would have produced a confidently wrong sentence that happens to look like a considered resolution. Recounted from the attributes the pin itself scans: 108 tools, 83 shared, 25 Darling-only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| private static async Task<string> EmptyRegressionsAsync( | ||
| LocalDataService dataService, int serverId, string serverName, int hours_back) | ||
| { | ||
| var (hasBaseline, hasRecent) = await dataService.GetQueryStoreRegressionCoverageAsync(serverId, hours_back); | ||
|
|
||
| if (!hasBaseline && !hasRecent) | ||
| { |
There was a problem hiding this comment.
Minor Lite/Darling asymmetry: on the empty-rows path, Darling captures start/end once at the top of GetQueryStoreRegressions and threads them into EmptyAsync so the coverage probe checks the exact same window the main query just ran against:
var end = DateTime.UtcNow;
var start = end.AddHours(-hours_back);
...
return await EmptyAsync(postgres, resolved.ServerName, resolved.ServerId, start, end, hours_back);Here, EmptyRegressionsAsync only receives hours_back and GetQueryStoreRegressionCoverageAsync re-derives the window itself via a second GetTimeRange(hoursBack, null, null) call, i.e. a fresh DateTime.UtcNow. Since the main regression query already ran (and found 0 rows) against the earlier window, the coverage probe a moment later is technically answering "is there baseline/recent data as of now", not "as of the window the empty result came from". In practice the drift is milliseconds against an hours-wide window, so this is unlikely to ever flip a status in reality — but it's worth threading the already-computed (startTime, endTime) through the same way Darling does, both for exact behavioral parity and to remove the theoretical race (a row landing in the gap between the two UtcNow calls could flip has_recent and change which of the four messages is returned).
Review summaryReviewed the What I checked closely:
One minor finding (posted inline on No SQL-injection, missing-index-DMV, or Lite/Darling capability-gap issues found. Nice test coverage on the four-branch empty-state logic and the dedup correctness story. |
1 participant
Sixth item on #2484, and the one the issue called "the only #2477 tab that is entirely unreachable rather than reduced".
What the viewer could see that nothing else could
Every other Query Store read answers "what is expensive". This answers "what got WORSE" — and the second is not derivable from the first. The costliest query on a server is usually the one that has always been the costliest; the one that changed last Tuesday sits well down that list.
Each
(database, query_id)group's averages inside the recent window are compared against its baseline — every capture collected BEFORE that window — giving baseline vs recent duration / CPU / logical reads with a regression percent for each, the plan counts on both sides, a duration-driven severity band, andadditional_duration_ms, which is both the ranking key and the number that says whether the regression matters at all: a 5 ms regression executed a million times outranks a 5-second one executed twice.The SQL is the viewer's — which is the Dashboard's
report.query_store_regressionsTVF — copied rather than re-derived. The gate (average CPU worse by more than 25%), the ranking, and the severity bands decide which queries a user is shown, and a browser that disagreed with the desktop viewer about that would be worse than one that showed nothing.The dedup is correctness here, not performance
Query Store rows are cumulative per-interval snapshots and the collector re-fetches an open interval every cycle. This read is the most exposed of any to that: the baseline arm is UNBOUNDED (potentially months) while the recent arm is a short window, so the two sides being compared have systematically different re-collection density per interval. Un-deduped, that alone moves the averages the percent is computed from and the 25% gate — manufacturing and hiding regressions for reasons that have nothing to do with the query. Both arms dedup on the full interval identity, and there is a pin asserting the count is 2, not 1.
What the empty branches say
Zero rows is four states, and only one is good news. One probe, two booleans, run only on the empty path, against the same table the read uses.
unavailableunavailableemptyget_collection_health— a collector that stopped looks exactly like this."emptyThe second row is why the branching earns its code. A server whose entire history sits inside the window has no BEFORE, so it cannot show a regression however badly it regressed — and "no regressions" there is a confident wrong answer, not a missing one. Its advice also runs the opposite way from every other read in this series: widening the window makes the baseline shorter, so it says shorten, and both suites pin that it never says "Widen".
Deliberate differences from the viewer
LIMIT $5, default 50 — the viewer's hardcoded number) so a caller can ask for fewer or more. The gate and ranking are untouched, so the first 50 rows of any call are the viewer's 50. Validated withMcpHelpers.ValidateTop, which refuses out of range; truncation is observed by over-fetching one, not inferred from the count.collect.query_store_text([BUG] 3.4.0 query_store collector runs 37–100 min on Azure SQL DB (3.3.0 median: 4.8 s) — starves all other collectors #2150) and falls back to the fact rows; that table is a Darling-store construct with no Lite equivalent, so Lite has only the fallback. Same text, one hop closer — a difference in where the text comes from, not in which rows the read returns.SKU parity
Ported to Lite in the same change rather than added to the divergence ratchet. Census 105 → 106 tools, 80 → 81 shared, Darling-unique unchanged.
New tool class + reader live in their own files rather than being appended to
DarlingMcpDataTools/DarlingDataReader, which several changes are touching concurrently.How I verified
node --checkclean; census re-derived with the same scan the pin uses (106 / 81 / 25), recounted against the currentdevrather than the branch point.< $2and recent>= $2share one boundary so nothing is double-counted or dropped; the TVF gate,ORDER BY additional_duration_ms DESC, the four severity bands and all sevenNULLIFguards survive; the probe reads the same base table for both halves in one round trip; the collector DDL actually has every column the read projects.DARLING_TEST_PG) walks all four empty branches, asserts the four messages are four distinct sentences, then seeds a real regression: 1 ms → 4 ms with the baseline interval re-collected at a higher cumulative count, and assertsbaseline_exec_count == 90(the deduped final snapshot) rather than 140 (the sum). That single number is the dedup, and it is what the whole read leans on. Also asserts CRITICAL banding, +300%,additional_duration_ms == 600, the refused cap, and that a result exactly at the cap is not reported truncated.table(...), not an object literal.What I could NOT verify
Both test suites target
net10.0-windows— built here, run in CI.No live instance or real Query Store was read. The dedup and the interval arithmetic are verified against seeded rows, not against a real store with months of baseline at a 60-minute
INTERVAL_LENGTH_MINUTES. In particular I have not measured what an unbounded baseline scan costs on a large store — the read inherits the viewer's shape, which has shipped, but the viewer runs it against one server on demand and an agent may not.The
query_store_textjoin is exercised only through itsCOALESCEfallback: the seeded rows carryquery_texton the fact row, and nocollect.query_store_textrow is planted, so the first arm of that COALESCE is pinned in SQL but not executed.