Repository navigation
Add the alert-notebook binding endpoint (#4222) - #4366
Merged
Merged
Conversation
Refs #4222. GET /api/alert-notebook resolves an alert link (server, metric, at, dedup) into a bound, read-only notebook definition: the dedup-matched alert (falling back to the nearest same-metric row), a four-arm status (Resolved / Fired again / No resolution recorded / Unknown), and a mechanical notebook template built from the same per-metric read list DarlingTriageEndpoint.SectionsFor already serves. Reuses DarlingTriageEndpoint's ResolveAnchor, ResolutionAliases, SectionsByMetric/SectionsFor, IsFleetLevelStoreServer and IsFleetLevelStoreMetric rather than copying them; widens AnchorSlack and AlertMatchLookback to internal for the same reason.
This was referenced Sep 26, 2026
Adds AlertNotebookEndpointTests.cs covering: - unauthenticated GET /api/alert-notebook returns 401 before the handler can dial the store (dead-pool proof, mirrors DarlingWebHostGateLiveTests) - out-of-CIDR remote is forbidden before the token gate - hostile/malformed authenticated queries (far-future at, SQL-ish server/ dedup, an over-long metric) never 500 and carry no exception text - dedup match beats a nearer-in-time row; no dedup falls back to nearest - window math: window_end = min(anchor + AnchorSlack, now), and a far-future at clamps rather than widening the window - the four status arms are distinct strings and none say "ongoing" - every mechanical read cell names a BuildReadDispatch key (census) - a stale/unknown alert link degrades to a non-error response
erikdarlingdata
marked this pull request as ready for review
September 26, 2026 01:53
erikdarlingdata
added a commit
that referenced
this pull request
Sep 26, 2026
* Refs #4222. Client render for the alert notebook (SPA slice) - panels.js renderPanel and compose.js renderComposedPanelCard take an optional onSettled callback, fired once a cell's load reaches a terminal state, so a caller with its own concurrency budget knows when a slot frees. - views.js renderNotebookDoc is refactored to accept either a saved view (mode: saved, unchanged behaviour) or an in-memory definition bound to a firing (mode: alert, read-only): no fleet scope picker, a max-3 in-flight InFlightLimiter gating read-cell loads, header/status cells rendered directly from the endpoint's own facts, Save as notebook and Open live controls. - triage.js tries GET /api/alert-notebook first and renders its definition through renderNotebookDoc in alert mode; on a 404 (endpoint not present on this build) it falls back to the existing /api/triage assembly unchanged. - New pin tests (Darling.Tests/AlertNotebookRenderClientTests.cs) over the shipped JS source text: in-memory render path, no /api/fleet call in alert mode, the max-3 limiter, and the Save-as-notebook provenance string. Built against PR #4366 (feature/4222-alert-notebook-endpoint, head ee63879) for the /api/alert-notebook response shape. Status refresh on the 60s poll is out of scope (needs slice d's poll guard on dev). Not opening a PR yet per the coordinator's rule: pins exist and build 0/0; a successor lane should verify the shape against #4366 once it lands and open the PR. * Render current_value/threshold_value/detail_text; guard alert-mode read cells on route change (#4368) * Fix #4368 CI: scope innerHTML pin to renderAlertHeaderCell, update panels.js loadPanel pin for onSettled * Fix #4368 pin false positives: strip JS comments before innerHTML check, add census entry
This was referenced Sep 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #4222.
Why
The alert-notebook MVP (#4222, Slice C) needs a server endpoint that turns an alert link (
server,metric,at,dedup) into a bound, read-only notebook definition. Today's#/triagepage matches by nearest time only and cannot become a notebook (its sections aren'treadcells). This PR is the (a) slice: the newGET /api/alert-notebookendpoint. Two parallel #4222 lanes cover the notebookreadcell type +views.jsrendering (a2) and the shell play/pause control (d); this PR does not touchValidateNotebookDefinitionor any JS.What changes
Darling/PerformanceMonitor.Darling.Service/AlertNotebookEndpoint.cs, mapped fromDarlingWebEndpoints.MapAllright afterDarlingTriageEndpoint.Map, same auth-middleware placement as every sibling route.GET /api/alert-notebook?server&metric&at&dedupreturns:{ "alert": { "alert_time", "server_id", "server_name", "metric_name", "current_value", "threshold_value", "detail_text", "incident_since", "involved_objects", "database", "total_occurrences" } | null, "status": "Resolved at <T>" | "Fired again at <T>" | "No resolution recorded" | "Unknown (not collected since <T>)", "notes": [ "..." ], "template": { "id": "mechanical/<metric>", "version": 1 }, "definition": { "kind": "notebook", "cells": [ { "type": "header", "title", "server", "alert_time", "incident_since", "involved_objects", "database", "total_occurrences" }, { "type": "status", "title": "Status", "status": "<one of the four status strings>" }, { "type": "read", "read": "<catalog read name>", "params": { "server", "as_of", "hours", "limit", ... }, "viz": "table", "title": "<section title>" }, ... ] } }alertmatch: scans same-metric history rows in the anchor window for an incident whosecontext_jsoncarries the link'sdedupkey (viaAlertContextSerializer.TryDeserialize); dedup wins over nearer-in-time. With nodedup, falls back to today's nearest same-metric row, same asDarlingTriageEndpoint.status, four arms: a later resolution-alias row afterat-> "Resolved at T"; else a later same-metric firing -> "Fired again at T"; else the metric's server has av_collection_logrow at or afterat-> "No resolution recorded"; else -> "Unknown (not collected since T)". Never "ongoing". A fleet-level metric (noserverId) always reads Unknown when neither of the first two arms fires — freshness can't be checked without a server.window_end = min(at + 15 min, now)viaDarlingTriageEndpoint.ResolveAnchor/AnchorSlack; a futureatclamps to now (inherited fromResolveAnchor). The alert match uses the sameAlertMatchLookbackbound before the anchor thatDarlingTriageEndpointuses and picks the key match nearest the anchor; the status arms read forward tomin(anchor + 24 h, now), so a later resolution or re-fire is found. A server name that doesn't resolve (and isn't fleet-level) reads Unknown rather than other servers' rows.DarlingTriageEndpoint.SectionsFor(metric)(or its fallback) into one{type:"read"}cell per section, each carryingserver,as_of, and the section's ownhours/limitwhere declared, else a24-hour /50-row default.template.id = "mechanical/<metric>",version = 1. The authored blocking/deadlock templates are explicitly not part of this slice — every metric, including those two, gets the mechanical conversion for now.DarlingTriageEndpoint.AnchorSlackandAlertMatchLookbackfromprivatetointernal(both alreadyTimeSpanconstants); every other member this endpoint calls (ResolveAnchor,ResolutionAliases,SectionsByMetric/SectionsFor,IsFleetLevelStoreServer,IsFleetLevelStoreMetric,TriageSection,DefaultSections) was alreadyinternaland is called directly, not duplicated.alert, never an error — same posture asDarlingTriageEndpoint(Alert deliveries lack Datadog-parity structure: no structured context tags, no linked triage artifact #2710).Known gaps (this slice only)
readcells), so no cell carries the spec's absolute per-cellrange {windowStart, windowEnd}yet —DarlingTriageEndpoint.TriageSectionhas no composed-cell concept to convert. That lands with authored/composed templates in a later slice.Lite parity: N/A (Darling web only).
Test plan
dotnet build Darling/Darling.Tests/Darling.Tests.csproj -p:EnableWindowsTargeting=true— 0 Warning(s), 0 Error(s)dotnet build Lite.Tests/Lite.Tests.csproj -p:EnableWindowsTargeting=true— 0 Warning(s), 0 Error(s)BuildReadDispatch, stale-link degrade, unauthenticated zero-store-commands) — not yet written; both test projects targetnet10.0-windowsand cannot run on this macOS lane. CI decides.CHANGELOG entry
SECTION: Added
ENTRY:
GET /api/alert-notebook, which turns an alert link into a bound, read-only notebook definition (a dedup-matched alert, a four-arm live status, and a mechanical per-metric read cell list), the server half of the alert-to-notebook MVP (Alert notebook MVP: #/triage renders a read-only notebook bound to the firing (blocking and deadlocks authored, every other family converted) #4222).REF:
[Add the alert-notebook binding endpoint (#4222) #4366]: Add the alert-notebook binding endpoint (#4222) #4366
For the coordinator
The SPA render slice (a2/other #4222 lanes) consumes this exact JSON shape from
GET /api/alert-notebook:{ alert, status, notes[], template: {id, version}, definition: {kind: "notebook", cells: [...]} }.alertis eithernullor an object with keys:alert_time(ISO-8601oformat),server_id,server_name,metric_name,current_value,threshold_value,detail_text, and — only present when an incident matched —incident_since(ISO-8601 or null),involved_objects(comma-joined string),database(string or null),total_occurrences(number or null).statusis always one of exactly four strings:"Resolved at <yyyy-MM-ddTHH:mm:ssZ>","Fired again at <yyyy-MM-ddTHH:mm:ssZ>","No resolution recorded","Unknown (not collected since <yyyy-MM-ddTHH:mm:ssZ>)".definition.cells[0]is always{type: "header", title, server, alert_time?, incident_since?, involved_objects?, database?, total_occurrences?}— a directly-rendered cell, not areadcell.definition.cells[1]is always{type: "status", title: "Status", status}— same non-readshape.definition.cells[2..]are{type: "read", read, params: {...}, viz: "table", title}— one perSectionsFor(metric)entry.paramsalways carrieshoursandlimitexplicitly (never omitted), plusserver/as_ofwhen applicable (fleet-level sections omitserver).rangeare emitted yet (see Known gaps above) — the render slice should treat everydefinition.cellsentry asheader|status|readfor now.AlertNotebookEndpoint.Map,AlertNotebookEndpoint.ResolveStatusAsync,AlertNotebookEndpoint.ReadCell,AlertNotebookEndpoint.HeaderCell. File:Darling/PerformanceMonitor.Darling.Service/AlertNotebookEndpoint.cs.pm-pr lane report (pins)
Head after this lane:
b3e0f8e8(wasee638793; branch has not moved otherwise).New file:
Darling/Darling.Tests/AlertNotebookEndpointTests.cs. Build:0 Warning(s), 0 Error(s)onDarling/Darling.Tests/Darling.Tests.csproj(net10.0-windows, macOS host,-p:EnableWindowsTargeting=true).Pins added (all in the new file unless noted):
Security (merge blocker), first.
Unauthenticated_GetAlertNotebook_Returns401_AndNeverDialsTheStore— mirrorsDarlingWebHostGateLiveTests.NetworkMode_NoToken_ApiPath_Returns401Jsonexactly, against/api/alert-notebook, with the data source pointed at a closed local port (Port=1) the same wayWebReadCancellationPinTests.DeadStore()does. The auth gate answers 401 JSON beforeAlertNotebookEndpoint's lambda runs, so the dead pool is never dialed — the assertion is the 401 + JSON body + no<html, same shape the sibling triage/fleet routes already prove.OutOfCidrRemote_GetAlertNotebook_IsForbidden_BeforeTheHandlerRuns— the OIDC sign-in for the web dashboard: give the web surface a per-user identity instead of one shared token #2550 CIDR-outermost gate, same dead-pool proof.AuthenticatedHostileQuery_NeverReturns500_AndCarriesNoExceptionText([Theory], 4 cases: far-futureat, SQL-shapedserver, an over-longmetric(4096 chars), SQL-shapeddedup) — asserts never 500, always 200 or 400, and the body never contains"Exception"/"StackTrace"(the Web viewer: tool errors still show the exception text in the browser #4283/Web: /api/ping detail and two degraded notes carry raw exception text (needs a ruling) #4316 rule).FarFutureAt_StillDegradesCleanly_NoWiderWindowSignalOnTheWire— confirms the future-atclamp doesn't open a distinct wider-window code path on the wire.DarlingWebHostGateLiveTests) byte-for-byte in shape, so risk of a false pass is low, but this is unchecked locally, CI decides.Dedup beats the nearer row:
DedupMatch_BeatsNearerRow_WhenTheNearerRowHasADifferentKey,NoDedupKey_FallsBackToNearestRow— pure logic reproducing the handler's exact scan-then-fallback rule (the handler has no extracted method to call directly; it's one inline lambda). GREEN, verified via a throwaway net10.0 console harness (see below).Four status arms, never "ongoing":
UnknownArm_NeverContainsTheWord_Ongoing,FourStatusArms_AreDistinctStrings_AndNoneSayOngoing. GREEN via the harness.Window math / future-
atclamp:ResolveAnchor_FarFutureAt_ClampsToNow_NotUnboundedForward(calls the realDarlingTriageEndpoint.ResolveAnchor, xunit — runs on CI only, but is a direct call to existing pure code so risk is minimal),WindowEnd_IsAnchorPlusSlack_ClampedToNow,WindowEnd_RecentAnchor_ClampsAtNow_NeverExceedsIt. GREEN via the harness for the window-math pair;ResolveAnchor_FarFutureAt_ClampsToNow_NotUnboundedForwardmatches the harness'sFarFutureAt_ClampsToNow/FarFutureAt_AsOfNullcases exactly (also GREEN).Mechanical templates name only
BuildReadDispatchreads:EveryMechanicalReadCell_NamesAKnownDispatchEntry— census over everyDarlingTriageEndpoint.SectionsForentry (every mapped metric +DefaultSections), asserting every.Readkey is inDarlingWebEndpoints.BuildReadDispatch(). GREEN via the harness (0 offenders across the full section set).Stale-link degrade:
UnknownMetricAndDedup_Returns200_WithEmptyAlertAndSections_NotAnError— an unknown metric+dedup against the dead pool never returns 500. CI-only (TestServer).How the pure-logic pins were verified GREEN without a Windows test run: a throwaway net10.0 console project (
AssemblyName=Darling.TestssoInternalsVisibleToonPerformanceMonitor.Darling.Serviceapplies) referencing the Service project directly, re-implementing each pure assertion inline and calling the realDarlingTriageEndpoint.ResolveAnchor/AnchorSlack/SectionsFor/SectionsByMetricandDarlingWebEndpoints.BuildReadDispatch(). All 11 checks printedPASS,TOTAL: 0 FAIL. Deleted after use; not part of the commit.Endpoint review findings:
windowStart(line ~199 ofAlertNotebookEndpoint.cs) is computed (anchor - AlertMatchLookback, adjusted byincidentSince) but never read anywhere after — it doesn't feedwindowEnd, isn't passed to any read cell, and isn't in the response body. Either dead code left over from an earlier draft, or a real gap (the read cells' ownhours/as_ofparams don't derive from it either — they useFamilyLookbackHours+asOffromResolveAnchorinstead). Filing as a note here rather than fixing blind, since removing it or wiring it into the cells are both plausible and the correct one depends on intent I can't infer from the diff alone. This should be triaged before merge or as an immediate fast-follow.Mapis called fromMapAllafterDarlingTriageEndpoint.Map, inside the sameConfigurePipelinegate chain — no bypass found.CancellationTokenthreading:context.RequestAbortedis threaded intoGetAlertHistoryAsyncandResolveStatusAsync'sExecuteScalarAsynccall. This route isn't inBuildReadDispatch(it's a dedicatedMap, like/api/triage), soWebReadCancellationPinTests's dispatch-table ratchet doesn't cover it — same posture as/api/triageitself, which also isn't in that ratchet. No gap introduced beyond the existing triage-endpoint precedent.catchblocks route throughDarlingWebFailureLog.Report+ a canned note string, matching Web viewer: tool errors still show the exception text in the browser #4283/Web: /api/ping detail and two degraded notes carry raw exception text (needs a ruling) #4316.DarlingServerResolver.ResolveOrErrorAsyncand the alert-history read are parameterized (Npgsql$1/$2/... placeholders), so the SQL-shapedserver/deduptheory cases are string comparisons only, not injection vectors — confirmed by readingPgAlertHistoryStore.cs's parameter binding.Not run locally (CI-only): the whole
Darling.Testsproject is net10.0-windows; only the throwaway harness ran the pure-logic pins. No live Postgres container was needed for verification — I startedpmpr-4366(port 55460) briefly per the brief but the pins never required it (the security/degrade pins use a dead-port pool by design, matchingWebReadCancellationPinTests's own pattern); it has been removed.CHANGELOG entry: already correctly labeled
[#4366], linkingpull/4366— no fix needed.Left unchecked, with reason: the 4 live-HTTP security theory/fact tests and the stale-link degrade test compile but did not run locally (no Windows runtime here); CI's
buildjob runs the full suite. ThewindowStartdead-variable finding above is reported, not fixed, per the "don't fix blind" rule when intent is ambiguous.pm-pr lane report (windowStart)
Question:
windowStart(~line 199, before this change) was computed but never used downstream — dead code, or a broken window binding?Answer: (b) dead code, per the #4222 spec. The spec's "Definition" section binds windows two ways:
window_end = min(at + 15 min, now)is the shared anchor, but the pairedwindow_start = min(Incident Since, at) − family lookbackonly feeds a composed cell's absoluterange {windowStart, windowEnd}(#2735/#2788). Read cells getserver,as_of = window_end, andhours = lookback— never arange. This slice's class doc already says so explicitly: "No composed cells are emitted here, so no cell carries an absolute range yet — that is this slice's honest gap, not an oversight."DarlingTriageEndpoint's own precedent binds the same way: its sections threadserver/as_ofper read, never a start/end pair. So every read cell here already carries the correct half of the window (as_of);windowStarthad no consumer because this mechanical slice emits no composed cells, and removing it changes no behavior.Change: removed the unused
windowStartlocal (and the now-deadincidentSincelocal that only fed it) fromAlertNotebookEndpoint.cs, and rewrote the window-math comment abovewindowEndto say plainly why only the end is computed here.JSON shape: unchanged — no new keys, no removed keys.
#4368's SPA consumer needs no changes.CHANGELOG: unaffected (no behavior change; the entry already covers the endpoint's addition).
Build:
Darling/Darling.Tests/Darling.Tests.csproj— 0 Warning(s), 0 Error(s) on macOS, before and after the merge fromorigin/dev(clean, no conflicts).Head:
b3e0f8e8→f2ae68c3(dead-code removal commit34cc7dc4+ merge commitf2ae68c3, no rebase).pm-pr lane report (review fixes F1-F4, test auth)
New head:
cabf3ebc(wasf2ae68c3). Branchfeature/4222-alert-notebook-endpoint, draft, unchanged base.Extraction
Pulled the match logic out as
internal static (Row, Incident) MatchAlert(rows, metric, dedup, anchor)and the two forward status arms out asinternal static string? StatusFromHistory(rows, metric, anchor, matchedRow, serverId, fleetLevelStore)onAlertNotebookEndpoint. Both are pure over their inputs;StatusFromHistoryreturns null when neither arm fires so the caller falls through to the collector-freshness arms unchanged.F1 (late resolution/re-fire)
Added a second, forward-looking history read
[anchor, min(anchor + AlertMatchLookback, now)], same row cap (200), cancellable onRequestAborted. Its rows are unioned with the original match-window rows and passed toResolveStatusAsync/StatusFromHistory. The notebook's cell window (windowEnd/asOf) is untouched.F2 (unresolved-server leak)
StatusFromHistorynow returnsUnknown (not collected since …)immediately whenserverId is null && !fleetLevelStore. When a server id is known, rows are filtered to that server before either arm runs.F3 (dedup nearest, not newest)
MatchAlertnow scans every dedup match and keeps the one nearestanchor(ties toward the earlier row), instead of the first (effectively newest, since rows arrive DESC) match.F4 pins
MatchAlert: nearest-wins over a same-key re-fire in the tail (2 variants), dedup beats a nearer non-matching row, no-dedup falls back to nearest — all against realAlertHistoryReadRow+AlertContextSerializer-builtContextJson.StatusFromHistory: resolution 40 min later reads Resolved (F1), another server's resolution is ignored when unresolved (F2), a known server ignores another server's rows, a later same-metric firing reads "Fired again", and a four-arms-distinct/no-"ongoing" census against the seam's real outputs.DedupMatch_*/NoDedupKey_*pins (they copied logic into the test body) and kept the existing window/anchor/census pins as-is.AuthenticatedRequest_ReturnsTheDocumentedNotebookShape: authenticatedserver=probe&metric=High+CPUagainst the dead-store fixture; assertsstatusstarts "Unknown (not collected since ",definition.kind == "notebook", cell[0]/[1] types header/status, every cell[2..] is typereadwithlimit+hoursparams,template.id == "mechanical/High CPU".CI auth fix (pm-pr's follow-up finding)
CI showed 4 "hostile query" cases getting 302 instead of 200/400 — the old pins presented
?token=but never followed the token→cookie exchange, so they hitSetCookieAndRedirectand never reached the handler. AddedSendAuthenticated: sends?token=once, asserts 302 + capturesSet-Cookie, then resends the real request with that cookie — the same two-step exchangeDarlingWebHostGateLiveTestsproves the gate performs. Applied to the hostile-query theory, the far-future-atpin, the stale-link pin (now asserts exactly 200, not "not 500"), and the new shape pin.Build
dotnet build Darling/Darling.Tests/Darling.Tests.csproj: 0 Warning(s), 0 Error(s) on macOS. xUnit/live suites are net10.0-windows only; CI decides those.JSON shape
Unchanged — no edits to
AlertRowNode,HeaderCell,StatusCell,ReadCell, or the response object's key set.git merge origin/devwas already up to date (no new dev commits since f2ae68c).Not done
F5/F6 and the PR body text are pm-pr's per the brief; not touched here.