Serve the Query Heatmap on the viewer's own 5-minute bins (#2484) - #2509
Conversation
The last of the ten viewer surfaces with no /api/read endpoint. The interactive
plot stays desktop-only by design; the read behind it is portable, and a bucketed
table is the same answer.
The bucketing is the viewer's, not a new one. Its bin width turned out to be a
CONSTANT — INTERVAL '5 minutes', hardcoded whatever range is on screen, not
derived from the window length — so there was nothing to reproduce, only a
default to honor. bucket_minutes exposes it and defaults to 5, because a browser,
an agent and a desktop pointed at the same server over the same window must not
draw different pictures. The seven log-magnitude bands are the viewer's too.
Making the width a parameter exposed a cross-SKU divergence the default hid.
Postgres date_bin takes an explicit origin and the viewer passes the Unix epoch;
DuckDB's time_bucket defaults to 2000-01-03 instead. The two origins are
15,780,960 minutes apart, which 5 divides exactly — so the default agrees by luck
— but 7 does not, and the two SKUs would have binned the same row a minute apart
the first time anyone passed an odd width. Lite's read now passes the epoch
explicitly. Verified identical across {1, 5, 7, 13, 60, 90, 360, 1440}-minute
strides on PostgreSQL 17 and DuckDB 1.5.5.
query_stats is a PERIODIC table, not an edge table: the collector writes rows
every cycle for whatever is in the plan cache, so an existence probe on the data
is the right denominator here — the opposite of what #2508 corrected for blocking
and deadlocks. Zero cells is three states: never collected (unavailable, nobody
looked), nothing in the window (empty, widen it), and one only this read has —
captures exist and every one recorded a zero execution delta, which is an idle
server rather than a broken one, where "widen the window" would be advice pointed
at the wrong problem.
The cap is on CELLS and it cuts the OLDEST bins, so a capped call keeps the recent
end of an incident window. That cut can land inside a bin, so the partial column
is dropped rather than returned with holes: a column missing its low buckets reads
as "nothing fast ran then" rather than "we stopped looking".
Also closes out uat-onboarding §3.4, whose "data with no read endpoint at all"
list named the heatmap and is now empty. Its counts were seven tools stale; they
are re-derived rather than incremented — 90 read endpoints of 109 tools, 69 of
them reached by the per-server page.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| cells = cells.OrderBy(c => c.TimeBucket).ThenBy(c => c.BucketIndex).ToList(); | ||
|
|
||
| var labels = LocalDataService.HeatmapBucketLabelsFor(parsedMetric); | ||
| var end = DateTime.UtcNow; |
There was a problem hiding this comment.
end (and therefore window_start/window_end in the returned JSON) is captured after dataService.GetQueryHeatmapCellsAsync has already run, but the query itself computed its own [start, end] independently via GetTimeRange(hoursBack, ...) (two separate DateTime.UtcNow calls) at the top of GetQueryHeatmapCellsAsync. The window reported to the caller is not the window that was actually queried — they can drift by however long the DuckDB call takes to acquire its read lock and run, which under active collection (the writer lock other collectors take) can be non-trivial, not just microseconds.
This is a real divergence from two things already established in this same PR:
- Darling's twin (
DarlingMcpQueryHeatmapTools.GetQueryHeatmap) computesstart/endonce, before callingGetQueryHeatmapAsync, and reuses that same pair for both the query parameters and the JSON output. - This very file's
GetTopQueriesByCpu(line ~39) explicitly hoistsnowUtcbefore the async call for exactly this reason, with a comment calling out the numerator/denominator window-skew risk of not doing so.
Suggest hoisting var end = DateTime.UtcNow; (and deriving start from it) to before the dataService.GetQueryHeatmapCellsAsync(...) call, and threading that same start/end through so the reported window matches the window that was actually queried — mirroring Darling's implementation.
|
Reviewed the query-heatmap read (Darling Overall this is well-executed and the two SKUs are in tight parity — the bucketing constant, magnitude bands, metric expressions, validation messages, and empty-state sentences all match verbatim as the PR description claims, and I verified the truncation/no-partial-column logic by hand against the test cases (including the single-bin-fallback edge case). No SQL injection surface (metric goes through an enum switch, One real bug found — left as an inline comment on Everything else I checked — parameter wiring in |
Review catch. Lite's GetQueryHeatmap took its window_start/window_end from a FRESH DateTime.UtcNow after the DuckDB call returned, while the window the query actually ran with is computed independently and earlier inside GetQueryHeatmapCellsAsync. The reported window drifted by however long the read took — on the one read whose entire output is a time axis, so a window that disagrees with the bins underneath it is worse here than almost anywhere. Hoisted before the call, the same move GetTopQueriesByCpu in this file already makes and for the same reason. Darling's twin never had the bug: it computes start/end once and passes them into the query. Pinned on BOTH SKUs rather than only the one that was wrong — the invariant is that the reported window spans exactly hours_back and brackets every bin returned, which is now asserted in Darling's live test too so its correctness-by- construction cannot quietly regress. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Good catch on the window skew — fixed in bfef9cb.
Pinned on both SKUs rather than only the one that was wrong: the reported window must span exactly |
|
Reviewed the diff against Notes from the review:
No inline comments filed — nothing rose to the level of a required change. |
#2504 landed as_of across 57 reads while this branch was open. The heatmap is a windowed read, so it takes the anchor too — on the same shared McpHelpers.AsOfDescription constant as the rest, not a second convention. It earns the parameter more than most reads do. A heatmap IS a time axis, so "the four hours ending Tuesday 03:00" is the shape of every question anyone brings to it, and widening hours_back until an old incident falls inside is not the same question here even in the weak sense: the extra hours arrive as extra COLUMNS, which push the incident's own columns past the cell cap. Proved by CONTENT rather than by the parameter existing, on both SKUs. #2495's own failure mode is a tool that advertises as_of, validates it, refuses a bad anchor correctly and then queries now — eight of those shipped past a green suite during #2504 — so both tests seed rows outside every default window and assert the anchored call returns them while the same-LENGTH default-anchored call does not. Neither tool names DateTime.UtcNow, and Lite threads the resolved instant into GetQueryHeatmapCellsAsync and GetQueryHeatmapCoverageAsync rather than letting either compute its own. That also subsumes the window-skew fix from the previous commit: one resolved instant now decides the query window, the coverage probe's window and the window reported back. Conflicts, and how they were taken: - DarlingWebEndpoints catalog + dispatch: dev's as_of on the neighbouring reads, plus PAsOf() / as_of: AsOf(c) on get_query_heatmap. - Both instructions tables: dev's rows win (they carry as_of); the heatmap row is re-inserted in place with as_of appended. - The census sentence did not conflict, but it was RECOUNTED rather than trusted: scanning McpServerTool(Name = "...") gives 109 / 84 / 25, which is what it says. The uat-onboarding counts were re-derived the same way and are unchanged at 90 read endpoints, 109 tools, 69 reached by the per-server page. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review: get_query_heatmap (#2484)Went through the new No blocking issues found. Specifically checked and confirmed clean on:
One non-blocking observation on the truncation heuristic (identical in both Nice work on the cross-SKU verification (real Postgres 17 + real DuckDB 1.5.5, 32 stride combinations) — that's exactly the kind of check this bucketing logic needed. |
The merge that took as_of left the tool's own parameter-list pin describing the pre-merge signature, so CI failed on exactly the thing that pin exists to catch — which is the pin working, not a defect in it. Expected list updated, and the anchor is now pinned by IDENTITY as well as by name: its description must BE McpHelpers.AsOfDescription, not merely mention an anchor. That is the constant's whole reason for existing — the same parameter described two different ways on two SKUs is a divergence no other test would see. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed the diff (Darling PG reader/tools, Lite DuckDB reader/tools, web panel, docs) against CONTRIBUTING.md and the Lite/Darling parity requirement. Summary: Correctness — Walked both Parity — Security — Both SQL builders are fully parameterized ($1–$6 / $1–$N); the only string-interpolated piece ( Style — New Postgres/DuckDB SQL is consistent with the rest of the codebase's existing (non-T-SQL) query style in this project; One non-blocking observation, not specific to this PR: the "top query in a cell" tie-break ( No blocking issues found. |
Both classes pin their tool set EXACTLY - seven blocking tools, three health tools - so adding a tool to either is meant to fail here until the pin is told about it. That is the pin working, not a defect in it, and it is the reason a tool cannot be added to a class without also being given a parameter contract and a Gemini-clean schema assertion. Each new tool also gets its anchor pinned by IDENTITY rather than by name: the as_of description must BE the shared constant, following the correction #2509 made for the heatmap. get_lock_wait_trend carries McpHelpers.AsOfDescription; get_daily_summary_range carries AsOfDaysDescription, and that pin also asserts the two constants differ, that the day-grained one does not name hours_back, and that it does name days_back - because the whole reason for a second constant is that a parameter description naming a parameter the tool does not have is worse than a generic one. LockWaitTrendSql gets its own pin on the three properties that are real defects if they drift: the LCK filter, the LAG partitioned BY WAIT TYPE (without which one wait type's cadence divides another's delta), and the CAST to double before the division. Also silences two xUnit2013 warnings the new range tests raised. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Closes the last item on #2484. The other nine shipped in #2494, #2496, #2497, #2498, #2503, #2507 and #2508.
The interactive plot stays desktop-only by design (#2484 group (c)) — there is no
heatmapviz in the web page's vocabulary and inventing a fifth one is not what the issue asked for. The read behind it is portable, and a bucketed table is the same answer: one row per (time bin x magnitude bucket) cell, with the most-executed query in each.What it adds over every other query read is the time axis.
get_top_queries_by_cpuranks a whole window and cannot show that the window had a quiet half and a bad half, which is the first thing anyone asks about an incident that has already ended.What the viewer's bucketing turned out to be
A constant, not a derivation.
ViewerDataService.QueryHeatmap.csbins with a literalINTERVAL '5 minutes'whatever range is on screen — it is Lite'stime_bucket(INTERVAL '5 minutes', collection_time)ported todate_bin, and neither one looks at the window length. So there was no derivation to reproduce, only a default to honor:bucket_minutesand defaulting to exactly that. A browser, an agent and a desktop pointed at the same server over the same window draw the same grid unless the caller asks for something else. The result echoesbucket_minutes_matches_desktop_viewerso a consumer can tell at a glance whether it is looking at the viewer's picture.< 1 / 1-10 / 10-100 / 100-1K / 1K-10K / 10K-100K / > 100K, labelled per metric family (milliseconds for duration and CPU, plain counts for the other three). The labels ship with every result — a barebucket_indexis unreadable.ViewerDataService.HeatmapMetricExprand Lite'sGetMetricColumn, all five of them.The SQL is the viewer's verbatim apart from two things a desktop chart does not need: the bin width is a bound parameter, and the tail carries
ORDER BY time_bin DESC+LIMITso a capped call keeps the recent end of the window.The divergence the parameter exposed
Making the width a parameter surfaced a cross-SKU bug the default was hiding.
Postgres
date_binrequires an origin and the viewer passes the Unix epoch. DuckDB'stime_bucketdefaults to a different origin — 2000-01-03. The two are 15,780,960 minutes apart, and 5 divides that exactly, so the default agrees by luck. 7 does not:date_bintime_bucket(default origin)10:05:0010:05:0010:02:0010:01:00The first caller to pass an odd
bucket_minuteswould have had Lite and Darling bin the same row a minute apart. Lite's read now passes the epoch explicitly. Verified identical across{1, 5, 7, 13, 60, 90, 360, 1440}-minute strides against PostgreSQL 17 and DuckDB 1.5.5 — 32 combinations, zero mismatches. Lite's WPF chart keeps its origin-less form deliberately: it only ever asks for 5 minutes, where the two agree.Which kind of table this is, and why the empty branches are what they are
query_statsis a PERIODIC table, not an edge table. The collector writes rows every cycle for whatever is in the plan cache, whether or not anything interesting happened. So an existence probe on the data is the right denominator here — the exact opposite of what #2508 corrected for blocking and deadlocks, where zero rows is the healthy answer and a data probe sends someone to fix collection that works. The probe readsv_query_stats, the same relation the read itself uses, so it cannot disagree with the read about which rows exist.Zero cells is three states:
unavailable— no rows for this server at all. Nobody looked; go find out why collection is not running.empty— rows exist outside the window, none in it. Widenhours_back, or checkget_collection_health; a collector that stopped looks exactly like this.empty— captures exist in the window and every one recorded a zero execution delta. This is the branch only this read has: collection is healthy and the server is idle. Telling that caller to widen the window would be advice pointed at the wrong problem. The same sentence covers adatabase_namefilter that matched nothing collected, and the two-cycle warm-up that delta collection needs.All three are pinned on both SKUs, asserted to be three different sentences, and the three shared sentences are added to
McpMissMessageParityPinTestsso they cannot drift a dash apart between the trees.The cap, and the partial-column problem
limitcaps cells (default 500) and is validated byMcpHelpers.ValidateTop— out of range is refused, never clamped. Over-fetch by one, so truncation is observed rather than inferred.Rows come back newest-bin-first so the cap cuts the oldest end, which is what anyone looking at an incident wants. That creates a hazard nothing else on this surface has: the cut can land in the middle of a bin and hand back a column missing its low buckets, which reads as "nothing fast ran then" rather than "we stopped looking" — a quiet wrong answer that a grid makes very easy to believe. So the partial column is dropped, and
first_time_bin/last_time_binsay which slice of the window actually came back. (It is kept only when it is the only column, i.e. a cap below one bin's seven cells, where there is nothing to fall back to.)bucket_minutesis the lever to reach for before the cap: a 7-day window at 5-minute bins is 2,016 columns, and widening the bin covers the window in fewer cells.An unknown
metricis refused, not silently turned into duration — a caller who asked for CPU and got elapsed time would read the wrong grid with nothing to tell them so.Also in here
docs/uat-onboarding.md§3.4 listed "data with no read endpoint at all", named the query heatmap in it, and said no amount of web work reaches it. That list is now empty, so it says so. Its counts were seven tools stale (the whole #2484 wave landed without them moving); they are re-derived from the source rather than incremented — 90 read endpoints out of 109 tools, 69 of them reached by the per-server page. The "desktop things a web imitation would be worse than" paragraph keeps the heatmap, narrowed to the interactive plot.Both SKUs, not the divergence ratchet. Census recounted by scanning the
McpServerTool(Name = ...)attributes: 108 → 109 tools, 83 → 84 shared, 25 Darling-unique unchanged.The window anchor (#2495 / #2504)
as_oflanded across 57 reads while this was open, and the heatmap now takes it too — on the sharedMcpHelpers.AsOfDescriptionconstant, not a second wording of the same idea.It earns the parameter more than most reads do. A heatmap is a time axis, so "the four hours ending Tuesday 03:00" is the shape of every question anyone brings to it. And widening
hours_backuntil an old incident falls inside is not the same question here even in the weak sense the general argument makes: on this read the extra hours arrive as extra columns, which push the incident's own columns past the cell cap.#2495's own failure mode is a tool that advertises the anchor, validates it, refuses a bad one correctly — and then queriesDateTime.UtcNowanyway, so the validation succeeding is exactly what makes the caller believe the window moved. Eight of those shipped past a green suite during #2504. This is therefore proved by content on both SKUs: rows are seeded outside every default window, the anchored call returns them, and the same-length window at the default anchor does not. Neither tool namesDateTime.UtcNow, and Lite threads the resolved instant into bothGetQueryHeatmapCellsAsyncandGetQueryHeatmapCoverageAsyncrather than letting either compute its own.That also subsumes the window-skew fix from the review below: one resolved instant now decides the query window, the coverage probe's window, and the window reported back.
Merge conflicts, and how they were taken
devmoved under this branch (#2504, #2505, #2507, #2508). Three files conflicted:DarlingWebEndpoints.cscatalog + dispatch — dev'sas_ofon the neighbouring reads, plusPAsOf()/as_of: AsOf(c)onget_query_heatmap.as_of); the heatmap row is re-inserted in place withas_ofappended.The census sentence did not conflict, and was recounted anyway rather than trusted: scanning the
McpServerTool(Name = ...)attributes gives 109 / 84 / 25, which is what it says. Theuat-onboardingcounts were re-derived the same way and are unchanged at 90 read endpoints, 109 tools, 69 reached by the per-server page.How this was verified
v_query_stats, and the text inDarlingQueryHeatmapReaderis asserted equal to the text that ran, modulo whitespace. Five-minute and sixty-minute bins, the database filter, and theLIMITpath all produce what the tests expect.Directory.Packages.props— and produces cell-for-cell identical output to the Postgres run on the same rows.EnableWindowsTargetingritual;global.jsonis untouched in every commit.AsOfWindowAnchorTestssource-scan rules (anchor reaches the query, noDateTime.UtcNowin an anchored body, no anchorable service call left un-anchored) were replayed locally over both trees before pushing: 58 anchored tools scanned on Darling, 51 on Lite, zero offenders.node --checkonserver-tabs.js. The panel is built withtable(...), not an object literal.<summary>hygiene, the census against a fresh attribute scan, the shared-sentence parity in both trees, and that every param key the panel sends is one the catalog binds.What I could NOT verify
net10.0-windows, so they build here and run in CI. The live Darling test is gated onDARLING_TEST_PG.node --checkand by the page-level pins, but no browser has drawn it.One judgement worth flagging for review: the metric list is the viewer's five, including
logical_readsandlogical_writes. Dropping them would have made the two surfaces disagree about what the heatmap can show, which is the thing this PR exists to prevent.CI
All six checks green. One red run along the way was
Lite.Tests.LiteServerTagsStoreTeststiming out on the DuckDB write lock — the documented writer-starvation shape of that suite's one process-wideReaderWriterLockSlim(SharedDuckDbFixture's own doc comment describes it; #2208 is the same shape). Unrelated to this change and green on re-run, but worth naming rather than quietly re-running.🤖 Generated with Claude Code