Repository navigation
Long-query trace: query_hash 0 is treated as unknown (#5711) - #5721
Conversation
…rid header to Query ID (#5711)
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /* #5711: query_hash is NULL when the engine gave none (the trace's batch-level events carry 0); query_identity | ||
| is the key to group by: the real hash, else a fingerprint of the scrubbed statement text. */ | ||
| query_hash = QueryHashIdentity.KnownHash(r.QueryHash), | ||
| query_identity = r.QueryIdentity |
There was a problem hiding this comment.
Parity: the two SKUs spell "no identity" differently on the wire.
r.QueryIdentity here is the grid-facing property — LocalDataService.LongQueries.cs:179 ends it ?? "", because LongQueryCompletionRow.QueryIdentity is a non-nullable string for the DataGrid binding. Darling's row returns string? (DarlingLongQueryReader.cs:49) and its payload passes it straight through (DarlingMcpLongQueryTools.cs:148). McpHelpers.JsonOptions is new() { WriteIndented = false } with no null-ignore (McpHelpers.cs:63), so for one and the same stored row the same tool name emits:
- Lite:
"query_identity":"" - Darling:
"query_identity":null
This is not a corner case. A row has no identity when it has neither a usable hash nor statement text, and that is every attention row: the collector's own note says statement_text is rpc_completed.statement or sql_batch_completed.batch_text; NULL for attention (LongQueryCompletionsCollector.cs:198), and your own measurement table has attention at query_hash = 0 on all three versions. Attentions are an advertised part of this page ("plus attentions (cancels/timeouts)"), so both payloads carry the key with a different "unknown" word on most pages that contain a cancel.
Nothing pins it either way: McpPageContractTests.TheSameToolName_EmitsTheSamePageContractKeys_OnBothSkus compares only *_returned / oldest_returned_* / newest_returned_* / order (ContractKeys, McpPageContractTests.cs:157), so query_identity is outside its reach.
The nullable form is the one the class documents ("else by nothing (NULL), so a row with neither stays out of every group rather than joining one") — and "" is the fabricated-value shape the traps file warns about on the collector side ("End at END (NULL) and carry the NULL to the wire"). Call the shared rule directly in the payload and leave the ?? "" on the grid property only:
| query_identity = r.QueryIdentity | |
| query_identity = QueryHashIdentity.Identity(r.QueryHash, r.StatementText) |
| public sealed class DarlingMcpLongQueryTools | ||
| { | ||
| [McpServerTool(Name = "get_long_query_completions"), Description("Gets the SLOWEST long-running query completions in the window: rpc/batch completions over the trace's duration threshold, plus attentions (cancels/timeouts), ranked duration DESC, attentions (no duration) last. THE PAGE IS THE window's limit SLOWEST, NOT ITS NEWEST: truncated means the window held more, none slower than the page; oldest/newest_returned_event_time bound the slowest runs' ages, NOT how far the read reached. Collector is opt-in, OFF by default: empty can mean none in the window, or the collector was off — enable 'long_query_completions' in the schedule. <<GUIDE>> Gets the SLOWEST long-running query completions in the window, captured by the opt-in long-query trace: rpc/batch completions whose duration exceeded the trace threshold, plus attentions (client cancels / query timeouts), ranked by duration DESC with attentions (no duration) last. Shows duration, CPU, reads/writes, row count, result (OK/Error/Abort — Abort means the long query was cancelled), the statement text, and the calling session/app/login. THE PAGE IS THE window's limit SLOWEST, NOT ITS NEWEST: completions_returned is how many rows you got and truncated says the window held more than limit. Because the page is duration-RANKED, oldest_returned_event_time / newest_returned_event_time tell you how old the slowest runs are and say NOTHING about how far back the read reached — every row in the window was a candidate, so a truncated page still holds the window's slowest. Both SKUs keep the same population. The collector is OFF by default; if it returns empty, enable the 'long_query_completions' collector in the schedule.")] | ||
| [McpServerTool(Name = "get_long_query_completions"), Description("Gets the SLOWEST long-running query completions in the window: rpc/batch completions over the trace's duration threshold, plus attentions (cancels/timeouts), ranked duration DESC, attentions (no duration) last. THE PAGE IS THE window's limit SLOWEST, NOT ITS NEWEST: truncated means the window held more, none slower than the page; oldest/newest_returned_event_time bound the slowest runs' ages, NOT how far the read reached. Collector is opt-in, OFF by default: empty can mean none in the window, or the collector was off — enable 'long_query_completions' in the schedule. <<GUIDE>> Gets the SLOWEST long-running query completions in the window, captured by the opt-in long-query trace: rpc/batch completions whose duration exceeded the trace threshold, plus attentions (client cancels / query timeouts), ranked by duration DESC with attentions (no duration) last. Shows duration, CPU, reads/writes, row count, result (OK/Error/Abort — Abort means the long query was cancelled), the statement text, and the calling session/app/login. THE PAGE IS THE window's limit SLOWEST, NOT ITS NEWEST: completions_returned is how many rows you got and truncated says the window held more than limit. Because the page is duration-RANKED, oldest_returned_event_time / newest_returned_event_time tell you how old the slowest runs are and say NOTHING about how far back the read reached — every row in the window was a candidate, so a truncated page still holds the window's slowest. query_identity is the key to group rows by: it is the engine's query_hash when the engine gave one (batch-level events carry none, so query_hash is null there), otherwise text: plus a fingerprint of the statement text that does not merge statements differing only in a literal. Both SKUs keep the same population. The collector is OFF by default; if it returns empty, enable the 'long_query_completions' collector in the schedule.")] |
There was a problem hiding this comment.
The guide names two cases and the data has three — and the third is the one that collapses.
The new sentence (identical in Lite/Mcp/McpLongQueryTools.cs:19) says:
query_identityis the key to group rows by: it is the engine'squery_hashwhen the engine gave one …, otherwisetext:plus a fingerprint of the statement text …
There is a third outcome the sentence does not name: no identity at all. QueryHashIdentity.Identity returns NULL when the hash is unknown and there is no fingerprint, and that happens on two populations this page serves:
- Every
attentionrow. The collector:statement_textisrpc_completed.statement or sql_batch_completed.batch_text; NULL for attention(LongQueryCompletionsCollector.cs:198), and your measurement table putsattentionatquery_hash = 0on 2016, 2017 and 2019. So every cancel/timeout row this tool returns — a population the same guide advertises — carriesquery_identity: null. - Every withheld statement.
StatementFingerprintdeliberately returns NULL forSensitiveStatements.PlaceholderText, andlong_query_completions.statement_textreally is withheld at collection time (StatementScrubEventsTests.LongQueryCompletions_StatementText_IsWithheldAndThePlainStatementIsUntouched).
The class docs are explicit that this means "joins no group" — but the guide is the only thing the MCP caller reads, and it tells that caller the field is the grouping key. A client that follows it groups every attention in the window under one key and reports N cancels as N runs of one query: the exact collapse #5711 is about, surviving at the surface this PR documents. (GROUP BY on a null/empty key groups, it does not skip.)
Worth adding the third arm to both guides, e.g. after the text: clause: a row with neither a hash nor statement text — every attention, and any statement the collector withheld — has no query_identity and belongs to no group. The served head is untouched by this, same as the sentence already added.
| public const string FingerprintPrefix = "text:"; | ||
|
|
||
| /// <summary>True for NULL, empty, and any all-zero value (<c>0</c>, <c>0x0000000000000000</c>): no usable hash.</summary> | ||
| public static bool IsUnknown(string? hash) |
There was a problem hiding this comment.
The rule is right, and two live sites that key on a possibly-all-zero query_hash still don't have it.
The body's claim that nothing else groups by this column is accurate for long_query_completions. But query_snapshots.query_hash is stored as CONVERT(varchar(18), der.query_hash, 1) (QuerySnapshotsCollector.cs:162), which is 0x0000000000000000 whenever sys.dm_exec_requests.query_hash is zero — BACKUP / DBCC / WAITFOR / a request with no cached plan — and two readers treat that non-empty string as a real identity:
SameStatementPileupDetector.StatementIdentity(:461) gates only on!string.IsNullOrWhiteSpace(row.QueryHash), so every zero-hash session in a database groups under one identity andComputeIncidentIdfingerprints them as one pileup. (Its#4348carve-out already shows the authors reasoning about exactly this collapse for the text surrogate; the zero-hash door is still open above it.)AlertContextBuilders.LongRunningQueryIncidents(:267) keys onq.QueryHash ?? "", andAlertFingerprint.ForKeydrops only the empty key — so0x0000000000000000becomes a live incident key and pools unrelated hashless long-running statements' occurrence totals in that database.IncidentObservationCoverageTests.ALongRunningQueryWithNoHash_ProducesNoIncidentpins onlyQueryHash = null, not the all-zero spelling.
PerformanceMonitor.Alerting already references PerformanceMonitor.Common (PerformanceMonitor.Alerting.csproj:21), so the second one is QueryHashIdentity.KnownHash(q.QueryHash) ?? "" and nothing else. PerformanceMonitor.Analysis has no ProjectReference at all, so the detector cannot reach this class — which is the same boundary WithheldStatementMarker documents ("held here because the readers … cannot reference Common without changing the packages.lock.json files of every project that references them").
Two related notes while this file is new:
- That boundary makes this class a second copy of a definition that already exists:
StatementIdentity+NormalizeText+ShortHashinSameStatementPileupDetectoris the same rule (hash lower-cased, else lower-case/whitespace-collapse → SHA-256 → first 16 lowercase hex, prefixedtext:) and therefore emits byte-identical keys into the same keyspace. The body says a private equivalent exists "for a different input", which is fair, but the two are not cross-referenced and nothing pins them equal — the The MCP trend reader's tier ladder and hourly SQL were pinned EQUAL to the Storage definitions #3666 moved them to, which fails only after two copies drift — they are now aliases; and the Query Store duration chart, the one Performance Trends chart left untitled, says what served it and where the rollup's floor cut the window's head (#3653) #3684 shape, and worse than it, since that trap at least assumes a pin. The repo's own precedent for a forced copy isWithheldStatementMarker, which names its twin in the doc comment and is held to it byStatementMarkerReaderTests. A sentence here namingSameStatementPileupDetector.StatementIdentity(and one note of the one place they differ on purpose —text:emptythere, NULL here) would cost nothing and makes a future drift visible. If a literal-normalizing fingerprint ever lands on one of them, these keys diverge silently. span.Length > 2on line 43 means a bare"0x"skips the prefix strip and then fails the all-zero loop on itsx, soIsUnknown("0x")isfalseand"0x"is reported as a real hash.>= 2makes the span empty and the loop returntrue. Not reachable from this collector (the action shreds to a decimaluint64string), but the method is public and shared.
| different identities, so a group or filter on the identity does not collapse them. Reverting the reader to the raw | ||
| value gives QueryHash "0" and an empty-hash identity bucket, and fails here. */ | ||
| [Fact] | ||
| public async Task TheLiteReader_DoesNotCollapseZeroHashRowsOfDifferentStatements_IntoOneQuery() |
There was a problem hiding this comment.
Non-blocking, test parity: the two new files cover different halves, and the union is not either SKU's.
Darling's twin carries TheReaders_PassTheStoredHashThroughKnownHash_AndTheGridsBindTheIdentity, which source-pins the Darling reader, the Darling payload key and the ViewerServerTab.xaml column ({Binding QueryIdentity} + Tag="QueryIdentity"). Lite has no equivalent, so Lite's two user-visible halves of this change — the query_identity key in McpLongQueryTools.cs:118 and the rebound column in ServerTab.xaml:3309-3310 — are pinned by nothing in this PR; putting either back to QueryHash leaves both suites green. Conversely Darling's end-to-end store read is Assert.SkipWhen(DARLING_TEST_PG), so on CI only Lite's DuckDB read actually exercises a reader.
Also: neither file stages a row with no statement text, which is the attention shape (LongQueryCompletionsCollector.cs:198) and the one that produces the no-identity value the two SKUs spell differently. One row with query_hash '0' and a NULL statement_text, asserted through the payload on both SKUs, would pin the thing that is currently unpinned.
There was a problem hiding this comment.
Verdict: CHANGES REQUESTED
Claims
- VERIFIED — only the MCP tool, the Lite grid and the Darling viewer grid read this
query_hash, none groups/counts/joins it, and compose + notebooks do not use it for this table —MeasureCatalog.cs:377-379(dimensions are database_name/object_name/result),AlertNotebookEndpoint.Templates.QueryPlans.cs:49-51,wwwroot/js/read-fields.js:328-342. - VERIFIED — the helper's four members, incl. the withheld-marker carve-out —
QueryHashIdentity.cs:35, 60, 68, 100. - VERIFIED — Darling reader/tool:
query_hashNULL when unknown, newquery_identity—DarlingLongQueryReader.cs:130, 49;DarlingMcpLongQueryTools.cs:147-148. - VERIFIED — "Lite MCP: same two fields" — the same two keys at
McpLongQueryTools.cs:117-118(not the same values; see Divergence). - VERIFIED — grids:
QueryHash""when unknown, newQueryIdentity, column + filter bind it —LocalDataService.LongQueries.cs:140, 179;ViewerDataService.LongQueries.cs:148, 41;ServerTab.xaml:3309;ViewerServerTab.xaml:4007. The filter reflects on the bound name (ColumnFilterMatcher.cs:32) andQueryIdentityis not value-list-excluded, so the funnel still works on a computed property. - VERIFIED — the fingerprint reads already-scrubbed text, so the placeholder carve-out is live —
StatementScrubEventsTests.LongQueryCompletions_StatementText_IsWithheldAndThePlainStatementIsUntouched. - VERIFIED — no literal normalization —
QueryHashIdentity.cs:74-95lower-cases and collapses whitespace only. - VERIFIED — served head unchanged, budget unmoved — both sentences sit after
<<GUIDE>>; the 620-byte assert is onserved.Served(McpToolGuideHeads.LongQuery.cs:49), and the two descriptions are byte-identical. - VERIFIED — no test pinned the old header — the only
"Query Hash"in the test projects isRecordDetailRenderingTests:161, 185, an unrelated drill-down. - VERIFIED — case counts: Lite 10
InlineData+ 3Fact= 13; Darling 4Fact+ 1 live = 5. - UNVERIFIED — the 2016/2017/2019 measurement table — a live XE session on three instances. The code is version-independent as claimed, so the fix does not rest on it.
- UNVERIFIED — the revert check (3 failures) — needs a run; plausible from the three assertions (
QueryHash == "",Assert.Null, and the source match onKnownHash(). - UNVERIFIED — every suite/warning count — needs a build. Lite and the Darling viewer are WPF, so this Linux checkout cannot build them; the XAML halves are unverifiable here by construction.
- UNVERIFIED — "not measured:
rpc_completed, 2022, 2025" — self-declared gaps; nothing contradicts them. - REFUTED — "No MCP tool description text was changed, to leave the tool-guide pins alone." Both descriptions changed (
McpLongQueryTools.cs:19,DarlingMcpLongQueryTools.cs:35) and both tail pins were edited (McpToolGuideHeads.LongQuery.cs:62-63, both SKUs). The body's own "Guide and grid header" section says so — this test-plan line is stale.
Riskiest lines
Lite/Mcp/McpLongQueryTools.cs:118— reuses the grid-facing property, whose?? ""(LocalDataService.LongQueries.cs:179) makes Lite emit""where Darling emitsnull; everyattentionrow is such a row — UNPINNED (McpPageContractTests' key census compares only*_returned/order,:157).Darling/.../Mcp/DarlingMcpLongQueryTools.cs:35— the guide callsquery_identitythe grouping key and names two of its three outcomes; the unnamed one is the wholeattentionpopulation — pinned byMcpToolGuideHeadsLongQueryTests.Tail_KeepsEveryOriginalSentence_TheHeadCompressedAwayfor the words only, never their truth.Lite/Controls/ServerTab.xaml:3309— the rebound column, the user-visible half on Lite — UNPINNED (Darling's twin is source-pinned byLongQueryUnknownHashTests.TheReaders_PassTheStoredHashThroughKnownHash_AndTheGridsBindTheIdentity:84-87).
Traps near this diff
- "Lite and Darling ship in parity … a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit — the census in
McpPageContractTestscompares the two SKUs' payload keys by tool name for exactly this (#3594)." — applies: both SKUs got the key, not the same value vocabulary (""vsnull), and that census's regex (:157) does not reachquery_identity. - "
CASE WHEN interval > 0 THEN delta / interval ELSE 0 ENDspells 'unknowable' as 0 … End atEND(NULL) and carry the NULL to the wire." — applies in general form: the class states the unknown case as NULL (QueryHashIdentity.cs:99-101) and Lite's wire turns it into"". - "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — applies: the same normalization, the same SHA-256-first-16 and the same
text:prefix already exist atSameStatementPileupDetector.cs:459-472, 683-725, minting byte-identical keys into one keyspace with no cross-reference and no pin. The copy is forced (PerformanceMonitor.Analysis.csprojhas noProjectReference), but the precedent for a forced copy names its twin and is pinned (WithheldStatementMarker.cs:11-18). - "A key in the graph is not a key in the data; grep the collector that emits it." — applies: the collector says
statement_textisNULL for attention(LongQueryCompletionsCollector.cs:198), so the new key is absent for an advertised slice of the population; and the zero hash this rule rejects is still a live identity atSameStatementPileupDetector.cs:461andAlertContextBuilders.cs:267overquery_snapshots.query_hash(QuerySnapshotsCollector.cs:162). - "A pin that encodes the lie … Ask what a pin would fail ON." — applies weakly: the pins are real, but neither file stages the no-statement-text row, so the one value the SKUs disagree about is the one no test constructs.
- "Truncation inferred from a full page … Fetch
limit + 1; the flag isfetched.Count > limit." — does not apply: paging untouched, stilllimit + 1and> limit, enforced byMcpPageContractTests.EveryPagedTool_ObservesTruncation_AndNeverInfersIt_OnEitherSku. - "A JSON number above 2^53 is rounded by every JavaScript client … Emit 64-bit ids as strings." — does not apply: both fields are
stringon the wire (DarlingLongQueryReader.cs:46, 49) though the action shreds to auint64decimal (LongQueryCompletionsCollector.cs:292).
Divergence (blind read vs PR body)
Three places, all one direction — the body's account of the rule is accurate, its account of the rule's reach is wider than the diff's.
- "same two fields" is the same two keys, not the same two values. For a row with no usable hash and no statement text Darling sends
nulland Lite sends"". The body never cashes out the "else none" branch at the wire in any of its three descriptions ofIdentity. - The guide sentence omits the case the body's own table contains. The three
attentionrows measured are exactly the population whosestatement_textthe collector stores as NULL (LongQueryCompletionsCollector.cs:198), so they end with no identity — while the new guide tells a client thatquery_identityis the key to group rows by. A client that obeys pools every cancel/timeout under one key: #5711's collapse, reproduced at the new field. - "No existing SQL Server statement normalizer fit" is true as written and understates what landed.
SameStatementPileupDetectordoes not merely have "a private equivalent for a different input" — it has the identical definition, and the obstacle is a project-reference boundary, not the input.PerformanceMonitor.Alertingdoes reference Common (:21), so the one live, reachable all-zero-hash site (AlertContextBuilders.cs:267) could have adopted the rule here and did not.
Everything else the body claims about the diff matches my blind read, including what it marks as not done.
Reviewer checklist
- Run both suites — the test-plan counts, and specifically
McpPageContractTestson both SKUs plusMcpPayloadContractCensusTests. - Build Lite and the Darling viewer on Windows and open the Long Queries grid against real rows: WPF does not build on the CI Linux checkout, so the two rebound columns, the
Query IDtooltip and the filter over the computed property have no automated cover in this PR. - In that grid, look at a stored
attentionrow —QueryIdentityis""there; confirm the blank cell and the funnel's blank entry read as intended. - Call
get_long_query_completionson both SKUs over a window holding anattentionand diff the raw JSON forquery_identity— this is the""vsnullsplit, and one live call each settles it. - Scope call on
AlertContextBuilders.LongRunningQueryIncidents/SameStatementPileupDetector: whether0x0000000000000000is pooling incidents in the field needs a live store running concurrent BACKUP/DBCC/maintenance in one database.
…ull not empty, pin Lite grid column (#5711) The guide now says an attention row carries no statement text, so its query_identity is null and a null identity is never a group key. Lite's MCP path reads a null-preserving McpQueryIdentity (the grid keeps its empty string), so both apps emit null for a row with no identity. The payload census reaches query_identity, a Lite behavioural test and a Darling live case stage the no-text zero-hash row, and the Lite grid's Query ID column binding is source-pinned like Darling's.
…e long-running-query incident key (#5711) An all-zero hash pooled unrelated requests into one pileup group and one alert incident. Both sites now key such a row by the statement text fingerprint, or leave it out when there is no text. The detector's copy of the hash test and fingerprint is pinned equal to QueryHashIdentity over one corpus.
|
Follow-up pass over This commit is a direct, well-targeted fix for all three findings from the prior review round:
New regression coverage for the specific No parity gap and no new trap-shaped defect found in this range. (Build verification was not possible here — |
There was a problem hiding this comment.
Verdict: LGTM
Claims
- VERIFIED — "Attentions have no identity: the guide sentence in both tools now says so" — both
<<GUIDE>>tails gain the identical sentence (Lite/Mcp/McpLongQueryTools.cs:19,Darling/.../Mcp/DarlingMcpLongQueryTools.cs:35). - VERIFIED — "both tail pins extended" —
Lite.Tests/McpToolGuideHeads.LongQuery.cs:64-65andDarling/Darling.Tests/McpToolGuideHeads.LongQuery.cs:66-67each assert the new sentence. - VERIFIED — "Lite's MCP path now reads McpQueryIdentity" —
Lite/Mcp/McpLongQueryTools.cs:118(query_identity = r.McpQueryIdentity), backed byLite/Services/LocalDataService.LongQueries.cs:179-181(QueryIdentity => McpQueryIdentity ?? "";McpQueryIdentitynullable, no fallback). - VERIFIED — "a new source fact pins that neither SKU spells the no-identity case as ''" —
Darling.Tests/McpPageContractTests.cs:280-296,LongQueryTool_StatesANoIdentityRowAsNull_OnBothSkus_NotAsAnEmptyStringassertsDoesNotContain("?? \"\"")on both tool bodies and regex-matches theMcpQueryIdentitydeclaration. - VERIFIED — "Lite has a DuckDB wire test and Darling a live case for a no-text zero-hash row" —
Lite.Tests/McpPageContractTests.cs:488-503(GetLongQueryCompletions_ANoTextZeroHashRow_EmitsNullQueryHashAndNullQueryIdentity) andDarling.Tests/McpPageContractTests.cs:1523-1533, the latter inside thelive-postgres-gatedMcpPageContractLivePostgresTestsclass. - VERIFIED — "Lite's rebound grid column is source-pinned" —
Lite.Tests/LongQueryUnknownHashTests.cs:86-101,TheLiteGrid_BindsTheIdentity_UnderTheQueryIdHeader, checked directly againstLite/Controls/ServerTab.xaml:3309-3310(binding,Tag="QueryIdentity",Text="Query ID", noQueryHashbinding in the grid). - UNVERIFIED — the stated test-run counts and "0 warnings" build claims for this round — this is a read-only Linux checkout and
dotnet testrequired an approval this session did not grant; Lite and the Darling viewer are WPF and would not build here regardless.
Riskiest lines
Lite/Mcp/McpLongQueryTools.cs:118— the exact line the prior round flagged as emitting""instead ofnull— pinned byLongQueryTool_StatesANoIdentityRowAsNull_OnBothSkus_NotAsAnEmptyString(source) andGetLongQueryCompletions_ANoTextZeroHashRow_EmitsNullQueryHashAndNullQueryIdentity(behavioral, DuckDB).Lite/Services/LocalDataService.LongQueries.cs:181— the new nullableMcpQueryIdentity; a future edit that re-adds?? ""here would silently regress the fix — pinned by the same two tests above (the regex in the Darling census matches the exact declaration, and the behavioral test would fail on the fallback).Lite/Controls/ServerTab.xaml:3309— the rebound grid column, no automated UI build on this checkout — pinned (source-level) byTheLiteGrid_BindsTheIdentity_UnderTheQueryIdHeader; still UNPINNED at the executed-UI level (WPF, Windows-only).
Traps near this diff
- "Lite and Darling ship in parity … a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit (#3594)." — does not apply to this commit: the parity gap the prior round found (Lite
""vs Darlingnull) is exactly what this commit closes, and both tool guides now carry byte-identical new text (McpLongQueryTools.cs:19,DarlingMcpLongQueryTools.cs:35). - "A pin that encodes the lie … ask what a pin would fail ON (#3699)." — does not apply: the new census test (
McpPageContractTests.cs:280-296) fails on the actual regression (a reintroduced?? ""or a revertedr.QueryIdentity), not on an unrelated proxy. - "A roster-based census does not see the file it does not roster (#3699)." — does not apply: the new null-vs-empty pin explicitly names both tool files by path rather than walking a roster, so there is no file it could silently miss.
Divergence (blind read vs PR body)
None — the PR body's "Review round 1" section names exactly the three items this commit fixes (attention guide text, Lite ""→null, Lite grid column pin), and the diff implements all three with matching test coverage; no claim about this commit's mechanism or test coverage was contradicted by the tree.
Reviewer checklist
- Run
Lite.TestsandDarling.Tests(LongQueryUnknownHashTests,McpPageContractTests,McpToolGuideHeadsLongQueryTests) to confirm the stated pass counts — blocked here by sandboxeddotnet test. - Build Lite and the Darling viewer on Windows and open the Long Queries grid against a real
attentionrow to confirm theQuery IDheader, tooltip, and blank cell render as intended — WPF does not build on this Linux checkout. - Run the
live-postgres-gatedMcpPageContractLivePostgresTestswithDARLING_TEST_PGset to execute (not just compile) the new Darling attention-row live case.
An unknown hash (NULL, empty or all-zero) emits no long-running-query incident, as a NULL hash always did. A known hash keys exactly as before. Zero-hash requests no longer share one incident.
1 participant
Claude posting for Erik Darling.
Closes #5711.
Why
The opt-in long-query trace stores the
sqlserver.query_hashaction on each completion. The issue saw it as0on two aborted completions and asked whether that is only aborted ones. It is not: it is every completion the trace captures.What I measured
Throwaway Extended Events session with the trace's own event list (
rpc_completed,sql_batch_completed,attention) and its nine-action list, duration filter 0. Three normal batches, and three batches run with a 2 second client timeout (oneWAITFOR, two CPU-bound cross joins; on SQL Server 2016 the last one finished inside the timeout, so it counts as normal there), read from the ring buffer. The probe session was dropped on each instance and confirmed gone (0 rows insys.server_event_sessions).sql_batch_completed, normal0sql_batch_completed, aborted0attention0sql_batch_completed, normal0sql_batch_completed, aborted0attention0sql_batch_completed, normal0sql_batch_completed, aborted0attention0Normal completions are 0 too, on all three versions.
query_hashis a statement-level value and these events are batch-level. Not measured:rpc_completed(the sqlcmd client sends batches only); the issue's own observation covers its aborted case, and it is the same batch-level action. Also not measured: SQL Server 2022 and 2025 (they hold real data and were off limits). The fix does not depend on the version: any all-zero, empty or NULL value is unknown.What readers did, and do now
Every reader of
long_query_completionsonly displaysquery_hashper row (MCP tool JSON, the Lite grid, the Darling viewer grid). None groups, counts or joins by it, and the compose catalog and notebook templates do not use it for this table. So the harm was: the MCPquery_hashfield and the grid column read0on every row, and filtering or grouping on it (the grid's column filter, or an MCP client grouping on the field) put every row in one bucket.Now:
PerformanceMonitor.Common/QueryHashIdentity.cs:IsUnknown(NULL, empty,0,0x0000000000000000),KnownHash,StatementFingerprint(SHA-256 of the lower-cased, whitespace-collapsed stored statement text, first 16 hex characters, prefixedtext:; none for the withheld-statement marker so withheld statements do not collapse into one group), andIdentity(the real hash, else the fingerprint, else none). No existing SQL Server statement normalizer fit:SameStatementPileupDetectorhas a private equivalent for a different input, andPgTargetDrillDownCollector.Fingerprintis a PostgreSQL display string.get_long_query_completions:query_hashis NULL when unknown; newquery_identityis the key to group by.get_long_query_completions: same two fields.QueryHashis "" when unknown; newQueryIdentityproperty; the Query Hash column and its filter now bindQueryIdentity, so it shows the real hash or atext:fingerprint, and filtering groups by it.Test plan
LongQueryUnknownHashTestsin Lite.Tests (13 cases): the helper's unknown rules, distinct identities for zero-hash rows of different statements, same statement in different case and spacing keys the same, and the Lite reader end to end over DuckDB (four stored rows, three with a zero hash, read back as three distinct identities).LongQueryUnknownHashTestsin Darling.Tests (5 cases): the MCP row and viewer row identities, source pins on the reader, the MCP payload and the grid binding, and a live PostgreSQL read throughDarlingLongQueryReader(4 stored rows, NULL hash and one identity per statement for the zero-hash ones). Joins thelive-postgrescollection.KnownHashcalls removed, the Lite reader test, the live store read and the source pin fail (3 failures); restored, all pass.*Census*and*Ratchet*class (622, 0 failed),-trait Stage=Guard(1584, 0 failed), plus the long-query viewer, column-filter andMcpPayloadContractCensusTestsclasses (278 passed),LivePostgresCollectionHygieneTestsandQueryEventsDatabaseFilterLiveTests(passed) against a throwaway PostgreSQL cluster, stopped afterwards.*Census*and*Ratchet*class (67),-trait Stage=Guard(496), plus the MCP page-contract, window-notice, database-filter, grid sort, tool-guide, stored-copies and data-start classes (401): 0 failed.Lite.Testsbuilt with 0 warnings;Darling.Testsshows one existing xUnit2013 warning inDarlingOperatorGroupScriptTests.cs, not in this diff.get_long_query_completionsdescriptions changed (guide tail, after<<GUIDE>>), and bothMcpToolGuideHeads.LongQuery.cstail pins were extended to match; the served head and the 620-byte budget are untouched.LongQueryUnknownHashTests+McpPageContractTests+McpToolGuide*+ every*Census*/*Ratchet*class (187, 0 failed) and-trait Stage=Guard(496, 0 failed); Darling.Tests the same set (770, 0 failed, 2 skipped) andStage=Guard(1584, 0 failed). Revert check: with the Lite MCP path back on the grid'sQueryIdentity, the new Lite wire test and the grid pin both fail.attentionrow read back throughDarlingMcpLongQueryToolsemits nullquery_hashandquery_identity) sits in thelive-postgrespage-contract test, which skips withoutDARLING_TEST_PG; it compiled but was not run on this laptop (CI decides). The Darling source census over the same fields did run.Changelog
SECTION: Fixed
ENTRY: - Long-query trace rows no longer all show query_hash 0 as one query ([#5721]) - The long-query trace stores the
query_hashaction, and on SQL Server 2016, 2017 and 2019 it is 0 on every completion it captures (batch completions normal or aborted, and attentions), so the MCPget_long_query_completionsfield and the Query Hash column in Lite and the Darling viewer read 0 on every row and grouped every row together. A hash of 0, empty or missing is now treated as unknown: the MCPquery_hashis null, and a newquery_identityfield (and aQueryIdentitycolumn value in the two grids, which the column filter now uses) is the real hash when there is one, else a fingerprint of the scrubbed statement text startingtext:, so rows of different statements stay apart. The same all-zero hash no longer pools unrelated requests elsewhere: BACKUP, DBCC and similar requests that carry a zeroquery_hashin the active-query snapshots no longer count as one statement in the same-statement pileup finding (they are keyed by their statement text instead, or left out when they have none), and a long-running query whose hash is unknown (null or zero) emits no long-running-query alert incident, as a null hash always did, so those requests no longer share one incident.REF:
[#5721]: #5721
Guide and grid header
attentionrow (client cancel or timeout) carries no statement text, so itsquery_identityis null and it cannot be grouped by it, and that a nullquery_identityis never a group key. Confirmed against the collector first:LongQueryCompletionsCollector.csdocumentsStatementTextas NULL for attention and the trace's hash is 0. Both tail pins assert the new words.query_identityis now null, not"", for a row with no identity, as Darling's is.LocalDataService.LongQueries.cskeeps the grid'sQueryIdentity("", blank cell) and gains a null-preservingMcpQueryIdentitythat onlyMcpLongQueryTools.csreads.get_long_query_completionsguides (LiteMcpLongQueryTools.cs, DarlingDarlingMcpLongQueryTools.cs) now carry the same sentence in the guide tail (after<<GUIDE>>):query_identityis the key to group rows by; it is the engine'squery_hashwhen the engine gave one (batch-level events carry none, soquery_hashis null there), otherwisetext:plus a fingerprint of the statement text that does not merge statements differing only in a literal. The served head is unchanged, so the tools/list byte budget did not move and no budget comment was added. BothMcpToolGuideHeads.LongQuery.cstail tests now pin the sentence.Lite/Controls/ServerTab.xamlandViewerServerTab.xamlis now headedQuery ID, with the header TextBlock tooltip "The engine's query hash, or text: plus a fingerprint of the statement when the engine gave none." (same markup as the other tooltipped headers). No test pins the old header text.Stage=Guard496 passed. Darling.Tests: the same class set 910 passed (8 skipped),Stage=Guard1584 passed. Zero failures. Full suite and live PG classes not run on this laptop.Review round 1
Part A (this commit, 7a23309; part B, the duplicate fingerprint and the zero-hash use in alerting, is a separate commit):
""where Darling emitted null: Lite's MCP path now readsMcpQueryIdentity. The census in Darling'sMcpPageContractTestsnow reachesquery_identity(ContractKeys), a new source fact pins that neither SKU spells the no-identity case as"", Lite has a DuckDB wire test and Darling a live case for a no-text zero-hash row.LongQueryUnknownHashTests.TheLiteGrid_BindsTheIdentity_UnderTheQueryIdHeader(theQueryIdentitybinding and filter tag, theQuery IDheader, noQueryHashbinding).Part B (commit 659f96a; the long-query readers, guide text and grid pins are part A above):
PerformanceMonitor.Analysiscan take aProjectReferencetoPerformanceMonitor.Commonwith no cycle (Common references nothing; Analysis is referenced by Notifications, Lite and Darling). It still cannot take it in this PR:performancemonitor.analysisis aProjectentry in every committed lock file that walks the graph, and that includesdeprecated/Dashboard/packages.lock.jsonanddeprecated/Dashboard.Tests/packages.lock.json, which CI restores with--locked-mode. Moving the definition down fails the same way (Common would gain an Analysis entry), and nothing underdeprecated/may change here. So the two copies stay, and the precedent for a forced copy is followed (WithheldStatementMarker): each names its twin (QueryHashIdentityclass remarks;StatementIdentity,IsUnknownHashandNormalizeTextinSameStatementPileupDetector) andLite.Tests/QueryHashIdentityParityTests.csruns both over one corpus (15 hashes x 21 statements: whitespace, case, comments, literals, the withheld marker with padding, empty, null) and assertsQueryHashIdentity.Identity(hash, text)equalsSameStatementPileupDetector.StatementIdentityfor every pair, which covers the zero-hash test and the fingerprint together. A second test asserts the corpus reaches a real hash, atext:fingerprint and a NULL, so it cannot go vacuous.SameStatementPileupDetector.StatementIdentity(was line 461): any non-blank hash was the identity, so every0x0000000000000000request in one database (BACKUP, DBCC, maintenance) was one "statement" and could fire a same-statement pileup. Now an unknown hash takes the text-fingerprint path (the one that already existed for NULL), and a row with no text, or only the withheld marker, has no identity and is left out. Behavior change on the NULL path too: a null-hash row with empty text used to share onetext:emptygroup; it is now left out, the same as the marker.AlertContextBuilders.LongRunningQueryIncidents(was line 267): the dedup key wasq.QueryHash ?? "", so every zero-hash long-running request on a server and database folded into one incident (one fingerprint, one cooldown, one occurrence count). The original rule (Null hash -> no incident) now extends to every unknown hash (QueryHashIdentity.IsUnknown: null, empty or all-zero): such a query emits no incident, whatever its text. A known hash keys exactly as before (the raw hash, pinned againstAlertFingerprint.ForKeydirectly). The alert card itself is unchanged; only which queries get an incident moves.AlertContextBuildersTests.BuildLongRunningQueryContext_NullQueryHash_EmitsNoIncidentis restored exactly as on dev (it also has a deprecated Dashboard copy that must keep passing; nothing underdeprecated/changed). New there:..._ZeroQueryHash_EmitsNoIncidentand a two-statements-under-a-zero-hash test that gets no incident and no pooled one.QueryHashIdentityParityTests, 21 cases): the parity corpus; zero-hash rows of one statement share an identity, of different statements do not, with no text have none; the pileup detector with three unrelated statements under a zero hash does not fire, three sessions on one statement under a zero hash still fire, zero-hash no-text rows do not fire, a real hash still groups on the hash; the alert emits no incident for a query with a null, empty or zero hash whatever its text, a zero-hash row does not hide a known-hash row, a real hash keys as before. Revert check: with both source files restored to their previous text, 14 of the 21 fail; with the change, all pass.DarlingOperatorGroupScriptTests.cs). Lite.Tests Census, Ratchet, Pileup, Alert, QueryHashIdentity, LongQuery, McpPageContract and McpToolGuideHeads classes: 1002 passed, 0 failed. Darling.Tests AlertEngine, Census, Ratchet, SameStatementPileup, LongQueryUnknownHash, McpPageContract and McpToolGuideHeads classes: 1058 passed, 6 skipped, 0 failed.Stage=Guard: Darling 1584 passed; Lite (ran with the class filters, 69). Full suites and live PG classes not run on this laptop.0x0000000000000000pools incidents in the field (the review's scope call); that needs a live store running concurrent BACKUP/DBCC in one database.Round 1, part C (d464ca7): the long-running-query alert follows the original rule for every unknown hash (null, empty or zero emits no incident); this replaces the text-keyed incident from part B, and a known hash is unchanged. Verified on this change: Lite.Tests AlertContextBuilders, QueryHashIdentityParity, LongQueryUnknownHash, Pileup, Census (67) and
Stage=Guard(496) all pass, 0 failed; Darling.Tests Pileup, LongQueryUnknownHash, Census (545), Ratchet (15) andStage=Guard(1584) all pass, 0 failed. Revert check: withq.QueryHash ?? ""put back, 8 of the 67 AlertContextBuilders and QueryHashIdentityParity cases fail, among them both new zero-hash tests.