diff --git a/dash/overview.go b/dash/overview.go index bb5226af..e3a6cc0a 100644 --- a/dash/overview.go +++ b/dash/overview.go @@ -3,6 +3,7 @@ package dash import ( "database/sql" "fmt" + "time" "golang.org/x/sync/errgroup" ) @@ -49,6 +50,16 @@ type WaterfallStep struct { // Overview is the payload behind the dashboard's headline. Every percentage here // is derived at read time from stored absolutes, so a rate change never rewrites // history and a filter change never needs a rebuild. +// invalidConfigRecentWindow is how recently a request must have run with a failed configuration +// for the dashboard to still call it a live problem. +// +// An hour, because the thing being detected is a stored config that cannot build: while it lasts +// EVERY request for that account is affected, so on any account with traffic the signal appears +// within seconds and keeps appearing. An hour is therefore long enough that a quiet account still +// trips it, and short enough that the banner clears on its own once someone fixes the config — +// without anyone having to know that clearing it is a thing that needs doing. +const invalidConfigRecentWindow = time.Hour + type Overview struct { Since int64 `json:"since"` Until int64 `json:"until"` @@ -65,6 +76,21 @@ type Overview struct { // zero before anyone noticed from a log line rather than from here. Nonzero here means the // account's Settings page needs attention now, not "check the logs". InvalidConfigRequests int64 `json:"invalid_config_requests"` + // InvalidConfigRecent is the same count over the last invalidConfigRecentWindow, scoped to + // the account but NOT to the filter's time range or any of its facets. It exists because + // InvalidConfigRequests alone cannot answer the question the banner above the headline asks, + // which is "is compaction broken RIGHT NOW". + // + // The dashboard's default view has no time filter at all, so a window-scoped count keeps + // reporting an incident forever after it is over: this deployment carried 1,752 such requests + // from a single afternoon (a config key removed with no migration, since fixed), and the page + // went on telling every viewer to "open Settings and fix the configuration" for days, about a + // configuration that was already correct. An alarm that cannot switch itself off is one people + // learn to scroll past, which costs exactly what it was built to buy. + // + // Facets are deliberately ignored, not just the time range: filtering to one model must not be + // able to hide a live breakage that happens to be showing up on another. + InvalidConfigRecent int64 `json:"invalid_config_recent"` TokensBefore int64 `json:"tokens_before"` TokensAfter int64 `json:"tokens_after"` @@ -680,6 +706,7 @@ func (d *DB) Overview(f Filter) (*Overview, error) { accountingM, cacheMissM, uncompressedM map[string]int64 p95cg, p95up float64 invalidConfigRequests int64 + invalidConfigRecent int64 ) var g errgroup.Group // See InvalidConfigRequests' own comment: preset is set to "invalid" by @@ -691,6 +718,20 @@ func (d *DB) Overview(f Filter) (*Overview, error) { return d.sql.QueryRowContext(d.readCtx(), `SELECT COUNT(*) FROM requests r WHERE `+cond+` AND r.preset = 'invalid'`, args...).Scan(&invalidConfigRequests) }) + // The same fact over a fixed recent window, which is what the banner is actually about. Built + // here rather than by copying f and overriding Since, so that it demonstrably carries the + // TENANT scope and nothing else: a facet leaking in could hide a live breakage, and dropping + // the tenant term would show one account another's problem. + g.Go(func() error { + rcond := "r.keepalive = 0 AND r.ts >= ? AND r.preset = 'invalid'" + rargs := []any{time.Now().Add(-invalidConfigRecentWindow).UnixMilli()} + if !f.TenantAll { + rcond += " AND r.tenant_id = ?" + rargs = append(rargs, f.Tenant) + } + return d.sql.QueryRowContext(d.readCtx(), + `SELECT COUNT(*) FROM requests r WHERE `+rcond, rargs...).Scan(&invalidConfigRecent) + }) // The replay ceiling's raw form, corrected below by the inflation query once both are in — // see the comment above `splitMoved` usage earlier in this function for the ceiling's own // derivation, and see the correlated-vs-window-function tradeoff explained where this query @@ -906,6 +947,7 @@ func (d *DB) Overview(f Filter) (*Overview, error) { o.Uncompressed = uncompressedM o.CGLatencyMsP95, o.UpstreamMsP95 = p95cg, p95up o.InvalidConfigRequests = invalidConfigRequests + o.InvalidConfigRecent = invalidConfigRecent o.SafetyCost.FrozenTokens = o.FrozenTokens o.SafetyCost.RestoredTokens = o.ExpandTokens diff --git a/dash/store_test.go b/dash/store_test.go index 16d8d7f7..cb55d658 100644 --- a/dash/store_test.go +++ b/dash/store_test.go @@ -489,6 +489,52 @@ func TestOverviewCountsInvalidConfigRequests(t *testing.T) { } } +// A resolved config incident must stop raising the alarm, which is what InvalidConfigRecent is for. +// +// The dashboard's banner is present-tense — "open Settings and fix the configuration" — but it was +// driven by a count over the filter's window, and the default view has NO window. So on this +// deployment 1,752 invalid-config requests from a single afternoon kept the banner up for days +// after the config was fixed, telling every viewer to go and repair something already correct. An +// alarm that cannot switch itself off is one people learn to ignore. +func TestInvalidConfigRecentIgnoresResolvedHistory(t *testing.T) { + db := openTestDB(t) + now := time.Now() + // A burst of breakage well in the past, and nothing wrong since. + old := mkEvent(now.Add(-48*time.Hour).UnixMilli(), "s-old", "m", 100, 100) + old.Preset = "invalid" + fine := mkEvent(now.Add(-time.Minute).UnixMilli(), "s-now", "m", 100, 90) + fine.Preset = "codesmart" + if err := db.insertBatch([]*Event{old, fine}); err != nil { + t.Fatal(err) + } + o, err := db.Overview(Filter{}) + if err != nil { + t.Fatal(err) + } + if o.InvalidConfigRequests != 1 { + t.Fatalf("invalid_config_requests = %d, want 1 (the historical fact must still be reported)", + o.InvalidConfigRequests) + } + if o.InvalidConfigRecent != 0 { + t.Errorf("invalid_config_recent = %d, want 0: a config fixed two days ago is not a live "+ + "problem, and the banner keyed on this must clear itself", o.InvalidConfigRecent) + } + + // ...and a breakage happening NOW must still raise it, or the fix has removed the alarm. + live := mkEvent(now.Add(-2*time.Minute).UnixMilli(), "s-live", "m", 100, 100) + live.Preset = "invalid" + if err := db.insertBatch([]*Event{live}); err != nil { + t.Fatal(err) + } + if o, err = db.Overview(Filter{}); err != nil { + t.Fatal(err) + } + if o.InvalidConfigRecent != 1 { + t.Errorf("invalid_config_recent = %d, want 1: a configuration failing to build right now is "+ + "exactly what this banner exists to surface", o.InvalidConfigRecent) + } +} + // TestNewInputRatioNeverDividesSavingsByThemselves is the guard the issue calls // non-negotiable: with no provider usage data the denominator would be `saved` // alone and the ratio would read ~100%. It must read n/a. diff --git a/dash/ui/app.js b/dash/ui/app.js index 2fc2e2b3..2ec10a8e 100644 --- a/dash/ui/app.js +++ b/dash/ui/app.js @@ -1477,10 +1477,16 @@ function renderTiles(o) { // proxy's own log. Shown above the headline, not folded into Diagnostics: the number it // reports is "your bill was bigger than it needed to be, right now", which is exactly what // this page exists to prevent from going unnoticed. - if (o.invalid_config_requests > 0) { + // Gated on the RECENT count, not the window's total, and that distinction is the whole point. + // The default view has no time filter, so a total keeps reporting an incident forever after it + // is fixed: this deployment carried 1,752 such requests from one afternoon and then told every + // viewer for days to go and fix a configuration that was already correct. The banner asks a + // present-tense question, so it reads a present-tense number and clears itself once someone + // fixes the config — see InvalidConfigRecent in dash/overview.go. + if (o.invalid_config_recent > 0) { host.appendChild(el('div', { class: 'banner bad', 'data-testid': 'invalid-config' }, - el('div', {}, el('strong', {}, num(o.invalid_config_requests) + ' request' - + (o.invalid_config_requests === 1 ? '' : 's') + ' ran with NO compaction. '), + el('div', {}, el('strong', {}, num(o.invalid_config_recent) + ' request' + + (o.invalid_config_recent === 1 ? '' : 's') + ' ran with NO compaction in the last hour. '), 'This account’s stored configuration failed to build, so every one of them was ' + 'forwarded as-is instead — traffic kept working, but none of it was compacted. ' + 'Open Settings, fix the configuration, and save it again.')));