Count collection faults per monitor by Areson · Pull Request #187 · block/blip

Areson · 2026-10-21T21:51:30Z

Stack: PR 1 of 5. Based on main. Review only this PR's commits. The PRs above it build on this one.

Why

When a host is short on CPU or a sink is slow, Blip loses or delays metrics without saying so. Domains that miss the engine max runtime (EMR, the per-collection deadline), dropped level collector ticks, and sink retry buffer overflow are logged at debug level or not at all. An operator can't tell that a monitor is collecting partial metrics.

What

Add cumulative per-monitor fault counters, kept in the monitor status.
Add GET /status/monitors/counters to read them as JSON.
Fix a separate data race in clutch.collect (commit 3d85f5f).

How

The status package already had an unused per-monitor counters map. This PR implements it instead of adding self-metrics to the sink path, which the same starved loop would produce.
Components get stable *atomic.Uint64 counters once, when the monitor starts. Increments are lock-free and don't allocate.
Counters: engine-emr-timeouts, engine-domains-not-started, engine-domains-not-returned, collector-faults, collector-panics, collector-timeouts, collector-flush-drops, level-ticks-dropped, level-metrics-dropped, level-transform-dropped, and sink-retry-dropped:<sink>.
The exporter engine counts Prometheus scrapes in the same engine-* and collector-* counters prefixed with exporter-, so scrapes aren't counted as level collector collections.
Sinks register their counters through the optional sink.CounterSink interface when the monitor starts. That's after the loader unloads any earlier monitor with the same ID, so a config reload keeps the counters, and a failed load or stop-loss leaves none behind.
Counters persist across monitor stop and start, and are removed with the monitor status on unload.
The existing, unused engine-emr-timeout event is now sent, as info rather than an error.
status.ReportCounters(ids...) lets a host that embeds Blip poll the counters and forward them.
The race fix is its own commit: clutch.collect read cl.ctx and cl.m.Level after it released the lock. It now uses the values captured under the lock.
On this PR alone, a monitor whose runLoop restarts it after Stop can recreate its counters. That restart is fixed up-stack in Stop removed monitors in parallel outside the loader mutex #190, where runLoop checks stopChan before every restart.

Risk

Low. Collection behavior is unchanged. The new code adds counters and one read-only API route.

Testing

A local benchmark ran 400 monitors in one process against 4 MySQL 8.0 servers. It read GET /status/monitors/counters the whole time.

On 1 CPU, the counters climbed steadily: 231 EMR timeouts and 1,304 domains not returned in 6 minutes.
With CPU to spare, they stayed at 0.

Unit tests cover each counter, including a collector panic, a CMR timeout, the exporter prefix, TransformMetrics drops, a panic after a collector fault (run with -race), and counters across a config reload and a stop-loss.

Bigger picture

This is the first of 5 fixes found by that benchmark. The counters make the faults the other PRs fix visible.

🤖 Generated with Claude Code

clutch.collect read cl.ctx and cl.m.Level after releasing the clutch lock to call the collector. If the collector doesn't stop at its CMR, the next interval's collect detects the fault and overwrites cl.ctx and cl.m under the lock while the first goroutine may still be reading them. Use the context and level captured under the lock instead, like the existing interval and cancel locals. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Collection faults were silent: domains that didn't start or return before the engine max runtime (EMR), dropped level collector ticks, and sink retry buffer overflow were logged only at debug level or not at all. When a host is CPU-starved or a sink is slow, nothing showed that a monitor was collecting partial metrics or dropping them. Add cumulative per-monitor counters: engine-emr-timeouts collections that hit the EMR engine-domains-not-started domains not started before the EMR engine-domains-not-returned domains started but not returned by the EMR collector-faults domains still running at their next interval (event collector-fault) collector-flush-drops domain metrics dropped because the engine collection channel was full (event drop-metrics-flush) level-ticks-dropped level collector ticks dropped while a collection blocked the loop level-metrics-dropped intervals dropped because sinks weren't keeping up (event lco-metrics-fault) sink-retry-dropped:<sink> metrics pushed off a full retry buffer Design: - Exposure: monitor status. Blip doesn't emit metrics about itself; it reports internal state per monitor through the status package and GET /status/monitors. The status package already had an unused per-monitor counters map, so this implements it rather than adding self-metrics to the sink path. Self-metrics would also be produced by the same starved collection loop they're meant to diagnose. - Cost: components get stable *atomic.Uint64 counters from status.Counter once, when they're created. Increments are lock-free and don't allocate; the collect path only adds a bool and two ints. - Reading: status.ReportCounters(ids...) returns map[monitorId]map[counter]uint64. New endpoint GET /status/monitors/counters returns the same as JSON. A host that embeds Blip can poll ReportCounters and forward each counter as a monotonic count tagged with the monitor ID, without scraping logs. - Lifetime: counters only increase and persist across monitor stop and start, because sinks and status outlive a monitor restart. They are removed with the rest of the monitor status on unload. Counters are created at zero, so they're reported before they first increment. - Events: collector-fault, drop-metrics-flush, and lco-metrics-fault each mark lost or late metrics, so each has a counter. drop-metrics-fence isn't counted: it's the later drop of the metrics already counted by collector-faults, and it fires only if the stuck collector ever returns. The existing, unused engine-emr-timeout event is now sent (not as an error, because a CPU-starved host can hit the EMR every interval) with the not-started and not-returned domain counts. - Dropped ticks are detected from the gap between consecutive tick times, rounded to whole ticks; the level collector doesn't catch up on dropped ticks, so each one delays every level by one tick. Collection behavior is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

