Repository navigation
Serve the raw collection log (#2484) - #2494
Conversation
First of the #2484 list. The viewer's Collection Log and Duration Trends tabs read the per-run collection log; nothing else could. /api/read is derived from the MCP catalog, so a missing tool is missing from the web dashboard AND from any agent -- and this is the log people reach for when the seven-day rollup says HEALTHY and collection still looks wrong. Until now the only way to it was the WPF viewer on a Windows desktop. No new collection. The store already holds these rows; this is an endpoint over v_collection_log, the same view the viewer reads, so the three surfaces cannot drift. Two things done deliberately rather than by copying the neighbours: The window is bounded on BOTH sides, not by a single now-relative lower bound. Someone asking about a past incident wants the runs from THEN, and an hours-back-from-now span cannot say that. Zero rows answers with WHICH nothing it found. A server that collected and was quiet gets "empty" and is told to widen the window; a server that has never collected at all gets "unavailable" and is told collection is not running. Both are literally "no runs in the last 24 hours", and the responses are opposites -- telling the second caller to widen sends them off filling a window that never fills. That is the #2485 defect and this read does not add another instance of it. Costs one LIMIT 1 probe, only on the empty path. The payload splits duration into time on the monitored server and time on the store. The total cannot separate a collector that is slow because the target is slow from one slow because the store is, which is the first question anyone asks of a slow collector. Live store test covers all four: never-collected, quiet-window, the data path with the split, and the cap announcing itself. Service and tests build clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| { | ||
| read: "get_collection_log", | ||
| params: { server, hours_back: 24, limit: 200 }, | ||
| title: "Collection Log", | ||
| subtitle: "individual runs, newest first, trailing 24 hours", | ||
| viz: "table", | ||
| rowsKey: "runs", | ||
| columns: COLLECTION_LOG_COLUMNS, | ||
| emptyText: "No collector runs in the last 24 hours.", |
There was a problem hiding this comment.
Bug: this panel descriptor is a raw object literal, not run through table()/renderPanel() like every other entry in SERVER_TABS (table(...), stat(...), and fanout(...) all resolve to actual DOM nodes via renderPanel). build()'s return array is passed straight to mount(gridNode, ...) (pages/server.js:78), and appendChildren (util.js:59) only special-cases Array and instanceof Node — anything else falls through to node.appendChild(document.createTextNode(String(children))). A plain object stringifies to "[object Object]", so this panel will render as literal text "[object Object]" in the Collection Health tab instead of a table, and get_collection_log is never even fetched (no renderPanel call means no wired-up readTool).
Fix: wrap it with the existing table() helper, e.g.:
| { | |
| read: "get_collection_log", | |
| params: { server, hours_back: 24, limit: 200 }, | |
| title: "Collection Log", | |
| subtitle: "individual runs, newest first, trailing 24 hours", | |
| viz: "table", | |
| rowsKey: "runs", | |
| columns: COLLECTION_LOG_COLUMNS, | |
| emptyText: "No collector runs in the last 24 hours.", | |
| table( | |
| "Collection Log", | |
| "get_collection_log", | |
| { server, hours_back: 24, limit: 200 }, | |
| "runs", | |
| COLLECTION_LOG_COLUMNS, | |
| "individual runs, newest first, trailing 24 hours", | |
| "No collector runs in the last 24 hours." | |
| ), |
| Says outright that the cap bit, rather than leaving a caller to infer it by | ||
| counting rows against a limit they may not have set themselves. | ||
| */ | ||
| truncated = rows.Count >= Math.Clamp(limit, 1, 5000), |
There was a problem hiding this comment.
Minor: truncated is derived from rows.Count >= Math.Clamp(limit, 1, 5000) rather than an actual "is there more data" check. When the window happens to contain exactly limit matching runs (no more, no less), this reports truncated: true even though nothing was cut off — a false positive. Given the PR's own stated goal ("the cap announces itself instead of being inferred"), the honest way to know is the classic over-fetch: LIMIT maxRows + 1 in CollectionLogSql, then truncated = rows.Count > maxRows and trim the extra row before returning. Low impact (worst case a caller redundantly raises limit once), but worth a look since it's the one signal this feature is built around.
|
Reviewed the diff. Left two inline comments:
Lite/Darling parity note (non-blocking): Lite's MCP surface ( Everything else looks solid: the empty-vs-unavailable distinction is well-tested ( |
All four were right, and three of them caught something I would not have noticed by reading my own diff. The tool-surface pin enumerates the data tools by name, so a sixteenth had to be declared rather than appear. Its doc comment said "fourteen" while the list held fifteen and the method name said Fifteen, so all three now say sixteen and agree. The page's canonical time key is `hours`; I wrote `hours_back`. Both bind to the same parameter, which is exactly why the rule exists -- with two spellings in play a reviewer cannot tell an alias from a typo. Fixing it surfaced a worse bug of mine underneath: I had hardcoded a 24-hour window, so the panel ignored the page's time picker and would have answered a question about last Tuesday with today. It now takes ctx.hours like its neighbours, and the subtitle and empty text say "the selected window" instead of naming a fixed 24 hours they no longer control. The cleanup ratchet refuses a teardown on the test body's own connection: a throw out of the finally replaces the exception already in flight, so a failing test reports its cleanup error instead of its own -- and it is the body's failure that closed the connection the cleanup then needs. Converted to LiveStoreCleanup.RunAsync, which opens its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| back the individual runs. This is the tab people reach for when the rollup says HEALTHY and | ||
| collection still looks wrong, and until now the WPF viewer was the only way to it. */ | ||
| { | ||
| read: "get_collection_log", | ||
| params: { server, hours: ctx.hours, limit: 200 }, | ||
| title: "Collection Log", | ||
| subtitle: "individual runs, newest first, over the selected window", | ||
| viz: "table", | ||
| rowsKey: "runs", | ||
| columns: COLLECTION_LOG_COLUMNS, | ||
| emptyText: "No collector runs in the selected window.", | ||
| }, |
There was a problem hiding this comment.
Bug: this panel will never render — it's a raw descriptor, not a DOM node.
Every other entry in this build() array comes from table(...), line(...), stat(...), or ...fanout(...), all of which ultimately call renderPanel(desc) and return an actual panel DOM node (panels.js:44). This new entry is a plain object literal that skips that call entirely.
mount() → appendChildren() (util.js:59-70) only special-cases values where children instanceof Node; anything else falls through to node.appendChild(document.createTextNode(String(children))). For a plain object that stringifies to "[object Object]", so the Collection Log panel will render as literal text [object Object] in the grid instead of a table — no exception is thrown, so the DOM-shim smoke test mentioned in the PR description ("every tab built without throwing") would not have caught this.
Fix by using the existing table() helper, consistent with every other table panel in this file:
table(
"Collection Log",
"get_collection_log",
{ server, hours: ctx.hours, limit: 200 },
"runs",
COLLECTION_LOG_COLUMNS,
"individual runs, newest first, over the selected window",
"No collector runs in the selected window."
),
| public void AdvertisedSchema_IsGeminiClean_ForAllSixteenDataTools() | ||
| { | ||
| var tools = BuildDataToolSchemas(); | ||
| Assert.Equal(15, tools.Count); |
There was a problem hiding this comment.
Stale count: this test method was renamed from ...ForAllFifteenDataTools to ...ForAllSixteenDataTools for this PR, but the assertion still hardcodes 15. BuildDataToolSchemas() registers every [McpServerTool] on DarlingMcpDataTools, which now includes get_collection_log, so tools.Count will be 16 and this assert will fail as soon as the suite runs.
| Assert.Equal(15, tools.Count); | |
| Assert.Equal(16, tools.Count); |
|
|
||
| try | ||
| { | ||
| var end = DateTime.UtcNow; |
There was a problem hiding this comment.
This is the only hours_back-taking tool in this file (and, per a quick sweep of the Mcp/ folder, essentially the only one anywhere in Darling) that skips McpHelpers.ValidateHoursBack(hours_back). That shared helper rejects hours_back <= 0 and caps it at MaxHoursBack (168h / 7 days) with a clear message; here Math.Abs(hours_back) is applied inline instead, which:
- silently reinterprets a negative
hours_backas positive instead of returning the same actionable error every other tool gives for bad input, and - has no upper bound, so a caller can request an arbitrarily wide window (e.g.
hours_back = 100000000) instead of getting the standard "exceeds maximum of 168 hours" rejection.
If the unbounded/bidirectional-abs behavior here is intentional (per the PR description's point about needing a window "bounded on both sides" for past-incident lookups), it'd be worth a comment explaining why this tool opts out of the shared validation — otherwise this looks like an oversight relative to the rest of the tool surface.
| _ => $"SQL Server v{sqlMajorVersion}", | ||
| }; | ||
|
|
||
| [McpServerTool(Name = "get_collection_log"), Description("Gets the RAW per-run collection log for a server, newest first: one row per collector run with its total duration, the part spent querying the monitored server, the part spent writing to the store, rows collected, status and any error. get_collection_health rolls seven days of these into a per-collector verdict; this is the underlying runs, which is what you need when the rollup says healthy and collection still looks wrong, or when you want to see what a collector was doing during a specific incident window.")] |
There was a problem hiding this comment.
Lite/Darling parity: get_collection_log is added only to Darling's MCP surface. The test file's own pin comment (DarlingMcpDataToolsTests.cs) describes this tool list as "the same names Lite and the Dashboard expose, so MCP clients see one consistent product," but there's no corresponding get_collection_log tool under Lite/Mcp/ (Lite currently only exposes this data via the WPF CollectionLogWindow, not MCP/JSON). If a Lite-side follow-up is planned, ignore this — but flagging per the parity requirement in case it was meant to land together.
Review summaryReviewed the new
Everything else (SQL parameterization, style, the empty/unavailable status split, the live-store test coverage) looked correct and consistent with the rest of the codebase. |
The surface size was written by hand in four places: a literal in the assertion, the method name, the list, and two lines of prose. Renaming the method left the literal at 15 while the list held 16, which is the drift the shape invites rather than an oversight -- so the assertion now reads the length off the pinned list and the prose stops naming a number it does not control. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| public static async Task<string> GetCollectionLog( | ||
| NpgsqlDataSource postgres, | ||
| [Description("Server name or display name.")] string? server_name = null, | ||
| [Description("Hours of history. Default 24.")] int hours_back = 24, | ||
| [Description("Maximum rows to return, newest first. Default 200.")] int limit = 200) | ||
| { | ||
| var (resolved, error) = await DarlingServerResolver.ResolveOrErrorAsync(postgres, server_name); | ||
| if (error != null) return error; | ||
|
|
||
| try | ||
| { | ||
| var end = DateTime.UtcNow; | ||
| var start = end.AddHours(-Math.Abs(hours_back)); | ||
| var rows = await DarlingDataReader.GetCollectionLogAsync( | ||
| postgres, resolved.ServerId, start, end, Math.Clamp(limit, 1, 5000)); |
There was a problem hiding this comment.
get_collection_log skips the shared McpHelpers.ValidateHoursBack(hours_back) / McpHelpers.ValidateTop(limit) calls that every other windowed tool in this file makes (e.g. GetAlertHistory, GetQueryStoreTop, GetActiveQueries). Concretely:
hours_back <= 0(including negatives) is silently absorbed byMath.Absinstead of returning the standard "must be a positive integer" error every sibling tool returns.hours_back = 0produces a zero-width window that will almost always read as "empty" (or worse, "unavailable" if it happens to race with a run), rather than telling the caller the input was invalid.- There's no upper bound on
hours_back— every other tool caps atMcpHelpers.MaxHoursBack(168h/7 days); this one will happily scan a window of any size. limitis silently clamped to[1, 5000]instead of validated viaMcpHelpers.ValidateTop(which caps at 1000 and returns an explicit error for out-of-range input). The 5x-larger silent ceiling is also inconsistent with every otherlimit-bearing tool in this file.
Given the PR's own stated philosophy ("the cap announces itself instead of being inferred"), it'd be more consistent to validate hours_back/limit the same way the neighboring tools do and surface an explicit error for bad input, rather than silently coercing it.
| Says outright that the cap bit, rather than leaving a caller to infer it by | ||
| counting rows against a limit they may not have set themselves. | ||
| */ | ||
| truncated = rows.Count >= Math.Clamp(limit, 1, 5000), |
There was a problem hiding this comment.
truncated = rows.Count >= limit is a false positive whenever the window contains exactly limit matching rows and no more — the SQL enforces LIMIT $4, so rows.Count can never exceed the cap, which means this reduces to rows.Count == limit. With the default limit = 200, a server with several collectors on short schedules can easily land on exactly 200 runs in 24 hours with nothing beyond that, and this would tell the caller the log was truncated when it wasn't.
The usual way to detect real truncation is to fetch limit + 1 rows and check whether the extra row came back, then trim to limit before returning. Worth fixing given this PR's emphasis elsewhere on not making callers infer things that should be stated precisely (the whole point of this field).
|
Reviewed. Solid, focused PR — the empty-vs-unavailable distinction is exactly the right call, the bounded-window design and the SQL/store duration split are well justified, and the parity pins (tool count, catalog wiring) are updated. Two things worth fixing and one to flag: 1. 2. 3. Lite/Darling parity gap (not necessarily a blocker for this PR, but worth tracking): Lite already stores everything this reads ( 4. No SQL-injection or T-SQL style issues — this PR doesn't touch install/ T-SQL, and the new Postgres queries are parameterized and consistent with the rest of |
The cross-app pin offers two ways to satisfy it: port the tool to Lite, or add it to the ratchet of Darling-only tools. The ratchet is documented as something that only ever shrinks, and #2485 is already open about SKU divergence being a defect rather than a fact of life, so adding to it to land my own change would have been taking the exit the pin exists to discourage. Porting was the honest option anyway: this is "not ported yet", not "Darling-only by architecture". Lite already had the reader and the same duration split in its store, so the tool is thin. Two things kept deliberately identical across the SKUs: The empty answer. Both say "empty" for a quiet window and "unavailable" for a server that has never collected, in the same words. A user moving between the SKUs must not be told a different story about the same state. The field name. Lite's column is duckdb_duration_ms and Darling's is a Postgres write, but both advertise store_duration_ms. The storage engine differs; the question the caller is asking does not, and a consumer should not need to know which SKU answered to read the field. The instructions census counts the real inventory, so it moves 101 -> 102 tools and 76 -> 77 shared; the Darling-unique count is unchanged, which is the arithmetic that says this landed as parity and not as another Darling-only tool. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| Says outright that the cap bit, rather than leaving a caller to infer it by | ||
| counting rows against a limit they may not have set themselves. | ||
| */ | ||
| truncated = rows.Count >= Math.Clamp(limit, 1, 5000), |
There was a problem hiding this comment.
truncated = rows.Count >= Math.Clamp(limit, 1, 5000) gives a false positive whenever the window contains exactly limit rows and no more. The SQL already applies LIMIT $4 (DarlingDataReader.CollectionLogSql), so rows.Count can never exceed the cap — it can only be < cap (true negative, correctly not truncated) or == cap, which this treats as "truncated" even when nothing was cut off. E.g. hours_back=24, limit=200 with exactly 200 runs in the window reports truncated: true, telling the caller data is missing when the full result set was returned.
The four-case test added in this PR only exercises limit=1 against 2 available rows (a real truncation), so it doesn't catch the exact-boundary case. Given the PR's own theme — not sending a caller off "widening a window that will never fill" — this is the same shape of false signal in the other direction (telling them to raise a limit that won't add rows).
Typical fix: fetch cap + 1 rows and slice to cap before returning, using rows.Count > cap (post-slice) as the truncation signal — or a separate existence probe for the cap + 1th row.
Same issue exists in Lite/Mcp/McpHealthTools.cs (truncated = rows.Count >= cap) — please fix both to keep parity.
| server = resolved.ServerName, | ||
| hours_back = hours, | ||
| run_count = rows.Count, | ||
| truncated = rows.Count >= cap, |
There was a problem hiding this comment.
Same off-by-one as the Darling twin: rows.Count >= cap is a false positive when the window contains exactly cap rows and no more (the underlying DuckDB query is itself LIMIT $4-bounded, so rows.Count can never tell you whether a cap+1th row exists). See the paired comment on DarlingMcpDataTools.cs for the reproduction and suggested fix — please fix both together so the two SKUs don't drift.
| /// <summary> | ||
| /// Whether this server has EVER recorded a collector run, ignoring any window. | ||
| /// <para>Lets an empty log read say WHICH kind of nothing it found. "No runs in the last N hours" is | ||
| /// true both of a quiet window and of a server that has never collected, and those want opposite | ||
| /// responses from the caller -- widen the window, versus go find out why collection is not running. | ||
| /// Darling's twin is <c>DarlingDataReader.HasAnyCollectionLogAsync</c>; the two must stay in step so a | ||
| /// user moving between the SKUs is not told a different story about the same state.</para> | ||
| /// </summary> | ||
| public async Task<bool> HasAnyCollectionLogAsync(int serverId) |
There was a problem hiding this comment.
This new HasAnyCollectionLogAsync method (and its doc comment) got spliced in between the existing /// <summary>...</summary> block just above ("Gets recent collection log entries for a server...", ending at the /// </summary> right before this block) and the method it actually documents, GetRecentCollectionLogAsync (a few lines below). As written, this method now has two consecutive <summary> blocks stacked on it, and GetRecentCollectionLogAsync below has lost its doc comment entirely.
Move this new summary block so it directly precedes HasAnyCollectionLogAsync, and restore the original "Gets recent collection log entries..." block directly above GetRecentCollectionLogAsync.
|
Reviewed. The Lite/Darling parity story is solid here — the Left 3 inline comments:
One non-blocking note: the PR description frames the window as "bounded on both sides" for past-incident queries, but the exposed |
I anchored the new probe on the method's SIGNATURE, which put it between that method and the doc block above it. The result reads as my member carrying two summaries; the real damage is that the method whose block it was is now undocumented, and XML docs silently take the last one, so tooling looks fine. Moved rather than deleted -- the hygiene test says so explicitly, and it is right here: the first block documents a different member, and deleting it would have lost that documentation instead of deduplicating anything. Anchor on the previous member's closing brace, not on the next member's signature. Swept the other three files this change touches; none has the same shape. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| var end = DateTime.UtcNow; | ||
| var start = end.AddHours(-Math.Abs(hours_back)); | ||
| var rows = await DarlingDataReader.GetCollectionLogAsync( | ||
| postgres, resolved.ServerId, start, end, Math.Clamp(limit, 1, 5000)); |
There was a problem hiding this comment.
Bypasses the established hours_back/limit validation convention. Every sibling data-read tool with these two parameters (get_wait_stats, and ~130 other call sites across Darling/Lite) validates them via McpHelpers.ValidateHoursBack(hours_back) / McpHelpers.ValidateTop(limit), which reject out-of-range input with a clear error and cap at MaxHoursBack (168h/7 days) and MaxTop (1000).
This tool instead silently coerces: Math.Abs(hours_back) (no upper bound at all — a caller can request a window of years, unlike every other windowed tool) and Math.Clamp(limit, 1, 5000) (a bespoke ceiling 5x higher than MaxTop). Two behavioral consequences:
- No
hours_backupper bound means an agent (or a typo) can trigger a full-history scan ofcollection_log/v_collection_logwhere every other tool would reject the request outright. - A caller passing
hours_back = -5gets silently treated as5instead of getting the same rejection every other tool gives for non-positive input.
Same pattern in the Lite twin (Lite/Mcp/McpHealthTools.cs GetCollectionLog), so this is a consistent deviation across both apps rather than a one-off.
| Says outright that the cap bit, rather than leaving a caller to infer it by | ||
| counting rows against a limit they may not have set themselves. | ||
| */ | ||
| truncated = rows.Count >= Math.Clamp(limit, 1, 5000), |
There was a problem hiding this comment.
truncated has a false-positive at the exact boundary. rows.Count >= Math.Clamp(limit, 1, 5000) reports truncated: true whenever the number of matching rows exactly equals the cap — even when there is no more data beyond it. E.g. a server with exactly 200 runs in the default 24h window will be told the log was truncated, when in fact every row was returned.
The PR description frames truncated as an authoritative signal ("the cap announces itself instead of being inferred"), so this false positive undermines that guarantee. The usual fix is to fetch cap + 1 rows (or a separate COUNT(*)) and check rows.Count > cap after trimming to cap, rather than comparing the already-capped count against the cap.
Same bug in the Lite twin at Lite/Mcp/McpHealthTools.cs:326 (truncated = rows.Count >= cap).
|
Reviewed the new
A couple of smaller, non-blocking notes:
No T-SQL/install changes in this PR, so the collector style rules don't apply here; the cross-app tool-surface and instructions-census pins look correctly updated and self-consistent. |
The panel descriptor was a raw object literal where every sibling goes through table(). mount()'s appendChildren handles arrays and Nodes and stringifies anything else, so the panel would have rendered as the text [object Object] and, never reaching renderPanel, would never have fetched get_collection_log at all. The web half of this change did not work. It builds, it parses, it passes every pin -- and it does nothing, because the pins check that named reads and vizzes exist, not that a descriptor is constructed the way the mounter can consume. The row cap took Math.Clamp(limit, 1, 5000), silently accepting five times what every sibling read allows and quietly rewriting anything out of range. Now McpHelpers.ValidateTop, which refuses instead: a caller asking for 5000 is told no rather than handed 1000 and left to believe it got 5000. truncated compared the row count to the cap, so a window holding exactly `limit` runs and nothing more reported truncated. That is a false positive in the one field whose entire reason for existing is that the cap should not have to be inferred -- so the read over-fetches by one and reports what it observed. Lite had no test. Its half of the parity promise now has one, at the TOOL level rather than the reader level, since the cap contract and the truncation signal both live in the tool and a reader test would have missed both -- including the truncated bug above, which Lite also had. That test found something on the way in: Lite does not store a server id, it DERIVES one from the storage name. Seeding under a hardcoded id would have written rows the tool looks straight past, and the never-collected assertion would have passed for entirely the wrong reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
All findings addressed. The first one was the important one — thank you for catching it. 1. The panel never rendered and never fetched. Correct, and it is the whole web half of this change. A raw object literal where every sibling goes through Worth naming what this says about the pins: the change built, the JS parsed, and every page-level pin passed. They check that a named read exists, that a named viz is in the vocabulary, that no tab fetches the same read twice — all true of a descriptor the mounter cannot consume. A panel that renders nothing looks identical to one nobody looked at. 2. 3. The row cap. On the parity note: Lite got One thing I overclaimed and am correcting rather than quietly fixing: the description said the two-sided window lets a caller ask about a past incident. The reader is bounded on both sides and the viewer uses that, but the MCP tool exposes only |
| var invalidLimit = McpHelpers.ValidateTop(limit); | ||
| if (invalidLimit != null) return invalidLimit; | ||
|
|
||
| try | ||
| { | ||
| var end = DateTime.UtcNow; | ||
| var start = end.AddHours(-Math.Abs(hours_back)); |
There was a problem hiding this comment.
Every sibling hours_back tool in this file validates with McpHelpers.ValidateHoursBack(hours_back) (often chained with ValidateTop, e.g. get_wait_trend, get_cpu_utilization, get_top_procedures_by_cpu...). This tool skips that and instead does Math.Abs(hours_back) inline. Consequences:
hours_back <= 0silently becomes a valid (possibly zero-width) window instead of erroring like every other tool.- There's no
MaxHoursBack(168h/7-day) ceiling, so a caller can request an arbitrarily large window — unlike every sibling read, which is capped at 7 days. That's a wider scan againstv_collection_logthan any other collection-log-adjacent read allows. int.MinValuethrows inMath.Abs, and a hugehours_backcan pushDateTime.AddHoursout of range — both get swallowed by the genericcatchinto a bland "Error during get_collection_log..." instead of the clear, actionable validation message every sibling gives.
Suggest calling McpHelpers.ValidateHoursBack(hours_back) (chained with ValidateTop) like the rest of the file does, rather than hand-rolling Math.Abs. Same issue exists in the Lite twin (Lite/Mcp/McpHealthTools.cs), so it's parity-consistent but both need the fix.
| var invalidLimit = McpHelpers.ValidateTop(limit); | ||
| if (invalidLimit != null) return invalidLimit; | ||
|
|
||
| try | ||
| { | ||
| var hours = Math.Abs(hours_back); |
There was a problem hiding this comment.
Same gap as the Darling twin: every other hours_back tool in the Lite MCP surface (McpBlockingTools, McpAlertTools, McpConfigHistoryTools, McpAnalysisTools, McpHealthParserTools, ...) validates with McpHelpers.ValidateHoursBack(hours_back). This tool uses Math.Abs(hours_back) instead, so hours_back <= 0 is silently accepted (rather than rejected), and there's no upper-bound (MaxHoursBack) check at all. Worth aligning with the established pattern used everywhere else in this file family.
|
|
||
| /* ── 5. an out-of-range cap is refused, not silently clamped ── */ | ||
| var tooBig = await DarlingMcpDataTools.GetCollectionLog(dataSource, ServerName, 24, 5000); | ||
| Assert.Contains("Must be a positive integer", tooBig, StringComparison.Ordinal); |
There was a problem hiding this comment.
This assertion looks wrong. McpHelpers.ValidateTop only returns "Must be a positive integer..." for top <= 0; for a value that's positive but over MaxTop (1000), it returns "limit value '5000' exceeds maximum of 1000. Use a smaller value." (see McpHelpers.ValidateTop in PerformanceMonitor.Common/Mcp/McpHelpers.cs). Since limit: 5000 here is positive-but-over-max, the actual returned string will be the "exceeds maximum" message, not "Must be a positive integer" — this assertion should fail once the test actually runs against the live store. Same issue in the Lite twin at Lite.Tests/CollectionLogToolTests.cs:127, which isn't gated behind an env var and will run unconditionally in CI.
| await SeedLogAsync("query_store", DateTime.UtcNow.AddMinutes(-10)); | ||
|
|
||
| var tooBig = await McpHealthTools.GetCollectionLog(service, _serverManager, ServerName, 24, 5000); | ||
| Assert.Contains("Must be a positive integer", tooBig, StringComparison.Ordinal); |
There was a problem hiding this comment.
limit: 5000 is positive but over McpHelpers.MaxTop (1000), so ValidateTop takes the top > MaxTop branch and returns "limit value '5000' exceeds maximum of 1000. Use a smaller value." — not "Must be a positive integer...", which is only returned for top <= 0. This test isn't gated behind a live-store env var, so it runs unconditionally and should fail as written. Same mistaken assertion exists in the Darling twin (Darling/Darling.Tests/DarlingCollectionLogReadTests.cs:131).
|
Reviewed the
No SQL injection concerns (fully parameterized), no secrets/network/process issues spotted. |
ValidateTop has two rejections, not one: below range says "Must be a positive integer", above range says "exceeds maximum of 1000". Both tests asked for 5000 and asserted the below-range wording, so they failed against a validator that was refusing correctly. Now asserts the over-range text and the ceiling itself, so a change to MaxTop shows up here rather than passing on a substring that no longer describes the limit in force. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| try | ||
| { | ||
| var end = DateTime.UtcNow; | ||
| var start = end.AddHours(-Math.Abs(hours_back)); |
There was a problem hiding this comment.
get_collection_log is the only hours_back-taking tool in this file that skips McpHelpers.ValidateHoursBack(hours_back) — every other sibling (get_cpu_utilization, get_wait_trend, get_temp_db_trend, etc., 8/9 occurrences in this file) validates before use. Here hours_back is only ever Math.Abs()'d:
- No upper bound: every other windowed read caps at
MaxHoursBack(168h / 7 days) with a clear error. This tool has no cap, so a caller can request an arbitrarily large window againstv_collection_log— inconsistent with the rest of the API contract and a potential unbounded-scan vector via/api/read/get_collection_log(reachable straight from the web dashboard's query string, not just MCP). hours_back = 0silently becomes a zero-width window instead of the standard "Must be a positive integer (1-168)" error every sibling returns for<= 0.hours_back = int.MinValuemakesMath.AbsthrowOverflowException(caught by the outer try/catch, but surfaces as a raw exception message instead of the clean validation error every other tool gives for bad input).
Worth adding the standard var validation = McpHelpers.ValidateHoursBack(hours_back); if (validation != null) return validation; and dropping the Math.Abs calls, matching every other tool in this file. Lite's twin (Lite/Mcp/McpHealthTools.cs, GetCollectionLog) has the identical gap.
|
|
||
| try | ||
| { | ||
| var hours = Math.Abs(hours_back); |
There was a problem hiding this comment.
Same gap as Darling's twin (DarlingMcpDataTools.GetCollectionLog): this is the only hours_back tool in the file that doesn't call McpHelpers.ValidateHoursBack(hours_back) before use — it just Math.Abs()s it. That drops the 168-hour (7-day) cap every other windowed read enforces, lets hours_back = 0 through as a silent zero-width window instead of a validation error, and lets hours_back = int.MinValue throw OverflowException out of Math.Abs (caught, but surfaces a raw exception string instead of the standard "Must be a positive integer (1-168)" message).
Since both SKUs need to change together for parity, suggest adding the standard McpHelpers.ValidateHoursBack(hours_back) check here too, matching McpCpuTools / McpQueryTools / etc. in this project.
Review summaryReviewed the new What's solid:
One correctness/consistency issue, filed inline on both SKUs: No SQL Server missing-index suggestions offered, per instructions. No secrets/network/process concerns found in this diff. |
First item on #2484's list, and the one it ranks first: the collection log is what people reach for when the rollup says HEALTHY and collection still looks wrong, and it covers two viewer tabs (Collection Log and Duration Trends).
The gap
/api/read/*is machine-derived from the MCP read catalog, so a read with no tool is absent from both surfaces.get_collection_healthserves the seven-day per-collector rollup; the raw per-run log had nothing. No projection of a rollup gives back the individual runs, so the WPF viewer on a Windows desktop was the only way to this data — invisible to the web dashboard and to any agent.No new collection. The store already holds these rows. This reads
v_collection_log, the same view the viewer uses, so the three surfaces cannot drift.Three decisions worth stating
The window is bounded on both sides. Most neighbouring reads take a single now-relative
hours_back. Someone asking about a past incident wants the runs from then, and an hours-back-from-now span cannot express that. Matches how the viewer's own tab windows this read.Zero rows says which kind of nothing it is. This is the substance of the change:
emptyhours_backunavailableBoth are literally "no runs in the last 24 hours". The responses are opposites — telling the second caller to widen the window sends them off filling one that will never fill. That is exactly the defect #2485 catalogues across six other reads, so this one is not allowed to add a seventh. It costs one
LIMIT 1probe, taken only on the empty path.Duration is split three ways — total, time on the monitored server, time writing to the store. A collector slow because the target is slow needs a different fix from one slow because the store is, and the total alone cannot tell them apart. That is the first question anyone asks of a slow collector, and it is why the raw log earns its place beside the rollup.
Verification
Live store test (
DarlingCollectionLogReadTests, runs in the PostgreSQL job) covers all four paths:unavailable, message contains "EVER", and asserts it does not say "widen".empty, asserts it does not say "EVER", and does say "widen". Same row count as case 1 — zero — and it must not reach for the same word.sql/storesplit values.limit: 1→truncated: true, so the cap announces itself instead of being inferred.Service and test projects build clean, 0 errors. JS parses clean.
The existing parity pin (every
[McpServerTool]needs a catalog entry) self-enforces the wiring; the 12-tab pin and the no-duplicate-read-per-tab pin both still hold, since this adds a panel to an existing tab rather than a tab.Remaining on #2484
Blocking Stats, Current Waits, Significant Waits, the three Performance Trends siblings, Query Store Regressions, and the heatmap — in the order the issue suggests.