diff --git a/.workhorse/specs/monitoring/incidents.md b/.workhorse/specs/monitoring/incidents.md index 337be083..9c2fa858 100644 --- a/.workhorse/specs/monitoring/incidents.md +++ b/.workhorse/specs/monitoring/incidents.md @@ -28,6 +28,10 @@ Lingering damps reporter flapping, not operator action: a last failure leaving t The membership history — which issues joined and left, and when — is kept and presented as the incident's timeline. An issue can leave and rejoin the same incident. +The timeline leads with what is worst rather than what is newest: issues are ordered by effective result, most severe first, and issues sharing a result are ordered most recent first. +An issue with no recorded result is ordered below every graded one. +Notes are ordered most recent first and sit below every issue. + Operator actions that change what counts (monitoring toggles, group membership changes, policy and silence changes) re-evaluate the affected issues' incident membership. Membership evaluation is asynchronous. A report records its issue state immediately; the resulting open, join, leave, or close follows within a short bounded delay rather than synchronously with the report. Membership is therefore eventually consistent with the current issue state. diff --git a/private-web/e2e/incident-ordering.spec.ts b/private-web/e2e/incident-ordering.spec.ts new file mode 100644 index 00000000..f39d005c --- /dev/null +++ b/private-web/e2e/incident-ordering.spec.ts @@ -0,0 +1,129 @@ +import type { Page } from "@playwright/test"; +import { expect, test } from "./test-fixtures"; +import { + resetSeededTables, + seedIncident, + seedIncidentNote, + seedIssue, + seedServer, + seedServerGroup, +} from "./seed"; + +// An incident's timeline leads with what is failing. Ordering is severity +// first, most recent second, and notes sit below every issue — an operator +// opening a page mid-incident shouldn't have to read a chronology to find +// the failure. Each fixture below is seeded so that ordering by time alone +// would produce the opposite order, so a passing assertion can only mean the +// severity rank is being applied. +test.describe("incident timeline ordering", () => { + test.beforeEach(async ({ sql }) => { + await resetSeededTables(sql); + }); + + async function seedTimeline(sql: Parameters[0]) { + const group = await seedServerGroup(sql, { name: "ordering-group" }); + const server = await seedServer(sql, { + name: "ordering-server", + kind: "central", + rank: "production", + groupId: group.id, + }); + + const minutesAgo = (n: number) => + new Date(Date.now() - n * 60_000).toISOString(); + + // Assertions key off the message, which is the row's always-visible + // summary text. The check name only shows in the expanded provenance + // line, and a recovered issue renders collapsed. + // + // Deliberately inverted: the newest thing is the least severe, so + // time-ordering alone would put the warning above the failure. + const warning = await seedIssue(sql, { + serverId: server.id, + ref: "health/freshwarning", + severity: "warning", + message: "marker-warning-newest", + }); + const failure = await seedIssue(sql, { + serverId: server.id, + ref: "health/oldfailure", + severity: "error", + message: "marker-failure-oldest", + }); + const recovered = await seedIssue(sql, { + serverId: server.id, + ref: "health/middlerecovered", + active: false, + message: "marker-recovered-middle", + }); + + const incident = await seedIncident(sql, { + serverGroupId: group.id, + openedAt: minutesAgo(60), + issues: [ + { issueId: warning.id, joinedAt: minutesAgo(5) }, + { issueId: recovered.id, joinedAt: minutesAgo(30) }, + { issueId: failure.id, joinedAt: minutesAgo(60) }, + ], + }); + // Newest entry on the page, and still below every issue. + await seedIncidentNote(sql, { + incidentId: incident.id, + body: "notewrittenlast", + createdAt: minutesAgo(1), + }); + return { group, server, incident }; + } + + /** Where each marker first appears in the rendered page text. Rows render + * in order, so a marker unique to one row first appears inside it, and + * comparing offsets compares row positions. */ + async function positionsOf(page: Page, markers: string[]): Promise { + const body = await page.locator("body").innerText(); + return markers.map((marker) => { + const at = body.indexOf(marker); + expect(at, `${marker} should be on the page`).toBeGreaterThan(-1); + return at; + }); + } + + test("failures sort above warnings, recoveries and notes", async ({ + page, + sql, + }) => { + const { incident } = await seedTimeline(sql); + + await page.goto(`/incidents/${incident.id}`); + await expect(page.getByText("marker-failure-oldest").first()).toBeVisible(); + + const [failure, warning, recovered, note] = await positionsOf(page, [ + "marker-failure-oldest", + "marker-warning-newest", + "marker-recovered-middle", + "notewrittenlast", + ]); + + expect(failure, "the failure leads, despite being the oldest").toBeLessThan( + warning, + ); + expect(warning, "the warning outranks the recovered check").toBeLessThan( + recovered, + ); + expect(recovered, "notes sit below every issue").toBeLessThan(note); + }); + + test("the issues filter keeps the same order", async ({ page, sql }) => { + const { incident } = await seedTimeline(sql); + + await page.goto(`/incidents/${incident.id}`); + await page.getByText(/^Issues \(/).click(); + await expect(page.getByText("marker-failure-oldest").first()).toBeVisible(); + + const [failure, warning] = await positionsOf(page, [ + "marker-failure-oldest", + "marker-warning-newest", + ]); + expect(failure).toBeLessThan(warning); + await expect(page.getByText("notewrittenlast")).toHaveCount(0); + }); +}); diff --git a/private-web/e2e/seed.ts b/private-web/e2e/seed.ts index a9c392b9..05d1a1a7 100644 --- a/private-web/e2e/seed.ts +++ b/private-web/e2e/seed.ts @@ -629,6 +629,32 @@ export async function seedIncident( return { id }; } +/** Add an operator note to an incident's timeline. */ +export async function seedIncidentNote( + sql: Sql, + opts: { + incidentId: string; + author?: string; + body?: string; + /** ISO 8601; defaults to NOW(). */ + createdAt?: string; + }, +): Promise<{ id: string }> { + const id = randomUUID(); + await sql.query( + `INSERT INTO incident_notes (id, incident_id, author, body, created_at) + VALUES ($1, $2, $3, $4, COALESCE($5::timestamptz, NOW()))`, + [ + id, + opts.incidentId, + opts.author ?? "operator@example.com", + opts.body ?? "a note", + opts.createdAt ?? null, + ], + ); + return { id }; +} + export interface SeededVersion { id: string; major: number; diff --git a/private-web/src/routes/IncidentDetail.tsx b/private-web/src/routes/IncidentDetail.tsx index 32a0b433..9266a6f8 100644 --- a/private-web/src/routes/IncidentDetail.tsx +++ b/private-web/src/routes/IncidentDetail.tsx @@ -30,12 +30,15 @@ import ResolverAvatar from "../components/ResolverAvatar"; import { usePageTitle } from "../hooks/usePageTitle"; import { humanDuration } from "../lib/humanDuration"; import { + CHECK_RESULT_ORDER, RESOLVED_REASONS, RESOLVED_REASON_LABEL, isIncidentLingering, + type CheckResult, type IncidentIssueData, type IncidentNoteData, type IncidentWithIssues, + type IssueData, type ResolvedReason, } from "../types"; @@ -404,6 +407,24 @@ type TimelineEntry = | { kind: "issue"; at: number; issue: IncidentIssueData } | { kind: "note"; at: number; note: IncidentNoteData }; +const CHECK_RESULT_RANK = new Map( + CHECK_RESULT_ORDER.map((result, index) => [result, index]), +); + +/// Where an issue sits in the severity ranking, by its *effective* result — +/// the graded one everything else acts on, so a check an operator capped at +/// warning ranks as a warning here too. +/// +/// An issue with no recorded result predates the check-state model and has no +/// severity to rank on, so it sorts below everything graded rather than above +/// it, which is where an unknown lands if you index it directly. +function issueSeverityRank(issue: IssueData): number { + const rank = issue.effective_result + ? CHECK_RESULT_RANK.get(issue.effective_result as CheckResult) + : undefined; + return rank ?? CHECK_RESULT_ORDER.length; +} + function Timeline({ issues, notes, @@ -425,7 +446,19 @@ function Timeline({ note: n, })), ]; - entries.sort((a, b) => b.at - a.at); + // Issues above notes, ranked by severity so whatever is failing is at the + // top of the incident and an operator doesn't have to read a whole + // chronology to find it. Ties, and notes among themselves, fall back to + // most recent first. + entries.sort((a, b) => { + if (a.kind !== b.kind) return a.kind === "issue" ? -1 : 1; + if (a.kind === "issue" && b.kind === "issue") { + const bySeverity = + issueSeverityRank(a.issue.issue) - issueSeverityRank(b.issue.issue); + if (bySeverity !== 0) return bySeverity; + } + return b.at - a.at; + }); if (entries.length === 0) { return No timeline entries yet.;