zyin-squareup

🤖 This review was generated by an AI agent on behalf of @zyin-squareup.

Summary. The counter design (stable *atomic.Uint64 per component, read through ReportCounters) is solid. The clutch.collect race fix is correct: m is the same value assigned to cl.m under the lock. There is one blocking problem: the sink-retry-dropped:<sink> counter stops being reported after a monitor's config changes and is reloaded.

BLOCKING

1. sink-retry-dropped:<sink> stops being reported after a config-change reload (sink/retry.go:107, monitor/loader.go:259-264, 389-413, 647)

The sink counter is registered when the sink is built. The loader builds the new sinks before it removes the old monitor's status, so the removal deletes the new counter too.

Call sequence for POST /monitors/reload (or Loader.Load) when an existing monitor's config changed:

ml.load() → makeMonitor(cfg) → sink.Make → NewRetry → status.Counter(id, "sink-retry-dropped:<sink>"). This writes the new counter pointer into s.counters[id], which already exists for the old monitor.
Load then calls ml.Unload(id) for diff.changed → status.RemoveMonitor(id) → delete(s.counters, id). The new monitor's Retry still holds that pointer, but it is no longer in the map.
StartMonitors → startup → newLevelCollectorWithDBProvider / newEngineWithDBProvider create a fresh s.counters[id] map with only the engine and level-collector counters.

Result: for that monitor, sink-retry-dropped:* is missing from GET /status/monitors/counters and status.ReportCounters until Blip restarts. The retry buffer keeps dropping metrics, but nothing reports it. That is the silent loss this PR is meant to expose. The engine and LCO counters are fine only because they're created at Start, after the unload. The Counter doc comment ("valid until RemoveMonitor is called") describes this hazard, but the loader's ordering violates it.

Related, same root cause: load() says it "has no side effects on error", but makeMonitor now registers counters. If load() fails partway, or Load returns ErrStopLoss, monitors that never get loaded still appear in /status/monitors/counters with sink-retry-dropped:*: 0.

Suggested fix (either):

Register the sink counter lazily, the first time it's needed (in push or the first Send). Or have the monitor register it at Start, like the engine and LCO counters.
Or split removal, so that unloading a changed monitor removes its status but not its counters, while unloading a removed monitor removes both. This also fits "counters persist across stop/start": a changed config for the same monitor ID would keep counting.

Please add a loader-level test: load a monitor with a Retry-wrapped sink (e.g. chronosphere with no options), change its config, call Load again, and assert the sink counter is still in ReportCounters(id) and still increments.

NON-BLOCKING

monitor/engine.go:609: the panic recovery in clutch.collect still reads cl.m.MonitorId without the lock. It's the same pattern the race-fix commit removes, though MonitorId never changes in practice. cl.event.MonitorId or the local m.MonitorId would avoid it.
monitor/engine.go:337-342: break exits only the select, so after the EMR expires the start loop keeps going, and Go's random choice between ready select cases can still start collectors. This is not new, but it means engine-domains-not-started can undercount, and an interval can count as engine-emr-timeouts with 0 not started and 0 not returned. Worth a comment, or a labeled break if "skip pending" is intended.

