Repository navigation
Serve get_query_trend from the tier that can answer, and say which one did (#2353) - #2356
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| public static async Task<QueryHistoryResult> GetQueryHistoryAsync( | ||
| NpgsqlDataSource postgres, int serverId, string databaseName, string queryHash, DateTime startUtc, DateTime endUtc, CancellationToken cancellationToken = default) | ||
| { | ||
| var useRaw = ShouldUseRawTier(startUtc, endUtc); | ||
|
|
There was a problem hiding this comment.
GetQueryHistoryAsync routes to query_stats_hourly purely by window age — it never checks whether that continuous aggregate actually exists on this store. query_stats_hourly is created WITH (timescaledb.continuous) (TimescaleSupport.CreateQueryStatsHourlySql), i.e. it only exists on a TimescaleDB-enabled store that has had the rollup created. The codebase already has an availability-gated path for exactly this (TimescaleSupport.DetectRollupsAsync → RollupAvailability, used by DarlingHealthReader.cs:208, and RetentionTierRouter.Resolve(..., hourlyAvailable, dailyAvailable)), but this new reader doesn't call into it.
Concretely: on a plain-PostgreSQL store (no TimescaleDB extension — per the comments elsewhere in this codebase, "a plain-PostgreSQL store never has [the rollups] and never drops raw either, making raw both available AND complete there"), requesting hours_back beyond ~4 days will now issue SELECT ... FROM query_stats_hourly ... against a relation that doesn't exist, throwing instead of returning the full (never-purged) raw history that the pre-#2353 code correctly served. That's a regression from "works, if imprecisely labeled" to "errors" for exactly the store type where the old code was already correct.
Same failure mode on a TimescaleDB store where the query_stats rollup hasn't been created/backfilled yet (partial setup / pre-upgrade). Worth threading RollupAvailability (or at least an hourlyAvailable: bool) through so this falls back to raw the way ComposeSourceRouter/RetentionTierRouter do, rather than assuming the rollup is always there.
| /// rows depending on when the purge last ran; preferring the aggregate there trades resolution for an answer | ||
| /// that does not change with the purge schedule. Exposed so a test pins the boundary rather than restating it. | ||
| /// </summary> | ||
| public static readonly TimeSpan RawTierMargin = TimeSpan.FromHours(1); |
There was a problem hiding this comment.
This introduces a second, independent margin constant for the same kind of decision RetentionTierRouter/ComposeSourceRouter already make. Those use RouteMargin = TimeSpan.FromDays(1) specifically because (per RetentionTierRouter.cs's own comment) "a drop lagging the boundary (1-day chunk granularity + the 3-day CAGG refresh) can never leave the chosen tier missing the oldest chunk." RawTierMargin here is only 1 hour — well inside that documented lag window for a retention policy whose default schedule interval is on the order of a day. That risks reintroducing a smaller-scale version of the exact bug this PR fixes: a window landing within roughly a day of the 4-day boundary could still route to raw and get silently truncated if the purge already ran but hasn't reached the margin yet.
Was reusing RetentionTierRouter.RawMaxAge (or its 1-day margin) considered instead of a new, smaller, hand-picked constant? If there's a reason query-trend reads can tolerate a tighter margin than every other reader in this codebase, it'd be worth a comment saying so — right now it just looks like a second source of truth that can drift from the first.
|
Reviewed the tiering fix in
No Lite-side changes are needed for this PR — Lite has no rollup/tiering system, so there's no direct counterpart to update. Didn't flag anything on the CHANGELOG or test file; both read fine. |
| public static async Task<QueryHistoryResult> GetQueryHistoryAsync( | ||
| NpgsqlDataSource postgres, int serverId, string databaseName, string queryHash, DateTime startUtc, DateTime endUtc, DateTime? nowUtc = null, CancellationToken cancellationToken = default) | ||
| { | ||
| var useRaw = ShouldUseRawTier(startUtc, nowUtc ?? DateTime.UtcNow); |
There was a problem hiding this comment.
This routes purely on age and never checks whether query_stats_hourly actually exists or has materialized the window — the exact gap RetentionTierRouter/ComposeSourceRouter were built to close (see ComposeSourceRouter.cs:152-167 and RetentionTierRouter.cs:109-134, and the sibling reader DarlingHealthReader.GetDailySummaryRangeAsync which calls TimescaleSupport.DetectRollupsAsync + DetectRollupCoverageAsync before routing, explicitly "so this tool and the viewer's calendar answer the same question").
Consequences:
- Plain-PostgreSQL stores (a fully supported configuration per
TimescaleSupport.cs:160) have noquery_stats_hourlyrelation at all. Anyget_query_trendcall whose window's oldest point is older than ~4 days will now issueSELECT ... FROM query_stats_hourly ...against a table that doesn't exist, turning a previously-working (if truncated) answer into a hard Postgres error. - A freshly-created or not-yet-backfilled CAGG (
WITH NO DATA, perRetentionTierRouter.cs:137-179) will silently return fewer/no rows for the older part of the window with no fallback to raw, even though raw may still hold those rows.
Recommend reusing RetentionTierRouter.Resolve(nowUtc, windowStartUtc, hourlyAvailable, dailyAvailable, coverage) (gated on TimescaleSupport.DetectRollupsAsync/DetectRollupCoverageAsync) instead of the new hand-rolled ShouldUseRawTier, the same way DarlingHealthReader does.
| /// rows depending on when the purge last ran; preferring the aggregate there trades resolution for an answer | ||
| /// that does not change with the purge schedule. Exposed so a test pins the boundary rather than restating it. | ||
| /// </summary> | ||
| public static readonly TimeSpan RawTierMargin = TimeSpan.FromHours(1); |
There was a problem hiding this comment.
This margin is 1 hour, but the established margin for the identical "how far inside the raw retention horizon can we safely route to raw" decision is 1 day (RetentionTierRouter.RouteMargin, RetentionTierRouter.cs:53-62, aliased as ComposeSourceRouter.RawRouteMaxAge). That constant's doc comment explains the margin is sized for "1-day chunk granularity + the 3-day CAGG refresh" (ComposeSourceRouter.cs:134-138) — i.e. it's not an arbitrary safety pad, it's derived from TimescaleSupport.ChunkIntervalDays = 1.
With a 1-hour margin, get_query_trend will route to raw for windows aged between ~3d and ~3d23h that the composer/viewer already route to hourly — and, since the whole point of the wider margin is that a chunk can lag the boundary by up to a day, this can route to raw for a chunk that has already been dropped, reproducing the exact silent/wrong-answer failure class this PR is fixing (just narrowed to a ~23-hour window instead of the full window).
Suggest deleting this local margin/threshold and delegating to RetentionTierRouter.RawMaxAge/RetentionTierRouter.Resolve so this reader can't drift from the composer and viewer's tiering decision.
| items, | ||
| useRaw ? "raw" : "hourly", | ||
| effectiveStart, | ||
| Truncated: items.Count > 0 && effectiveStart > startUtc.AddMinutes(90)); |
There was a problem hiding this comment.
Minor: the 90-minute threshold here is unexplained — no comment on why that value (vs., say, the hourly bucket width or RawTierMargin). This codebase's convention (called out in CONTRIBUTING.md's T-SQL section, and followed consistently elsewhere in this file) is to document the reasoning behind constants like this so a later reader doesn't have to reverse-engineer it.
Review summaryThis is a well-tested fix for the core reported bug (raw-only reads silently truncating/misreporting long windows), and the tests ( However, the new tiering logic in
Recommend routing |
…e did The read went to raw query_stats only, and the raw tier of a ROLLED table is dropped at 4 days independently of the collector's much longer advertised retention (#2353). So a request for 168 hours returned whatever had not aged out, under a label still saying 168. The empty path was worse than the short array. It asserted "No history found ... within the last 168 hours" over a span the read never covered -- for a query whose history had simply aged out that is a false statement, not an incomplete one, and an agent acts on it by concluding the query never ran. It now says which tier it searched and tells the caller to confirm the hash before drawing that conclusion. Tiering is by the age of the window's OLDEST point measured from WALL CLOCK, matching ComposeSourceRouter. Writing the test caught this being wrong first time: comparing the start against the window's END makes a two-hour window from ten days ago look "recent" -- it is recent relative to its own end -- and routes it to a tier that dropped those rows six days earlier. Retention answers to now, so now is the parameter. A window past the horizon is served ENTIRELY from the hourly rollup rather than stitched: a series whose bucket width changes partway is a worse answer than a coarser consistent one. The rollup keeps executions, CPU and elapsed and nothing else, so the other eight columns are selected as typed NULLs and surface as null -- never zero, because on an aggregate row a zero reads as "none observed", a measurement nobody made. The response now carries source, effective_hours_back, bucket, truncated and a note naming exactly which fields are unavailable and why. Both projections are pinned to identical ordinals, since one mapper serves both and a column added to one alone would mis-map silently rather than fail. Reported by @carlei1978, who traced it to the SQL rather than stopping at "the numbers look short". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…dow's end DocCommentHygieneTests caught the splice: extracting ShouldUseRawTier put its doc block below GetQueryHistoryAsync's, so the method's summary ended up stacked on RawTierMargin's and the method itself was left undocumented. Moved rather than deleted -- the test's own message warns that seven of the eight found in #1745 were displaced blocks whose real member had been left bare, and deleting them would have lost documentation rather than deduplicating it. This was the eighth. Fixing that surfaced a real bug underneath it. GetQueryHistoryAsync was calling ShouldUseRawTier(startUtc, endUtc) while the parameter means nowUtc -- exactly the mistake the test ANarrowWindowInThePast_StillTakesTheAggregate exists to catch, sitting in the one call site the test does not cover. It happens to work today because the only caller passes now as endUtc, which is the worst kind of correct: right by coincidence, and silently wrong for the first caller that asks for a historical window. Tiering now measures against the real clock, with an optional nowUtc for tests. Deliberately not defaulted to endUtc, which would restore the bug behind a nicer signature. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adding nowUtc after the token tripped CA1068 -- one new warning against a base branch that had zero, which is the check that catches this class of edit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
a55b9a8 to
48b8cb8
Compare
| var useRaw = ShouldUseRawTier(startUtc, nowUtc ?? DateTime.UtcNow); | ||
|
|
||
| var items = useRaw | ||
| ? await ReadQueryHistoryAsync(postgres, QueryHistorySql, serverId, databaseName, queryHash, startUtc, endUtc, cancellationToken) | ||
| : await ReadQueryHistoryAsync(postgres, QueryHistoryHourlySql, serverId, databaseName, queryHash, startUtc, endUtc, cancellationToken); |
There was a problem hiding this comment.
Regression: unconditionally queries query_stats_hourly with no availability gate — will throw 42P01 on a plain-PostgreSQL store.
ShouldUseRawTier/GetQueryHistoryAsync route purely on window age and, when the raw horizon is exceeded, query query_stats_hourly (QueryHistoryHourlySql) unconditionally. But this codebase treats TimescaleDB as optional — a plain-PostgreSQL store never has continuous aggregates, and this exact failure mode (routing an old window to a rollup relation that doesn't exist → 42P01 undefined_table) was already hit and fixed once, in #1661, via RetentionTierRouter/ComposeStoreAvailability:
// RetentionTierRouter.cs
public static RetentionTier Resolve(DateTime nowUtc, DateTime windowStartUtc, bool hourlyAvailable, bool dailyAvailable)RetentionTierRouterTests.cs even pins the plain-PostgreSQL case explicitly: [InlineData(120, false, false, RetentionTier.Raw)] — "no rollups, raw holds full history: every window is raw."
Since get_query_trend's hours_back maxes out at 168 (McpHelpers.MaxHoursBack = 7 days), any request past roughly the 4-day raw horizon on a store without the hourly CAGG will now throw where it previously always succeeded (querying only query_stats, which always exists). The exception is caught by the outer try/catch in DarlingMcpTrendTools.GetQueryTrend and surfaces as an opaque FormatError, rather than the graceful "stay on raw, it's complete" fallback the rest of the codebase uses for this exact deployment shape.
This reader should route through RetentionTierRouter.Resolve(nowUtc, startUtc, hourlyAvailable, dailyAvailable) (with availability probed the way ComposeStoreAvailability/DetectRollupsAsync do elsewhere) instead of the standalone ShouldUseRawTier age-only check.
| /// rows depending on when the purge last ran; preferring the aggregate there trades resolution for an answer | ||
| /// that does not change with the purge schedule. Exposed so a test pins the boundary rather than restating it. | ||
| /// </summary> | ||
| public static readonly TimeSpan RawTierMargin = TimeSpan.FromHours(1); |
There was a problem hiding this comment.
Margin inconsistent with the established routing constant, and duplicated instead of reused.
RawTierMargin here is TimeSpan.FromHours(1), but RetentionTierRouter.RouteMargin (the single-source-of-truth margin ComposeSourceRouter and the viewer both use) is TimeSpan.FromDays(1), applied specifically "so a window is never routed at a tier whose oldest chunk is about to be dropped underneath it." A 1-hour margin is much thinner than the margin the rest of the codebase deliberately settled on for this same retention-purge race — the periodic drop_chunks job doesn't run every hour, so a window landing in this narrower margin can still get routed to raw and then find its oldest rows already gone by the time the query runs, i.e. the same silent-empty failure this PR is fixing, just moved closer to the boundary.
Reusing RetentionTierRouter.RawMaxAge (RawRetentionSpan - RouteMargin) instead of introducing a second, independently-tuned constant would remove this drift and the duplicated tiering logic in ShouldUseRawTier below.
|
Reviewed the diff. The core fix (tiering
No Lite/Darling parity concern — Lite has no |
Closes #2353. Reported by @carlei1978 with the SQL already traced —
DarlingTrendReader.QueryHistorySql,TimescaleSupport.RawRetentionInterval, and theComposeSourceRoutercomparison that showed the rollups were right there.The bug, and the part that is worse than reported
The read went to raw
query_statsonly. The raw tier of a rolled table is dropped at 4 days, independently of the collector's much longer advertised retention — so a request for 168 hours returned whatever had not aged out, still labelled 168.The empty path did more than omit:
For a query whose history aged out of raw but sits in the hourly rollup, that is a false statement, not an incomplete one. An agent reading it concludes the query did not run. That is the worst failure available to a monitoring tool, and it is the part I fixed first.
Tiering, and a bug the test caught
Routing is by the age of the window's oldest point, measured from wall clock — the same rule
ComposeSourceRouterstates.I got this wrong on the first pass: I compared the window start against the window end. Writing
ANarrowWindowInThePast_StillTakesTheAggregatecaught it. A two-hour window from ten days ago is recent relative to its own end, so that rule called it raw-servable and would have routed it to a tier that dropped those rows six days earlier — the same silent-empty failure in a new place. Retention answers tonow, sonowis the parameter, and the test now pins that reasoning explicitly.A window past the horizon is served entirely from the rollup rather than stitched: a series whose bucket width changes partway is a worse answer than a coarser consistent one.
Null, never zero
The rollup carries executions, CPU and elapsed — and nothing else. The remaining eight columns are selected as typed NULLs and surface as
null. Not zero: on an aggregate row a zero reads as "none observed", which is a measurement nobody made.Worth noting what survives is exactly what a trend is usually asked for. The columns lost are reads/writes/rows/spills/DOP/plan-hash; the ones kept are the ones that answer "is this query getting slower or more expensive".
The response now describes the data, not the request
source,effective_start,effective_hours_back,truncated,bucket, and anaggregate_notenaming precisely which fields are unavailable and why.hours_backis still echoed, but it is no longer the only thing that looks like a description of what came back.Tests
DarlingQueryTrendTieringTests: the boundary (inclusive, and one second past it flips), that the margin sits inside the horizon rather than outside, the narrow-window-in-the-past case above, exact ordinal parity between the two projections (one mapper serves both, so a column added to one alone mis-maps silently rather than failing), and structural pins that the aggregate query reads the rollup and the raw query still does not.Verification
Both projects build clean, 0 warnings. Suites target
net10.0-windowsand cannot run on macOS, so the tier rule and the column-parity parser were exercised in a throwawaynet10.0harness against the shipped SQL constants — 19 checks, all passing, including the 12-column ordinal match.