Repository navigation
Route the viewer's built-in tabs to the retention rollups (#1661) - #1664
Conversation
…#1661) Groundwork for routing the viewer's built-in tabs off the CAGGs. No reader is switched over yet -- this puts the decision in one place first, so the two readers cannot drift into disagreeing about which tier still retains a window. RetentionTierRouter lands in Storage because both consumers need it and neither can see the other: the composer lives in the service, the built-in tabs live in the viewer, and the viewer does not reference the service. Both already reference Storage, which is also where TimescaleSupport owns the CAGG names and retention intervals the thresholds are derived from. ComposeSourceRouter now delegates its age decision here and aliases its public constants to the shared values, so its behaviour is unchanged (209 composer tests still green) and there is no second copy of the thresholds to update. It also carries the text horizon. The CAGGs group by identity columns and sum deltas -- they hold no query_text and no query_plan -- so a reader projecting either is limited to the raw window no matter what range was asked for. ClampToTextHorizon returns that clamp AND whether it applied, because the fix for those readers is to say so, not to silently return four days of a thirty-day request. Scope note found while mapping the readers: only query_stats, procedure_stats and query_store_stats drop at 4 days. Everything else keeps DarlingRetention's 30-day default, so within Daily Summary only the one v_query_stats CTE is affected -- its wait and deadlock CTEs are fine. The affected surface is narrower than the issue first suggested. Tests pin the tier boundaries, the future/present window, the text clamp and its signal, that the composer still delegates, and -- as an invariant against the retention constants -- that each route threshold stays strictly inside the horizon it protects. A threshold reaching its horizon would reintroduce #1661 one tier up. Darling.Tests 3076 passed; solution builds 0 warnings. Refs #1661 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
First reader switched over. GetDailySummaryRangeAsync now resolves a tier from the window's oldest point and swaps the query-count CTE onto query_stats_hourly or query_stats_daily. Before this the Performance Calendar reported zero distinct queries for every day older than ~4 days once retention began dropping chunks -- silently, since a zero is indistinguishable from a quiet day. Only that one CTE moves. Every other source in the daily summary (waits, deadlocks, blocked-process reports, DMV snapshots, CPU, collection log, memory pressure, alerts) reads a table on DarlingRetention's 30-day default and is unaffected by the 4-day drop, so routing them would be wrong. COUNT(DISTINCT query_hash) is exact over a rollup because query_hash is one of the CAGG's GROUP BY columns -- the rollup preserves every distinct hash per bucket, so counting them per day gives the identical answer raw would. That is pinned against the CAGG definition, since it is the assumption the whole substitution rests on. The raw constant is returned byte-for-byte on the raw tier, so the recent-window path is provably unchanged, and parameter positions are identical on every tier so the caller binds the same three values. The substitution throws if it matches nothing rather than returning the raw SQL. That earned its keep immediately: the first version of the extracted CTE had a raw-string margin that did not match the real query, and the guard failed the test instead of quietly leaving the calendar on raw -- which is exactly how the original bug hid. Darling.Tests 3080 passed; solution builds 0 warnings. Refs #1661 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…1661) Routing the viewer's calendar exposed a second copy of the same query. The service's DarlingHealthReader -- behind the get_daily_health MCP tool -- carried its own literal, documented as the viewer's "verbatim" with nothing enforcing it. Routing one and not the other would have left the calendar and the MCP tool answering the same day differently. Compared them first: they differed ONLY in stripped comments, so the SQL was genuinely identical and unifying is behaviour-preserving. But nothing would have caught it had they drifted, and this is the second hand-mirrored pair to bite this codebase. The canonical definition now lives in Storage as DailySummarySql, which both projects already reference and neither can reach the other through. Viewer and reader keep their public DailySummaryRangeSql as thin aliases so existing pins still resolve, and both get the tier routing for free. The proof this move was faithful is that both suites' SQL assertions still pass untouched -- ViewerDailyHealthTests and DarlingMcpHealthToolsTests pin the query shape independently, from opposite sides, and neither needed editing. Darling.Tests 3080 passed; solution builds 0 warnings. Refs #1661 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
FinOps' database-grain workload view sums delta_logical_reads, delta_physical_reads and delta_logical_writes. No continuous aggregate carried any of them -- the existing rollups were built to the composer's measure set, which never exposed I/O -- so that view could not route past the raw horizon at all. Added as a NEW aggregate rather than widening query_stats_hourly. TimescaleDB cannot ALTER columns into a continuous aggregate, so widening means DROP + recreate; with retention now active that rebuild re-materializes from 4 days of raw and permanently destroys the 21-day hourly and indefinite daily history the tiers exist to preserve. On the field box that is unrecoverable. A new aggregate costs nothing existing -- its history simply starts accumulating from deploy, which is honest and unavoidable either way. Grouped by database_name and deliberately NOT query_hash: that coarser grain is what keeps it small despite carrying more columns than the query-grain rollup -- one row per database per hour rather than one per query. It also applies the same delta_worker_time IS NOT NULL filter the reader uses, so the pre-aggregated totals match what the raw query would have produced. Wired into the ensure sweep (hourly before daily, since the daily is hierarchical) and given the standard hourly retention policy; the daily gets none, so it is kept indefinitely like its siblings. Tests pin the I/O columns, that those columns exist here and NOWHERE else (if they are ever added to the query-grain aggregate this becomes redundant, and that should be deliberate), the database grain, the hierarchical daily source, and the NULL-worker-time filter. Darling.Tests 3084 passed; solution builds 0 warnings. Refs #1661 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three FinOps queries and the get_daily_health MCP reader now resolve a tier and read the matching rollup instead of always hitting raw. The MCP reader was the sharper of the two. It answers the same question as the viewer's calendar, so routing only the calendar would have had them report different query counts for the same day with no way to tell which was right. FinOps splits by what each query actually needs. Database resource usage sums the I/O columns, so it is the one reader that needs the new per-database rollup. Both top-consumer queries want only CPU and executions, which the query-grain aggregate already carried -- they route without it. The rollups pre-filter NULL worker time, so the routed forms drop that predicate rather than re-applying it to a column that no longer exists. Each swap throws if its raw fragment no longer matches the SQL. A guard that throws is only protective if something calls it, so the tests exercise every tier of every routed reader -- otherwise a drifted fragment would surface as an exception in front of a user instead of a failed build. The raw tier is asserted to return each constant byte-for-byte, so the recent-window path is provably unchanged. Also added last_execution_time_max to the per-database rollup, which the FinOps storage view needs. Free to change: that aggregate is new in this branch and has not shipped anywhere. Darling.Tests 3087 passed; solution builds 0 warnings. Refs #1661 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The expensive-queries FinOps panel selects query_text and query_plan_xml. No rollup carries either -- the CAGGs group by identity and sum deltas -- so unlike the aggregate readers this one cannot be routed at any tier. It now clamps its window to the text horizon and the panel says so, rather than presenting a few days of data as though it were the month that was asked for. That matters more now than before: the aggregate FinOps panels DO reach further back via the rollups, so without the note the two would look inconsistent for no visible reason. The label reuses the shared router rather than hardcoding a horizon, so it can never disagree with the clamp it is describing, and it lands in the existing count indicator -- no XAML change. Correcting my own earlier scoping: ItemHistory is NOT a text reader. It projects query_plan_HASH, a fixed-size hash, and no query_text at all -- an earlier grep conflated the two. It is not routable either, since it selects per-item columns (spills, dop, grant sizes) no rollup carries, but as a drill into one query's recent behaviour the raw window is a reasonable scope, so it is deliberately left alone rather than clamped for a limit it does not have. That makes the expensive-queries panel the only true text reader, and #1661's reader work complete. Darling.Tests 3087 passed; solution builds 0 warnings. Closes #1661 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…in-PG stores) The gated-live CI caught the first cut routing by window age alone: the continuous aggregates are runtime TimescaleDB setup, so a plain PostgreSQL store (CI's dev PG, and any bring-your-own store without the extension) never has them, and an old-window Daily Summary / FinOps / get_daily_health read threw 42P01 at the user. On plain PG raw is never dropped either (retention policies need the extension), so raw is complete there - the right answer, not a downgrade. - TimescaleSupport.DetectRollupsAsync + RollupAvailability: one to_regclass probe over the four rollup views, privilege-free under the viewer role. - RetentionTierRouter.Resolve(now, start, hourlyAvailable, dailyAvailable): degrades daily-missing -> hourly (the composer's existing DailyView-null rule) and hourly-missing -> raw. - Viewer: cached probe (positive cached for life; negative re-probed at 5m so a mid-session service upgrade converges); all four routed readers gated. - Service: get_daily_health probes per call. - Tests: availability-degrade theory + probe-coverage pin; a new live test at -10 days (deterministically Hourly-age - the -3d sibling straddles the margin by time-of-day, which is how the first cut's runs happened to pass). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Took this over after the gated-live CI failure. Root cause: the tier routing chose a relation by window age alone, but the CAGGs are runtime TimescaleDB setup — CI's dev PG (and any BYO plain-PostgreSQL store) doesn't have them, so old-window reads threw 42P01. Fix: a to_regclass availability probe (TimescaleSupport.DetectRollupsAsync, cached in the viewer) gating RetentionTierRouter — plain PG routes to raw, which is complete there since retention policies also need the extension. Partial builds degrade one tier at a time, mirroring the composer's DailyView-null rule. New live test at -10 days makes the failure deterministic (the -3d test straddles the route margin by time-of-day). Note the composer has the same pre-existing exposure from #1625 — filed separately. |
…actually has (erikdarlingdata#1665) The composer's arm of the erikdarlingdata#1664 availability guard. ComposeSourceRouter picked hourly/daily rollup relations purely by window age and dimension coverage; on a plain-PostgreSQL store (no TimescaleDB, so no continuous aggregates exist) any composed panel with a window past ~3 days compiled SQL against a missing relation and failed at run time with 42P01. A partially-built TimescaleDB store routed onto its missing aggregates the same way. - TimescaleSupport.RollupProbeSql / RollupAvailability widened 4 -> 8 flags (procedure_stats + query_store_stats pairs), with Has(view) and an All shorthand; unknown views answer absent, so a catalog entry added without extending the probe degrades to raw instead of routing blind. Probe-coverage pin extended to all 8 + a Has() drift guard. - ComposeSourceRouter.Resolve takes RollupAvailability and delegates the degrade to the SHARED RetentionTierRouter ladder with per-table flags (daily missing -> hourly capped, hourly missing -> raw); the old DailyView-is-null fallback feeds the same ladder unchanged. The all-rollups path routes byte-for-byte as before. - ComposeRunContext carries Rollups; ComposeCompiled carries the Route it took. The one shared runner (web /api/compose/run + MCP run_custom_view_panel) probes lazily via ComposeStoreAvailability, cached per data source with the viewer's exact rule: permanent once fully built, 5-minute re-probe while partial, raw on a failed probe. - Partial window, and says so: on a retention-active store, a route landing on a tier whose retention cannot reach the window start (raw ~4d, hourly ~21d) returns a payload notice; scoped to the TIERED tables only (everything else lives on the 30-day collector purge, so raw is complete there and a notice would be a false alarm). Plain PG stays silent - raw is complete, not degraded. Rendered above the chart on every web surface (noticeStrip); rides verbatim through MCP. - Gated-live end-to-end: a 10-day window through RunComposedPanelAsync against plain PostgreSQL - the exact 42P01 repro - now runs clean on raw with no notice. Validated locally against a real plain PG 17 (bundled runtime): full suite 3235/0. - Test hygiene drive-by: ViewerDrillDownTests / ViewerTimeHelperTests / ViewerManageServersColumnTests all flip the process-wide ViewerTimeHelper.CurrentDisplayMode static and raced under xUnit class parallelism (caught as a 1-in-4 local flake); serialized via a shared [Collection("viewer-time-statics")]. Closes erikdarlingdata#1665 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rlingdata#1680) TimescaleDB runs a new retention policy's first check immediately at creation rather than on its next interval. A policy created live therefore drops before any external session can pause it. On the field store this fired all six new retention jobs within the same minute as service startup and permanently removed ~2 days of query_stats/procedure_stats history plus all of query_store_stats, before the runbook's pause could take effect. There is no window to win, so the fix cannot be "pause it faster". Policies are now created scheduled => false and armed as a separate step, gated on a coverage check: the tier below must already reach at least as far back as the source does -- raw covered by its hourly rollup, hourly by its daily. That makes arming provably non-destructive rather than a race. It also self-heals. A fresh store has nothing to lose and arms immediately. A store whose rollups have not caught up is held paused with a warning naming the rollup to backfill, and arms itself on the first start after that backfill, with no manual step. The probe fails CLOSED. An unknown coverage state leaves the policy paused rather than arming on an assumption -- the whole point is that the destructive direction must never be the default. Covers all seven policies, including the query_stats_db_hourly one added by erikdarlingdata#1664, which would otherwise have shipped with the same hazard. Darling.Tests 3131 passed; solution builds 0 warnings. Closes erikdarlingdata#1680 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…so (erikdarlingdata#1687) A grouped time-series panel whose buckets x groups product exceeds the 10,000-row hard cap got "ORDER BY bucket LIMIT 10000", which keeps the EARLIEST rows. A 24-hour wait_stats panel grouped by wait_type at minute grain therefore rendered its first ~87 minutes as though that were the whole window - and because the truncated domain never crossed midnight, the x-axis showed no dates, so nothing on screen looked wrong. Same silent-truncation class as the retention-routing gaps (erikdarlingdata#1661/erikdarlingdata#1664/erikdarlingdata#1665): the numbers are right, the WINDOW is wrong. Two halves, both needed: 1. The cap now applies ORDER BY bucket DESC inside a subquery, with the survivors re-sorted ascending for the renderer. A truncated chart shows the RECENT end, which is what a monitoring chart is for. The wrapper opens AFTER any CTE so the WITH stays top-level, and no parameter positions move - the time-series LIMIT is a literal (only the Ranked arm binds a LIMIT parameter) and the wrapper only brackets text whose parameters were already appended in order. 2. When a time-series panel comes back sitting exactly on the cap, it attaches a notice through the existing erikdarlingdata#1665 notice plumbing, rendered by the noticeStrip already present on every web surface and carried verbatim through MCP. Exactly-at-cap is the only available signal: the compiler cannot know the product ahead of time because group cardinality is a property of the data, not the spec. A panel that legitimately produces exactly 10,000 rows is therefore reported as capped - a false positive accepted deliberately, because the reverse mistake is the bug this fixes. Retention and row-cap notices are joined rather than one winning: both truncate a panel from the same end, and a long grouped panel on a retention-active store hits both. Showing one and dropping the other would under-report the panel in the most trouble. Ranked and Scalar are untouched - a Ranked LIMIT is the caller's own topN, so reaching it is the request being honored, not truncation. The issue's optional third item (a write-time hint) is deliberately not implemented: MaxBuckets (5,000) is already below HardRowCap (10,000), so buckets alone can never trip the cap, and any pre-flight warning would have to guess at group cardinality it cannot know. The runtime notice covers the real case with real data instead. Validated against a throwaway PostgreSQL 17.10 cluster, not just string matching: the live compose-run test executes the wrapped SQL and the full compose suite ran with zero skips (192 tests, including the live-gated ones). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rlingdata#1746) The line's counts were computed ({Ready}/{Total}, correctly reporting 8) but the parenthetical text was hand-written and still enumerated the six aggregates that existed before erikdarlingdata#1664 added the db-grain pair. An operator mid-upgrade at a client site spent real time reconciling 8-against-6 before concluding it was not a bug. Fixed by shape rather than by number: the names now come from the same array the counts come from, so the summary quotes its own source and a future pair cannot make it stale again. Replacing "3 hourly" with "4 hourly" would have been the same defect waiting to recur. Swept the sibling summary lines for the same class. The retention, hypertable and compression lines already interpolate computed counts and interval constants, and the RetentionPolicyCount = 7 comment (three raw plus four hourly) checks out against the policy array. This was the only hardcoded enumeration. Closes erikdarlingdata#1746 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review finding: the flag was rollups != None, a store-wide test, while the #1680 gate arms each raw table's purge only once that table's OWN rollup covers it. On #1664's failure-isolated partial build, a grain whose rollup is missing has its raw rows intact — and the store-wide flag would have told that grain's caller the rows were dropped and widening cannot help. Now hourlyAvailable, the per-grain flag already threaded through the resolver; the past-horizon raw branch's dead "rollup not present" arm goes with it (a grain with no rollup can no longer reach that branch), and the pin asserts the per-grain split.
…urce like get_query_trend already does, so a 7-day request no longer returns 4 days labelled quiet (#3541 A2) (#3590) * The duration-trend trio routes by retention tier and discloses its source like get_query_trend already does (#3541 A2) get_query_duration_trend and get_procedure_duration_trend read raw query_stats / procedure_stats only, whose rows a TimescaleDB store drops at four days, while hours_back accepts 168 — so a 7-day request returned 4 days under a label saying 7, and when nothing survived the empty branch called the window "genuinely quiet — widen hours_back", which is false (dropped, not absent) and harmful (widening cannot help). Both now route through the ladder get_query_trend already uses (age → availability → coverage, DarlingTrendReader.ResolveTier) onto the hourly rollups, dividing by the bucket width rather than a LAG, and every sibling — the Query Store one included — publishes source / effective_start / effective_hours_back / truncated / bucket / aggregate_note on the data path and the empty one. get_query_trend gains the availability + coverage rungs (it answered 42P01 on plain PostgreSQL past four days). Lite's twins publish the same block with Lite's truth (raw, per-collection). value gains its named twin elapsed_ms_per_second on both SKUs. Partial: completes checklist item A2 of #3541 only. * The retention clock lives in the reader, not the anchored tool bodies AsOfWindowAnchorTests pins, as an absolute, that a tool advertising as_of never names DateTime.UtcNow — its only "now" is the anchor it resolved. The trio's route resolution measured raw age against the wall clock IN the tool body, which is the right clock for retention (the anchor decides the window, not how old its rows are) named in the wrong place. ResolveQueryDurationTrendRoute / ResolveProcedureDurationTrendRoute now default nowUtc to the real clock the way GetQueryHistoryAsync already does, and the route carries ResolvedAtUtc so the empty branch's horizon comparison reads it from the route. * RawRetentionApplies is scoped to the grain, because the arming gate is Review finding: the flag was rollups != None, a store-wide test, while the #1680 gate arms each raw table's purge only once that table's OWN rollup covers it. On #1664's failure-isolated partial build, a grain whose rollup is missing has its raw rows intact — and the store-wide flag would have told that grain's caller the rows were dropped and widening cannot help. Now hourlyAvailable, the per-grain flag already threaded through the resolver; the past-horizon raw branch's dead "rollup not present" arm goes with it (a grain with no rollup can no longer reach that branch), and the pin asserts the per-grain split. * RawRetentionApplies: say that existence-not-armed is the deliberate upper bound, and why (review question)
Closes #1661. Grew from a foundation PR into the full fix; the commits tell the story in order.
The bug
The three-tier retention from #1623 drops raw
query_statsat 4 days and runs automatically on every service start (DarlingWorker.cs:651). The CAGG read-routing from #1625/#1626/#1627 only ever covered the Custom Views composer. The viewer's built-in tabs readv_query_stats— a plain passthrough over the raw hypertable.So the Performance Calendar reported zero distinct queries for every day older than ~4 days, and the FinOps panels aggregated a 4-day sample while the picker said 30. Nothing errored; the numbers were quietly wrong. For a cost view whose entire purpose is trend over time, that is the worst failure mode available.
Each prior PR was correct in isolation — the write side and read side were designed as a pair, and the read side was scoped to the surface being built at the time.
Never released. Caught in
devbefore 3.3.0.One tier decision, one definition
RetentionTierRouterlands in Storage because both consumers need it and neither can see the other (composer in the service, tabs in the viewer; both reference Storage).ComposeSourceRouterdelegates to it and aliases its constants — behaviour unchanged, 209 composer tests still green, and no second copy of the thresholds.An invariant test parses
TimescaleSupport's retention intervals and asserts each route threshold stays strictly inside the horizon it protects. Change a retention interval without moving the margin and the build fails — otherwise you reintroduce this exact bug one tier up.Readers, by what they can actually be served
Routed — Daily Summary's query count, the
get_daily_healthMCP reader, and three FinOps aggregates. These sum additively over dimensions the rollups materialize.New rollup — FinOps' database-grain view sums
delta_logical_reads/delta_physical_reads/delta_logical_writes, which no aggregate carried (the others were built to the composer's measure set, which never exposed I/O). It gets a newquery_stats_db_hourly/_dailypair rather than a widened existing one:Grouped by
database_nameand deliberately notquery_hash, so it stays small despite carrying more columns.Clamped and labelled — the expensive-queries panel projects
query_text/query_plan_xml, which no rollup carries at any tier. It clamps to the text horizon and says so, rather than presenting a few days as a month. Without the note it would look inconsistent against the aggregate panels, which now do reach further back.Two things routing exposed
A second copy of the daily-summary query lived in the MCP health reader, documented as the viewer's "verbatim" with nothing enforcing it. They differed only in stripped comments, so unifying was behaviour-preserving — but routing one and not the other would have had the calendar and the MCP tool disagree about the same day. Now one definition in Storage. The proof the move was faithful:
ViewerDailyHealthTestsandDarlingMcpHealthToolsTestspin the shape independently from opposite sides, and neither needed editing.ItemHistoryis not a text reader. An earlier pass called it one; it projectsquery_plan_hash— a fixed-size hash — and noquery_textat all. It is not routable either (it selects spills/dop/grant columns no rollup carries), but as a drill into one query's recent behaviour the raw window is a reasonable scope, so it is deliberately left alone rather than clamped for a limit it does not have.Test plan
Every substitution throws if its raw fragment no longer matches the SQL. A guard that throws only protects you if something calls it, so the tests exercise every tier of every routed reader — otherwise a drifted fragment surfaces as an exception in front of a user instead of a failed build. That guard earned its keep during development: the first extracted CTE had a mismatched raw-string margin and failed the test rather than silently leaving the calendar on raw, which is exactly how the original bug hid.
Raw is asserted to return each constant byte-for-byte, so the recent-window path is provably unchanged.
🤖 Generated with Claude Code