Validation

go test -race -count=1 ./status/ ./sink/: pass.
go test -race -count=3 ./monitor/ -run 'TestEngineCounts|TestClutchCountsFlushDrops|TestDroppedTicks|TestLevelCollectorCounts': pass.
go vet ./status/ ./sink/ ./monitor/ ./server/: clean.
The broader ./monitor/ run failed only in TestLevelCollector_RGB_Fault, which needs a local MySQL (dial tcp 127.0.0.1:33800: connection refused). That's environmental, not caused by this PR.
I couldn't run a scratch repro for finding 1 in this environment. It is established by the call trace above.

Retry registered its sink-retry-dropped counter when the sink was made. When a monitor's config changes, the loader makes the new monitor and its sinks first, then unloads the old monitor, which removes the status and counters for that monitor ID. The new Retry kept incrementing a counter that was no longer reported, so retry buffer drops were silent again until Blip restarted. A load that failed or hit the stop-loss also left zero counters for monitors that were made but never loaded. Add sink.CounterSink, an optional interface with RegisterCounters. Retry implements it, and Delta forwards it to the sink it wraps. The monitor calls it on its sinks at startup, where the engine and level collector get their counters, which is always after the loader has unloaded any earlier monitor with the same ID. Retry doesn't count drops before it's registered. Also, in clutch.collect panic recovery, use the local m.MonitorId rather than reading cl.m without the lock, and document that the EMR break in Engine.Collect exits only the select, so domains can still start after the EMR and engine-domains-not-started counts domains that actually didn't start. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Areson · 2026-10-22T14:56:46Z

🤖 Thanks, all three are addressed in ef1fd3d.

Blocking: the sink counter is now registered when the monitor starts. There's a new optional sink.CounterSink interface with RegisterCounters(). Retry implements it and Delta passes it to the sink it wraps. The monitor calls it in startup, next to where the engine and level collector get their counters, so it always runs after the loader's Unload and RemoveMonitor. Retry doesn't count drops until it's registered. Building a monitor no longer touches status, so failed loads and ErrStopLoss leave no counters behind. A config change still unloads the monitor, so its counters reset; the docs now say so.

Tests: TestLoaderReloadKeepsSinkCounters uses a chronosphere sink, changes its config and loads again, then checks the counter is still reported and increments after the new monitor starts. TestLoaderUnloadedMonitorsHaveNoCounters covers monitors that are loaded but not started, and the stop-loss case. When I moved registration back into NewRetry, both failed the way you described. #190 adds a test for a new monitor whose start waits for the old one to stop.

engine.go:609: now uses m.MonitorId. engine.go:337-342: behaviour unchanged; I added a comment that break exits only the select, and that notStarted counts the domains that actually didn't start.

zyin-squareup

🤖 This review was generated by an AI agent on behalf of @zyin-squareup.

Summary. Re-review of ef1fd3d. It fixes the earlier blocking issue: sink-retry-dropped:<sink> was lost after a config-change reload. Both earlier non-blocking notes are also addressed. I found no new blocking issues. Approving.

Earlier findings

#	Finding	Status	Evidence
1 (blocking)	Sink counter deleted by `RemoveMonitor` on config-change reload	Fixed	`Retry` no longer registers in `NewRetry`. `Monitor.startup` calls `registerSinkCounters(m.sinks)` (`monitor/monitor.go:413`), and that runs only from `StartMonitors`. `Loader.Load` unloads changed monitors first (`loader.go:262-264` → `Unload` → `status.RemoveMonitor`), so registration always lands in the new map. `Delta` forwards `RegisterCounters`, which covers both shapes `sink/factory.go:178-180` returns. `push` reads `rb.dropped` under `stackMux`, the same lock `RegisterCounters` writes it under, so there's no race. Stop/Start without unload re-registers into the existing map, so counts persist as the docs say.
1b	`load()` side effects: counters left for monitors made but not loaded (error/stop-loss)	Fixed	`makeMonitor` no longer touches status. `TestLoaderUnloadedMonitorsHaveNoCounters` needs no MySQL, so it runs in CI. It would fail if registration moved back into `NewRetry`.
2	Panic recovery read `cl.m.MonitorId` without the lock	Fixed	Uses the local `m.MonitorId` (`engine.go:~611`).
3	EMR `break` exits only the `select`	Addressed	Comment added. `notStarted := len(domains) - len(running)` now counts domains that actually didn't start, so the undercount I described is gone.

