perf(relay): bound fleet usage metrics collection - #7176
TheSentinel454 wants to merge 3 commits into
Conversation
Signed-off-by: tornquist <tornquist@squareup.com>
🔐 Codex Security Review
|
Signed-off-by: Luke Tornquist <tornquist@squareup.com>
## Summary Make the relay read completed storage-accounting snapshots from PostgreSQL by default. An environment with no snapshot emits no storage metrics. When a worker publishes its first result, the relay picks it up on the next successful leader usage tick, without a restart or configuration change. This changes how storage usage is measured and reported. **S3 remains the object store. PostgreSQL stores the completed counts, byte totals, community breakdown, and calculation metadata—not the objects themselves.** ### Why move relay storage accounting entirely to database reads? Today the relay supports two sources: its own S3 scan (`inline`, the default) and the worker's saved snapshot (`external`). That requires operators to deploy a worker and then coordinate a separate relay-mode change in each environment. The worker already owns the S3 listing and calculation. It runs in a separate process with its own object cap, memory limit, and deadline. Keeping a second scan inside the serving relay retains the resource pressure and failure modes that this isolation is meant to remove. PostgreSQL provides a durable handoff. The worker replaces one completed snapshot atomically after a successful calculation. The relay reads that row and exposes its values through the existing metrics endpoint. A relay restart or leadership change can load the last completed result without scanning the bucket again. There is deliberately no S3 fallback when a snapshot is absent or a read fails. Such a fallback would put the expensive scan back in the relay precisely when a worker is absent or unhealthy. It would also leave two calculation paths with different resource limits and freshness behavior. ### What happens if no worker exists? **A worker that has never published is a normal, successful inactive state.** The snapshot table must exist through the normal database schema, but it may contain zero rows. The reader returns success, emits no storage totals or failed-load gauge, and checks again on the next usage tick. It does not exit the relay, start a worker, or list S3. The relay checks for a completed database row; it does not inspect Kubernetes Jobs or CronJobs. Worker presence and snapshot presence are therefore different: | Situation | Storage-reader behavior | |---|---| | No worker and no snapshot row | Return success, emit no storage metrics, and query again next tick. | | Worker starts later and publishes its first row | Read and emit it on the next successful leader usage tick; no restart or mode change. | | Worker is removed or fails, but its last valid row remains | Continue reporting the last completed totals with their increasing age. | | Query succeeds but a previously present row has been removed | Clear the cache and stop refreshing storage series; existing exporter series expire through the idle timeout. | | Database read fails, times out, or returns an invalid snapshot | Log a warning, set `buzz_storage_snapshot_load_ok=0`, retain any last-good cached totals, and retry next tick. | | Snapshot table is missing | Treat this as a schema error, not normal worker absence; follow the failed-read behavior above. | | `BUZZ_STORAGE_METRICS=off` | Skip the snapshot query and all storage metric emission. | No row means “not measured,” not zero bytes. A valid completed snapshot of an empty bucket can report zero. If a read fails before any good snapshot has been cached, only failed-load health is emitted. These reads happen in the existing background metrics task, not the relay startup path. A storage-read failure does not request process exit or change readiness directly. A wider database outage can still affect the relay's existing database-dependent operations and readiness. ### Read, parse, and emit path ```text Worker: S3 listing -> BucketSnapshot -> JSON -> PostgreSQL Relay: PostgreSQL -> JSON -> BucketSnapshot -> existing metric gauges Collector: relay /metrics endpoint -> Datadog ``` 1. `main.rs::run_storage_sweep_tick` calls the reader from the existing leader-only usage task. The default interval remains 300 seconds. 2. `storage_sweep.rs::run_storage_metrics_tick` coordinates the read, cache update, failure health, and metric emission. 3. `refresh_persisted_snapshot` calls the existing `Db::load_storage_accounting_snapshot()` method. A five-second timeout covers pool acquisition and the query. It uses the relay's existing pool. 4. The existing query in `buzz-db/src/store/storage_accounting.rs` reads `snapshot`, `completed_at`, `duration_ms`, `max_objects`, and `code_sha` from the singleton row. 5. `serde_json::from_value(stored.snapshot)` decodes the JSON into the existing `buzz_media::BucketSnapshot` type. Rust infers that type from `cache_persisted_snapshot`'s argument. The generated `Deserialize` implementation handles all fields, including the UUID-keyed `per_community` map. 6. `emit_cached_storage_metrics` sets the existing physical/logical totals and community byte/object gauges. The existing Prometheus exporter exposes them for Datadog collection; this code does not send snapshot JSON to Datadog. The worker's `BucketSnapshot`/`CommunityStorage` structures, JSON field names, SQL publication method, and database schema are unchanged. The old external-mode JSON conversion moves out of `main.rs` into the reader helper. No new format or hand-written field parser is needed. Duration and object-cap metadata are validated before replacing the cache. The five-second relay read timeout is separate from the worker's 30-second startup acquisition budget introduced in #7770. This PR does not change the worker's pool or startup logic. ### Configuration and code changes | Configuration | Before | After | |---|---|---| | Unset, `inline`, or `on` | Relay-local S3 scan | Read completed database snapshots. `inline` logs one migration warning. | | `external` or `snapshot` | Read completed database snapshots | Continue reading snapshots. | | `off` | Disabled | Disabled. | | Unknown or empty value | Disabled with a configuration error | Same. | Existing `inline` settings intentionally become reader aliases. Merely changing the default would leave older deployments on their explicit `inline` setting and preserve the need for coordinated configuration PRs. Remove relay-local scan scheduling, in-flight scan state, scan configuration, and obsolete scan-attempt tests and health metrics. Preserve the physical/logical total and per-community metric names, leader-only emission, community scope filtering, and cleanup of old community labels. The shared relay-state field remains in place; its comment now describes a cached worker result. The Helm chart defaults to `relayMode: external` while leaving `storageAccounting.enabled: false`. Its rendering test verifies that the reader is configured even when no worker is created. The reader regression tests cover missing rows, later publication, updates, invalid data, timeout, removal, reactivation, and the explicit off switch. `docs/storage-accounting.md` describes this operating model. ### Freshness and rollout impact `buzz_storage_snapshot_age_seconds` uses the worker's original completion timestamp. Re-reading an old row never makes it fresh. `buzz_storage_snapshot_load_ok=1` means the row was read and decoded; it does not prove that the worker's latest attempt succeeded. Monitor snapshot age against the worker's schedule and allowed runtime, and monitor Job failures separately. The old relay scan-attempt health gauges are retired because the relay no longer runs those scans. Deploy this relay version through the usual release process. Once it reaches an environment, workers can be enabled independently. Already-released charts that still set `inline` work with the new binary; future chart releases default explicitly to `external`. An intentional `off` override still requires an operator to enable the reader. **Environments with no completed snapshot lose relay-calculated storage totals after this upgrade.** That is the intended tradeoff for removing scans from the serving process. Environments with a prior snapshot continue showing its last completed totals and age. This PR does not install workers in additional environments or deploy the new relay image. ### Related issue Related to #4601. Follows the worker startup fix in #7770. The separate fleet-usage collector work in #7176 overlaps this code and may need reconciliation when it merges. ### Testing Built and ran the relay locally against isolated PostgreSQL and Redis with the legacy `inline` setting, five-second usage ticks, and a 15-second metric idle timeout. The same process was exercised through these steps: 1. Start with an empty snapshot table: relay becomes ready and exposes no storage series. 2. Publish a snapshot containing 123 bytes: the community gauge becomes 123. 3. Replace it with 456 bytes: the gauge becomes 456. 4. Remove the row: storage series expire without fabricating a zero-byte measurement. 5. Publish 789 bytes: metrics reactivate without restarting the relay; readiness remains healthy. The local S3 endpoint recorded zero requests. The unrelated Git conformance startup probe was disabled for this isolated reader check; the test covers storage accounting, not every other use of S3 in the relay. No new relay image was deployed to staging or production during this test. The broad local integration run also hit an existing `p0_pool_acquisitions_use_typed_operation_pairs_without_other` failure in `buzz-db/tests/observability_source.rs`. The same source-only test fails on unmodified `main` at `0ef7a2222`; this PR changes neither the test nor its failing source. The full local `just ci` run stopped at mobile native-asset setup: the `objective_c` build hook received no SDK path from `xcrun`. Mobile tests did not run. Generated with Codex Signed-off-by: Ravneet Arora <rarora@squareup.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested. Reviewed HEAD aa3e70234f5842f9bfa49b472e2e66fa3011f727 against BASE 42b42447b0dc47c3e1a4d95caab893567dcd1677. Keep the fleet-only default, replica-only collection, cached fixed-row snapshots, and explicit all rollback. Merge criteria: correct warm-refresh availability, make rollback availability/gating coherent, and execute the five new relay/media regression witnesses in CI. Details are inline.
Source-only review; no PR code or tests executed. Existing PostgreSQL CI passed the three new fleet database tests on a merge tree identical to this HEAD. The PR-body live-test claims were not independently verified.
| } | ||
|
|
||
| fn mark_stock_failure(&mut self, now: std::time::Instant) { | ||
| self.next_stock = now + self.retry_interval; |
There was a problem hiding this comment.
[P2] Mark a failed refresh unavailable even when a cached snapshot exists
After one successful stock collection, let the next due refresh return Ok(None) (reader missing/stale/unavailable) or Err. This method only advances the retry deadline; stock_snapshot remains Some, so stock_available() remains true and emit_fleet_db_usage_metrics re-emits the old values with buzz_usage_snapshot_available{family="stock"}=1 on every tick. The activity family has the same path. Repeated failures therefore advertise arbitrarily stale data as available.
That contradicts .env.example:30–34 and the availability gate prescribed in docs/usage-metrics.md:19–33,44–45; snapshot age exists, but those instructions do not require an age gate. Preserve last-known values if desired, but track latest-refresh availability separately: clear it on a due skipped/failed refresh and restore it only after success. Add a success → failed/omitted refresh → successful recovery regression for both families, including the emitted availability gauges.
| } | ||
|
|
||
| #[test] | ||
| fn usage_metrics_default_to_fleet_only() { |
There was a problem hiding this comment.
[P2] Wire the new relay/media regression tests into an executed CI lane
Five of the eight new tests are excluded by the configured Rust CI selections. just test-unit (Justfile:357–433) enumerates packages without buzz-media and runs buzz-relay --lib only under test(/^api::admin::/): the two tests here are binary-target tests, while the new config::tests and storage_sweep::tests are filtered out. Backend Integration selects workflow_sink/e2e_event_reminder; the PostgreSQL lane selects ignored PostgreSQL tests, not these five nonignored witnesses. Existing run 33551030165 confirms those invocations on a merge tree identical to this HEAD; the three new database tests do execute.
Consequently, reverting the fleet-only default, breaking cadence/cache reset, or making totals-only storage return incorrect totals/false-zero attribution can pass the configured gates despite these tests. Add narrow infra-free selections for the five existing witnesses (including --bin buzz-relay for these two) to the unit recipe, preserving existing exclusions. Verify their test names execute; compiling them is not a regression gate. This is not a request to broaden the entire relay suite or add a new harness.
| 1. Deploy with per-community mode unset or `off` and the telemetry replica | ||
| freshness budget at its 30000ms default. | ||
| 2. Update dashboards to use fleet gauges and gate alerts on the availability | ||
| gauges above. |
There was a problem hiding this comment.
[P2] Make availability gating coherent with the documented rollback
Following step 2, a dashboard gates fleet totals on buzz_usage_snapshot_available == 1. Restarting with the documented BUZZ_USAGE_METRICS_PER_COMMUNITY=all rollback then suppresses those totals in the dashboard even when collection succeeds: run_usage_metrics_tick calls only emit_db_usage_metrics in that mode (main.rs:1782–1791), whereas the only assignments of availability 1 are in emit_fleet_db_usage_metrics (:1886–1894). Thus the gates are absent after startup; after demotion they can also be 0, with no success path restoring them.
Either publish coherent availability for successful/failed legacy collection as well, or explicitly document that these gates apply only to off and give the corresponding rollback dashboard procedure. The rollback must not silently invalidate the monitoring instructions immediately above it. If fixing the emitter, add a successful all collection/demotion/recovery witness.
wpfleger96
left a comment
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Review at aa3e70234f5842f9bfa49b472e2e66fa3011f727 (base 42b42447b). Thanks for this. The fixed-row fleet SQL and the replica-only routing look right to me. route_usage_read never falls back to the writer, it records a skip reason for every early exit, and the two queries get their own SET LOCAL statement_timeout inside the proved transaction. The new PG tests (fleet_snapshots_match_seeded_stock_and_activity_exactly and friends) run and pass in the PostgreSQL lane at this head.
Three things block it for me:
-
IMPORTANT / Correctness: the storage half is superseded by main and needs a rework on rebase, not just a conflict fix. Since this branched, #7543 and #7845 moved S3 listing out of the relay. On main,
storage_sweep.rsonly loads the latestbuzz-adminworker snapshot from PostgreSQL (run_storage_metrics_tick), and the relay never callsfold_bucket_listing. That makesfinish_totals,fold_bucket_listing_totalsand the relay-sideinclude_per_communitybranch dead code. There's also a trap if the rebase keepshost_mapempty inoffmode and passes it to main'semit_cached_storage_metrics. Everyper_communityentry misses the host map and lands inunmapped_bytes, sobuzz_storage_unmapped_community_byteswould report the whole logical total as unmapped, which is exactly the false anomalycommunity_breakdown_availablewas meant to prevent. I think the storage changes (bucket_index.rs, thelib.rsexport,storage_sweep.rs, therun_storage_sweep_tickbranch) should be dropped. Then fleet-only mode on main's reader either skips the unmapped/per-community emission or passes a real host map. The branch currently conflicts inbucket_index.rs, relaymain.rsandstorage_sweep.rs. -
IMPORTANT / Correctness: the availability gauges don't carry the contract the docs give them.
docs/usage-metrics.mdtells dashboards to gate onbuzz_usage_snapshot_available, but two paths break that:- Stale data reads as available.
stock_available()/activity_available()are justsnapshot.is_some(), andmark_*_failureonly moves the retry deadline. After one success, a reader that goes stale or unavailable keeps re-emitting the cached values withavailable=1indefinitely. That's the same thing Carl flagged inline, and I confirmed it inemit_fleet_db_usage_metrics. Main's storage reader handles this with a separatebuzz_storage_snapshot_load_ok. I think either drop availability to 0 once age passes interval plus grace, or add a last-refresh-ok gauge and document age-based gating. - Rollback hides everything. In
BUZZ_USAGE_METRICS_PER_COMMUNITY=all,run_usage_metrics_tickonly runsemit_db_usage_metrics, so the availability gauges are never set to 1, and demotion explicitly sets them to 0. Dashboards gated the documented way would blank the fleet panels during the rollback. The rollback path should set both families available (andcommunity_breakdown_available) when the legacy collection succeeds.
- Stale data reads as available.
-
IMPORTANT / Correctness: five of the new non-ignored tests never run in CI. These are
usage_metrics_default_to_fleet_onlyandfleet_usage_families_have_independent_cadences(relay binary target),usage_metrics_replica_budget_defaults_on_independently_of_serving_reads(config.rs),totals_only_snapshot_marks_breakdown_unavailable_without_false_unmapped_zero(storage_sweep.rs) andtotals_only_omits_community_map_without_changing_fleet_values(bucket_index.rs). None of them show up in the Unit Tests, Backend Integration or PostgreSQL job logs at this head. The head'stest-unitrunsbuzz-relay --libscoped toapi::admin::and nobuzz-mediaor relay bin tests. After rebasing, the two storage tests go away with finding 1. Main'stest-unitalready has a relay selector list that grows by module prefix, so the remaining three need explicit selectors there: aconfig::tests::exact match plus a--bin buzz-relayline for the two schedule tests. Please mirror them in therun-tests.sh unitfallback. The cadence test is the only thing pinning the retry/interval logic behind finding 2, so it matters that it actually runs.
Non-blocking:
- MINOR / Correctness: default config without a replica loses every DB fleet total. With
offas the new default and no writer fallback, any deployment withoutREAD_DATABASE_URL, or with a failed floor-guard verification, goes from emittingbuzz_total_users/channels/active_users/etc. to emitting nothing, foreveravailable=0. That covers local compose and single-node self-hosts..env.exampledoes say this, so it may well be intended. If so, I'd put it in the rollout section ofdocs/usage-metrics.mdtoo, since it's the most visible behavior change for anyone not running Aurora.
This branch has not been deployed
3 participants
Why
The usage poller currently pays community-proportional memory and query costs even when per-community emission is disabled: every pod loads the full host map and the leader materializes all per-community aggregates before suppressing their labels. That makes telemetry collection itself a scaling constraint.
What changed
allas an explicit rollback mode.No migrations, tables, indexes, or new persistent storage are added.
Risk
Medium. The default changes which telemetry is emitted:
buzz_communities_estimatedreplaces the exactbuzz_communities_totalin fleet-only mode.buzz_total_messagesandbuzz_total_active_channelsare not refreshed in fleet-only mode.buzz_usage_poller_is_leader == 1.Setting
BUZZ_USAGE_METRICS_PER_COMMUNITY=allrestores the legacy collector for temporary rollback.Verification
cargo fmt,git diff --check,actionlint, PostgreSQL test discovery/wrapper contracts, and focused clippy passed.cargo test -p buzz-db --lib: 115 passed, 258 PostgreSQL tests ignored in the infrastructure-free run.cargo test -p buzz-media: 127 passed, 3 external MinIO tests ignored.cargo test -p buzz-relay: 1,025 passed, 90 infrastructure tests ignored.allrestores per-community series.Generated with Codex
Implemented and verified with Codex.