Repository navigation
The alert notebook finds an alert's resolution wherever it sits, and a fleet-level store alert no longer reads as a stopped collector (#4755, #4756) - #4778
Merged
Conversation
… reading a store alert's silence as a stopped collector (#4755, #4756) The notebook's status checked a 24-hour, 200-row, newest-first window of config_alert_log that skipped dismissed rows. A resolution that landed more than 24 hours after the alert, that the viewer's "Dismiss all" had hidden, or that sat behind 200 newer rows (a fleet-level alert has no server id, so one cap covered every server) was invisible, and the page said "Fired again" or "Unknown (not collected since ...)" for an alert that had cleared. The status now reads the first resolution row and the first later firing of the metric with two targeted reads: strictly after the alert, no later than now, dismissed rows included, oldest first, one row, scoped to the server only when its id is known. The names the resolution read looks for come from one list (ResolutionAliases plus the recovery edges) that the status classification uses too. StatusFromHistory is unchanged apart from reading that list. A fleet-level store alert with no resolution row and no later firing now reads "No resolution recorded". A store self-alert has no collector, and the process that evaluates it serves the page, so "Unknown (not collected since ...)" told the reader an instrument was down when it could not be.
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.
Fixes #4755
Fixes #4756
Why
The alert notebook shows a status for each alert: "Resolved at T", "Fired again at T", "No resolution recorded", or "Unknown (not collected since T)". The first two came from one read of
config_alert_log: the rows from the alert time to 24 hours after it, at most 200, newest first, with dismissed rows left out. That read missed a resolution in three cases:The page then said "Fired again", or for a fleet-level store alert "Unknown (not collected since ...)". That last text says a collector stopped, but a store self-alert has no collector, and the process that evaluates it also serves the page (#4756).
The page never showed a false "Resolved" or a false "ongoing", so this is a display fix.
What changes
DarlingAlertReadergets two targeted reads next toGetAlertHistoryPageAsync:GetFirstResolutionAfterAsyncandGetFirstRefireAfterAsync. Each readsconfig_alert_logfor the earliest row withalert_timestrictly after the alert and no later than now, whosemetric_nameis in a list of names (= ANY($3)), one row, dismissed rows included.server_id = $4is added only when the server id is known. The re-fire read also skips the matched row's ownalert_time, so the alert the page is about is not its own re-fire. The row mapping is shared with the page read.AlertNotebookEndpoint.ResolutionRowNames(metric)lists the stored names that resolve a metric: everyDarlingTriageEndpoint.ResolutionAliasesentry whose canonical name is the metric, plus the recovery name of eachNotebookRecoveryEdgespair whose firing name is the metric. The input metric is matched case-insensitively, but the names come back exactly as the product writes them, because the SQL compares with=.StatusFromHistorynow takes its list of resolution names from the same method, so the read and the classification cannot disagree. Its logic and its existing tests are unchanged.AlertNotebookEndpoint.ReadStatusRowsAsyncruns the two reads and returns at most one row: the re-fire read runs only when the resolution read found nothing, because arm 1 wins. The endpoint passes that row toResolveStatusAsync. For the re-fire read the name is the matched row's stored spelling when there is a match, else the trimmed input. The old forward read (24 hours, 200 rows) is gone from the status path; the read that finds the alert row is unchanged.ResolveStatusAsync, a fleet-level store alert with no resolution row and no later firing returns "No resolution recorded" instead of "Unknown (not collected since ...)". The four-arm doc comment says so. A per-server alert with an unresolved server still reads "Unknown".Two small behavior notes:
Test plan
Darling.Testsbuilds with 0 warnings and 0 errors.AlertNotebookResolutionReadTests(pure) andAlertNotebookResolutionReadLiveTests(live, against a local PostgreSQL 18.6 with TimescaleDB, in UTC): 29 tests, all pass. The live ones seedconfig_alert_logand check: a store self-alert whose Cleared row lands 30 hours later reads "Resolved at T"; so does one whose Cleared row was dismissed; with 250 newer rows from other servers inside the first 24 hours the resolution is still found; a server-scoped alert ignores another server's resolution row and reads its own (30 hours later, dismissed) when it has one; a later firing 30 hours on (dismissed) reads "Fired again"; the matched row is not its own re-fire; a lower-case metric in the link still finds the resolution and the re-fire; Cleared rows at or before the anchor do not count. The pure ones coverResolutionRowNames(every alias and every recovery edge, exact spelling, never a firing name), the read's SQL shape (no dismissed filter, no window,LIMIT 1,server_idonly when known), and "No resolution recorded" for a fleet-level store alert.ReadStatusRowsAsynctemporarily put back to the 24-hour, 200-row, dismissed-excluded read and the fleet-level answer removed, 8 of those tests failed (Cleared 30 hours later, dismissed Cleared row, 250 newer rows, own resolution on a server-scoped alert, re-fire 30 hours later, matched row not its own re-fire, rows before the anchor, and the pure fleet-level test). Restored, all pass. The file was restored byte for byte before the commit.origin/dev(AlertNotebook*includingAlertNotebookEndpointTestsandAlertNotebookCollectorFreshnessLiveTests,DocCommentHygieneTests,StoreSqlClockDisciplineTests,LivePostgresCollectionHygieneTests,LiveCleanupConversionRatchetTests,AlertReadFailureSurfaceTests,StartupCommandTimeoutTests,StorageCommandTimeoutTests), together with the 29 new tests: 511 tests, 0 failed.EXPLAIN (ANALYZE, BUFFERS)on 1.5 millionconfig_alert_logrows with the(server_id, metric_name, alert_time)index: the fleet-level read is an index-only scan with 41 index searches (PostgreSQL 18 skip scan), 163 buffers; the server-scoped read is one index search, 6 buffers.Darling.Testssuite once, withoutDARLING_TEST_PG, after the merge oforigin/dev: 17227 total, 0 failed, 0 errors, 1133 skipped (the live classes skip by design in that run), 1 not run. This run covers the three classes that readDarlingAlertReader.csas text (DarlingMcpAlertToolsTests,LongRunningQueryExclusionKnobRungTests,UncorroboratedRouteStoreKnobTests).Installer.Tests, which this change does not touch.For the reader to double-check
DarlingAlertReader.csas text and were not in the targeted batch above:DarlingMcpAlertToolsTests,LongRunningQueryExclusionKnobRungTestsandUncorroboratedRouteStoreKnobTests. Their first pass over this change is the full suite, so check that result (or CI) for them.server_id, so the index(server_id, metric_name, alert_time)serves it through PostgreSQL 18's skip scan (one search per server id). On an older PostgreSQL it would fall back to another plan. The store's bundled runtime is 18, but a store on an older PostgreSQL is worth a look.AlertNotebookEndpointTests.csis untouched.CHANGELOG
SECTION: Fixed
ENTRY:
REF:
[The alert notebook misses a resolution that lands more than 24 hours after the alert #4755]: The alert notebook misses a resolution that lands more than 24 hours after the alert #4755
[The alert notebook says collection stopped for a fleet-level store alert #4756]: The alert notebook says collection stopped for a fleet-level store alert #4756