NON-BLOCKING

The reload regression test is skipped in CI. TestLoaderReloadKeepsSinkCounters (monitor/loader_counters_test.go:649-655) skips when MySQL isn't running and test.Build (GITHUB_ACTION) is set. .github/workflows/ci.yml runs only go test ./... with no MySQL, so CI never runs it. The CI-run stop-loss test catches a move back to registering in the constructor. Nothing in CI catches a dropped registerSinkCounters call in startup. Nothing on the path to registerSinkCounters appears to connect to MySQL: makeDB doesn't connect, the plan comes from the default loader, and HA is off. If that holds, the MySQL gate could be dropped. I haven't verified that by running it.
The PR description is slightly stale after the fix. It says "Components get stable *atomic.Uint64 counters once, at creation." Retry now gets its counter at monitor start through the new exported sink.CounterSink interface, which the description doesn't mention. The docs and code comments are accurate.

Validation

I couldn't run go vet or go test in this session: Go commands need interactive approval. The findings above come from reading the code at ef1fd3d. The CI build job was still pending when I checked.
Coverage: correctness, concurrency (stackMux, clutch locals), loader lifecycle (load/unload/start/stop-loss), contracts (CounterSink, API, docs), test effectiveness. Security and performance aren't materially affected.

A collector panic was counted only as an EMR timeout and a domain not returned, so it looked like a CPU-starved host. Count it in collector-panics. Also count collectors that return context.DeadlineExceeded at their CMR in collector-timeouts. Flushing on panic is unchanged: the engine still waits for the domain until the EMR, so a panic is also an EMR timeout, as documented. The exporter engine registered the same counters as the level collector engine, so Prometheus scrapes were counted as LCO collections. The exporter engine now uses the same counters prefixed "exporter-", like exporter-engine-emr-timeouts. Count collections dropped because TransformMetrics returned an error in level-transform-dropped. Metrics lost to a sink or TransformMetrics panic are still not counted, as documented. Reword level-metrics-dropped: it counts collections, which can include late metrics from more than one interval. clutch.collect panic recovery read cl.bg without the lock. After a collector fault, the next interval's goroutine sets cl.bg while the faulted goroutine can still panic. Read cl.bg into a local while the lock is held at the start, and set the local where this goroutine sets cl.bg, so the recovery keeps its foreground or background behavior. Document that a sink wrapping another sink must forward RegisterCounters, and tighten the dropped ticks test to expect exactly 2 with half a tick of margin. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

zyin-squareup

🤖 This review was generated by an AI agent on behalf of @zyin-squareup.

Summary. This is a re-review of 54b2026, which adds collector-panics, collector-timeouts, level-transform-dropped and the exporter- counter prefix, and fixes the cl.bg read in panic recovery. I found no blocking issues. Approving.

Earlier findings

No.	Finding	Status	Evidence
1 (blocking)	Sink counter deleted on config-change reload	Fixed (unchanged since `ef1fd3d`)	`registerSinkCounters` still runs from `startup`. `TestLoaderUnloadedMonitorsHaveNoCounters` passes with `-race`.
2	Reload regression test is skipped in CI	Still open (non-blocking)	`TestLoaderReloadKeepsSinkCounters` still requires MySQL (`loader_counters_test.go:69-75`). Locally it fails with `dial tcp 127.0.0.1:33800: connection refused`; in CI it is skipped.
3	PR description stale about counter registration	Fixed	The description now says counters are created "when the monitor starts" and explains `sink.CounterSink`.

New code in 54b2026

bg local in clutch.collect (engine.go:618, 679): correct. cl.bg is never set back to false anywhere in monitor/, so reading it under the lock at the start and setting the local where this goroutine sets cl.bg keeps the old semaphore-release behavior and removes the unlocked read. TestClutchPanicAfterCollectorFault passes 3/3 with -race.
countTimeout: it is called after the fence check and under the lock in both the foreground and background paths, so a run is counted once and a fenced run is not counted.
Exporter prefix: the exporter engine now registers its own exporter-* counters, and its clutches get e.counters (engine.go:227), so scrapes no longer increment the LCO counters.

NON-BLOCKING

collector-timeouts misses CMR timeouts from trx and stmt.current (inline on engine.go:701).
The exporter prefix test calls newExporterEngine directly (inline on monitor.go:368).
Docs overstate the panic/EMR link (docs/content/api/status.md:60, status/counters.go COLLECTOR_PANICS). They say every panic is also counted in engine-emr-timeouts and engine-domains-not-returned. That is true only for a panic in the foreground Collect of the current interval. A panic in the ErrMore background loop, or in a fenced run, happens after the domain was already flushed or after its interval was swept, so the EMR counters do not change. Suggest "a foreground panic is also counted in ...".

Validation

go vet ./status/ ./sink/ ./monitor/ ./server/: clean.
go test -race -count=1 ./status/ ./sink/ ./server/: all pass.
go test -race -count=3 ./monitor/ -run "TestEngineCounts|TestClutch|TestDroppedTicks|TestLevelCollectorCounts|TestExporterEngine|TestLoader": everything passes 3/3 except TestLoaderReloadKeepsSinkCounters, which needs a local MySQL (environmental).
I could not run mutation checks in this session. Pin coverage comes from reading the assertions: the panic, timeout, transform and -race panic-after-fault tests each assert their counter exactly. The exporter wiring in startup is not pinned (note 2).
CI (build, Semgrep, zizmor, DCO) is green on 54b2026.

Coverage: correctness, concurrency (clutch locals, fence), contracts and docs, test effectiveness, observability. Security and performance are not materially affected.

Only a panic in the foreground Collect of the current interval is also counted in engine-emr-timeouts and engine-domains-not-returned. A panic in the ErrMore background loop, or in a run behind the collector fault fence, is counted only in collector-panics. collector-timeouts counts only collectors that return the context error or wrap it with %w. Some built-in collectors wrap query errors with %s, so their CMR timeouts aren't counted yet. On a CPU-starved host, collector-timeouts rises almost one for one with engine-emr-timeouts, so document level-ticks-dropped as the counter to watch for starvation. Also comment that startup must make the exporter engine with newExporterEngine so its counters are prefixed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This was referenced Oct 21, 2026

Apply timeout-connect to the MySQL DSN #188

Open

Stop the level collector without waiting for the next tick #191

Draft

Areson added this pull request to stack #192 October 21, 2026 21:52

Ian Oberst and others added 2 commits October 21, 2026 15:43

Areson force-pushed the Areson/collection-fault-counters branch from 4e3c5bd to fd13992 Compare October 21, 2026 22:43

Areson marked this pull request as ready for review October 21, 2026 22:43

zyin-squareup requested changes Oct 21, 2026

View reviewed changes

zyin-squareup approved these changes Oct 22, 2026

View reviewed changes

zyin-squareup reviewed Oct 22, 2026

View reviewed changes

Comment thread monitor/engine.go

zyin-squareup reviewed Oct 22, 2026

View reviewed changes

Comment thread monitor/monitor.go

zyin-squareup approved these changes Oct 22, 2026

View reviewed changes

Areson mentioned this pull request Oct 22, 2026

Wrap collector query errors with %w so CMR timeouts are counted #193

Draft

Navigation Menu

Repository navigation

Count collection faults per monitor - #187

Count collection faults per monitor#187
Areson wants to merge 5 commits into
mainfrom
Areson/collection-fault-counters

Areson commented Oct 21, 2026 •

edited

Loading

Uh oh!

zyin-squareup left a comment

Uh oh!

Areson commented Oct 22, 2026

Uh oh!

zyin-squareup left a comment

Uh oh!

Uh oh!

Uh oh!

zyin-squareup left a comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Areson commented Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What

How

Risk

Testing

Bigger picture

Uh oh!

zyin-squareup left a comment

Choose a reason for hiding this comment

BLOCKING

NON-BLOCKING

Validation

Uh oh!

Areson commented Oct 22, 2026

Uh oh!

zyin-squareup left a comment

Choose a reason for hiding this comment

Earlier findings

NON-BLOCKING

Validation

Uh oh!

Uh oh!

Uh oh!

zyin-squareup left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Areson commented Oct 21, 2026 •

edited

Loading