Add database pressure observability - #6700
Conversation
|
Looks like the CI failure is due to some other stuff outside of this PR. Work happening in this PR: #6618 |
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Combined review at b74e724 (two independent source reviews plus live verification against a release relay at this exact head). No blocking findings.
Source verification: every instrumented advisory-lock site wraps the identical SQL, bind values, lock order, and shared/exclusive mode — observe_advisory_lock only times the same awaited future, and the deletion.rs helper refactor doesn't move any lock relative to row locks. Label cardinality is closed at the source level (four enums plus compile-time LitStr operation names), with PoolTimedOut/55P03 correctly mapped to timeout. All early-return Ok paths in the six timed transactions call mark_success(), and the explicit timer drop before the post-commit mention indexing in the membership publisher keeps that work out of the measured window. Privacy is enforced both by the source-guard test and the runtime tracing-layer assertion (parent: None, exactly operation/outcome/elapsed_ms). The relay's Prometheus _seconds matcher already covers the four new duration families.
Live verification at this head: the new metric families appeared on a real /metrics scrape; under genuine advisory-lock contention a 525 ms wait recorded correctly and the ≥500 ms slow warning fired with only the fixed fields — no host, pubkey, SQL, or error content leaked. Both new ignored PostgreSQL pressure probes passed against a real database (pool saturation timeout, closed-pool error, blocked-waiter contention, 55P03, aborted-transaction error).
Two non-blocking notes:
- The red Unit Tests check is unrelated to this PR: it fails linking
sherpa-onnx-c-apiwhile compilingbuzz-voice, and the identical failure reproduces on main tip alone (intermittent — main alternates green/red on it). Needs a rerun or main-side fix before merge, but nothing here caused it. - MINOR for the ignored-test CI follow-up you announced:
mark_success()is manual, so a future early-returnOkpath added to one of the six timed transactions would silently recorderror. AnOk-driven success wrapper would remove that footgun; also the two new#[ignore]observability tests fall into the same unselected-by-CI bucket as the rest.
b74e724 to
ca61bb1
Compare
Signed-off-by: tornquist <tornquist@squareup.com>
Signed-off-by: tornquist <tornquist@squareup.com>
ca61bb1 to
6363c3a
Compare
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Re-approved at exact head 6363c3aedf3930e11f88d71c96248e03e6fca272 after verifying the rebase.
git range-diff d7cf833e^..b74e724e 6f8ad98b^..6363c3ae reports both reviewed commits as exact patch-equivalent (=); their stable patch IDs also match individually. The current branch is directly based on 9d1e4b257657f382d3111ce748f3da8d063b7671, which is the current PR base and origin/main tip fetched for this check. All current required and optional CI checks are green or intentionally skipped. No new source changes to review.
Signed-off-by: tornquist <tornquist@squareup.com>
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Re-reviewed and approved at exact head d75c4e5733b4bf22292703304a8bad26ea62789c.
The only new commit since the previously approved 6363c3aedf3930e11f88d71c96248e03e6fca272 is Harden transaction observability coverage. I traced all six converted transaction paths: the new TransactionTimer::observe wrapper preserves each SQL operation, lock order, commit/rollback decision, return value, and the intended transaction measurement boundary while deriving the metric outcome from the final Result. The membership path still excludes post-commit mention indexing and now correctly skips that indexing when no event was inserted. The added PostgreSQL CI step selects the two ignored real-database observability probes that were previously uncovered. No new findings.
All current CI checks at this head are green or intentionally skipped.
…-history * origin/main: Add database pressure observability (#6700) revert fixed mention highlight (#6716) highlight search terms in results and messages (#6702) fix(desktop): make lightbox zoom controls interactive (#6710) Support community deletion in versioned media buckets (#6738) Fix TipTap editor mount race (#6779) feat(buzz-agent): gate LLM tool calls on session/request_permission (#5712) Add staging dev relay image workflow (#6709) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…esktop * origin/main: Add database pressure observability (#6700) revert fixed mention highlight (#6716) highlight search terms in results and messages (#6702) fix(desktop): make lightbox zoom controls interactive (#6710) Support community deletion in versioned media buckets (#6738) Fix TipTap editor mount race (#6779) feat(buzz-agent): gate LLM tool calls on session/request_permission (#5712) Add staging dev relay image workflow (#6709) Signed-off-by: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz>
…trigger-foundation * origin/main: Add database pressure observability (#6700) revert fixed mention highlight (#6716) highlight search terms in results and messages (#6702) fix(desktop): make lightbox zoom controls interactive (#6710) Support community deletion in versioned media buckets (#6738) Fix TipTap editor mount race (#6779) feat(buzz-agent): gate LLM tool calls on session/request_permission (#5712) Add staging dev relay image workflow (#6709) Signed-off-by: Logan Johnson <loganj@squareup.com>
Co-authored-by: Matt Kursmark <kursmark@squareup.com> Signed-off-by: Matt Kursmark <kursmark@squareup.com> * origin/main: (21 commits) feat: navigate images across message threads (block#6705) Add database pressure observability (block#6700) revert fixed mention highlight (block#6716) highlight search terms in results and messages (block#6702) fix(desktop): make lightbox zoom controls interactive (block#6710) Support community deletion in versioned media buckets (block#6738) Fix TipTap editor mount race (block#6779) feat(buzz-agent): gate LLM tool calls on session/request_permission (block#5712) Add staging dev relay image workflow (block#6709) Extract community persistence (block#6668) Fix mobile Huddle agent voice turn states (block#6611) Add inline profile camera capture (block#6680) Hide Huddles in mobile agent DMs (block#6676) fix(desktop): polish inline chip states (block#6718) Centralize replaceable event persistence (block#6660) feat(workflows): discover trigger filter values (block#6712) feat(desktop): simplify the message action rail (block#6529) fix(desktop): restore icon-only remote marker (block#6491) fix(ci): prevent poisoned Rust caches (block#6618) docs(security): route reports through private advisories (block#6728) ... Signed-off-by: Matt Kursmark <kursmark@squareup.com>
…trigger-foundation * origin/main: Add database pressure observability (#6700) revert fixed mention highlight (#6716) highlight search terms in results and messages (#6702) fix(desktop): make lightbox zoom controls interactive (#6710) Support community deletion in versioned media buckets (#6738) Fix TipTap editor mount race (#6779) feat(buzz-agent): gate LLM tool calls on session/request_permission (#5712) Add staging dev relay image workflow (#6709) Signed-off-by: Logan Johnson <loganj@squareup.com>
…picker * origin/main: Qualify canonical relay images for staged delivery (#6781) feat(desktop): persist agent addressing across composer messages (#6714) feat: navigate images across message threads (#6705) Add database pressure observability (#6700) revert fixed mention highlight (#6716) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz> # Conflicts: # desktop/src/features/messages/ui/MessageComposer.tsx
…r-contracts * origin/main: Qualify canonical relay images for staged delivery (#6781) feat(desktop): persist agent addressing across composer messages (#6714) feat: navigate images across message threads (#6705) Add database pressure observability (#6700) revert fixed mention highlight (#6716) highlight search terms in results and messages (#6702) fix(desktop): make lightbox zoom controls interactive (#6710) Support community deletion in versioned media buckets (#6738) Fix TipTap editor mount race (#6779) feat(buzz-agent): gate LLM tool calls on session/request_permission (#5712) Add staging dev relay image workflow (#6709) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…cp-sessions * origin/main: (31 commits) fix(desktop): stop pulsing addressed agents on send (#6873) fix(desktop): prioritize sidebar channel status (#6861) feat(desktop): hyperlink selected composer text on link paste (#6684) chore(release): release Buzz Desktop version 0.5.20 (#6839) feat(desktop): add KLIPY GIF search to composers (#5554) fix(desktop): respect automatic mention preference after send (#6837) fix(release): attribute desktop candidates to the operator (#6831) fix(ci): check out source in docker.yml merge job (#6833) chore(release): release Buzz Desktop version 0.5.19 (#6828) Remove public relay signing key fallback (#6729) docs(nest): make commit attribution policy-neutral (#6707) fix(desktop-messages): preserve inline agent mentions with persistent addressing (#6793) Qualify canonical relay images for staged delivery (#6781) feat(desktop): persist agent addressing across composer messages (#6714) feat: navigate images across message threads (#6705) Add database pressure observability (#6700) revert fixed mention highlight (#6716) highlight search terms in results and messages (#6702) fix(desktop): make lightbox zoom controls interactive (#6710) Support community deletion in versioned media buckets (#6738) ... Signed-off-by: Salman Mohammed <smohammed@squareup.com>
## Why Finish the replaceable-event slice of [tracker #2](TheSentinel454#2) and [domain issue #6](TheSentinel454#6) without disturbing the runtime/store boundary established by #6660 and #6668. PR #6700 has merged; this PR now targets current main containing its replacement lock and transaction observability. ## What - Move `Db::replace_addressable_event`, its SQL, lock/transaction instrumentation, and focused addressable/parameterized replacement tests from `lib.rs` to `replaceable.rs` - Preserve the transaction-required parameterized API, replacement ordering, rollback semantics, mention indexing, and exactly one datastore span per public operation - Keep implementation and focused PostgreSQL tests co-located; follow-up review removed the dedicated replaceable ownership source guard as low value ## Stack - Exact base: main at f249710 - Exact head: codex/issue-6-finish-replaceable-store at da01840 - Tracker: TheSentinel454#2 - Domain: TheSentinel454#6 - Test/span acceptance: TheSentinel454#17 and TheSentinel454#19 ## Non-goals - No SQL, schema, retry, timeout, lock ordering, transaction boundary, or client-visible behavior changes - No store traits, domain-handle redesign, `PgExecutor` migration, raw pool accessor, new crate, or directory reorganization - No changes to, retargeting of, or merge action on PR #6700 ## Risk Assessment Low. This is a mechanical ownership move with unchanged signatures and SQL. The primary review risk is losing or nesting instrumentation, covered by exact moved PostgreSQL tests and the cumulative production-diff audit. ## Blox Verification Author workstation: `buzz-tornquist-issue-2-store-stack` (`2046520`), exact head `2de5444e14606ce6912d1bb932bc88bffa379a77`. - `cargo fmt --all --check` — passed - `cargo clippy -p buzz-db -p buzz-relay --all-targets -- -D warnings` — passed - `cargo test -p buzz-db --quiet` — 108 passed, 200 ignored; both integration source guards passed - Native PostgreSQL: `cargo test -p buzz-db replaceable::tests:: -- --ignored --test-threads=1` — 11 passed - `cargo test -p buzz-relay --lib api::mesh_demo::tests::demo_join_forwarded_arm_round_trips_echo -- --exact --test-threads=1` — passed - Full serial relay suite was also exercised: 908 tests passed but the existing load-sensitive mesh demo test returned 504 under suite load, matching PR #6700's documented baseline; it passed in isolation and this diff does not touch that subsystem Independent exact-head Blox review: `buzz-tornquist-pr-6777-review` (`2047422`) at `2de5444e14606ce6912d1bb932bc88bffa379a77`. - Review findings — none (critical, important, or minor) - `cargo fmt --all --check` and the ownership guard — passed - `cargo clippy -p buzz-db -p buzz-relay --all-targets -- -D warnings` — passed - `cargo test -p buzz-db --quiet` — 108 passed, 200 ignored; source guards passed - Native PostgreSQL moved suite — 11 passed - `cargo test -p buzz-relay --lib` — 909 passed, 48 ignored Generated with Codex ## Superseded pre-comment restack verification PR #6700 merged before publication completed. This layer was restacked onto current main through the exact parent named above; the final cumulative tip is 2ddcc8a. Cumulative author gates passed: formatting and diff checks; buzz-db and buzz-relay all-target clippy with -D warnings; DB lib 111 passed / 200 ignored; ownership 22/22; observability 1/1; the full isolated PostgreSQL domain matrix; and relay lib 910 passed / 49 ignored. - Workstation: `buzz-tornquist-pr-6777-final-review` (`2057617`), fresh shallow checkout - Base: `f24971033178926153b49d320bd876d15d9cb2bf` - Head: `ffbeaaf00810aa359ab85818ff4820f92263e45f` - Findings: none Reviewed `base..head` for the replaceable-store extraction. The addressable replacement SQL, stale/duplicate outcomes, advisory-lock ordering, transaction rollback/commit boundaries, mention-index atomicity, and public `Db` signature are retained in `replaceable.rs`. The PR #6700 transaction timer remains around the same logical transaction through `TransactionTimer::observe`, and the public operation has exactly one datastore span. Focused addressable and parameterized tests moved with the implementation; the ownership guard excludes duplicates from `lib.rs`. Verification: format and diff checks passed; `buzz-db --all-targets` clippy passed with `-D warnings`; DB lib tests passed (111 passed, 200 PostgreSQL tests ignored); ownership (1/1) and observability (1/1) guards passed; all 11 replaceable PostgreSQL tests passed on native PostgreSQL 17 with migrations 1-32 successful; relay lib test target compiled successfully. Final worktree was detached at the exact head and clean. Complete evidence archive SHA-256: `e37f92bbde4e2343196085095b3093b1bc78196be58bc86826325e0e1639d7ea`. ## Comment-addressed restack Review follow-up removed the dedicated `replaceable_store_has_single_ownership` source test as requested. No production code changed. - Exact base: `f24971033178926153b49d320bd876d15d9cb2bf` - Exact head: `da018405cc83605362125c2d5e5a3f91492431ac` - Final cumulative tip: `6fa2f104d42c6ba85bdf62e7ccb74ceaf4a84f67` - Focused PR verification: formatting and diff checks passed; strict `buzz-db`/`buzz-relay` Clippy passed; DB lib 111 passed / 200 ignored; all 11 replaceable PostgreSQL tests passed. - Cumulative Blox gate: formatting and diff checks; strict `buzz-db`/`buzz-relay` Clippy; DB lib 111 passed / 200 ignored; ownership 21/21; observability 1/1; every moved PostgreSQL test; relay lib 910 passed / 49 ignored. - Independent re-review at this exact head: no findings; fresh exact-parent/head Blox review passed fmt/diff, strict `buzz-db` Clippy, DB lib 111 passed / 200 ignored, manual one-owner/one-span replacement checks, observability, 11 replaceable PostgreSQL tests, and relay compilation. --------- Signed-off-by: tornquist <tornquist@squareup.com>
Why
Database pressure currently collapses several distinct delays into one symptom. This adds the evidence layer needed to distinguish pool acquisition wait, logical database operation time, advisory-lock wait, and selected transaction duration before changing timeout or retry policy.
This is the phase 2 Lane A observability bundle for #26, #28, and #33. It is stacked on #6668.
What
buzz_db_pool_acquire_wait_secondsandbuzz_db_pool_acquisitions_total.#[datastore_span(name = "...")]seam withbuzz_db_operation_duration_seconds, so operation labels remain static source literals instead of request data.buzz_db_transaction_duration_seconds, starting afterBEGINsucceeds and ending after explicit commit/rollback or scope exit.operation,outcome, andelapsed_ms.Fixed labels are deliberately small:
pool_role:writer,readerlock_type:replacement,membership,push_gate,deletion,migration_schema_safetyoutcome:success,error,timeoutwhere SQLx/PostgreSQL can distinguish it accuratelyoperation: compile-time datastore names plus the six closed transaction operation names documented in the runbookNo metric or slow warning contains community IDs, event IDs, event kinds, coordinates, d-tags, SQL/query text, query IDs, returned errors, or event content.
Coverage boundaries
&PgPoolremains folded into operation duration.BEGIN, asynchronous rollback cleanup after an early return, and caller-ownedDb::begin_transactionlifetime.Relationship to #6229
#6229 is the incident-driven timeout precursor. This PR does not add or change
statement_timeout,lock_timeout,idle_in_transaction_session_timeout, retries, audit durability, or client-visible conflicts. It provides the missing distributions needed to evaluate those policies later and intentionally leaves #6229's open audit retry/durability finding untouched.The branches overlap in
crates/buzz-db/src/lib.rsandcrates/buzz-db/src/migration.rs, so a later rebase may need textual conflict resolution, but the behavior is complementary rather than duplicated.Risk assessment
Moderate-low. The primary risk is instrumentation overhead and added static series. Cardinality is source-bounded, slow logs are sampled/redacted root events, and the lock/transaction changes wrap existing awaits without changing policy or ordering.
Verification
Author workstation:
buzz-tornquist-db-pressure-observability(2010927), exact headd7cf833e26c528adfcde3917ded80daf6f4ddac9, parent6f50e6b2b2a996349149af61d35bdd6a355f77fd.cargo fmt --all --check— passedcargo clippy -p buzz-datastore-tracing -p buzz-db -p buzz-audit -p buzz-search -p buzz-relay --all-targets -- -D warnings— passedcargo test -p buzz-datastore-tracing --quiet— 4 passedcargo test -p buzz-db --quiet— 109 passed, 200 ignoredcargo test -p buzz-audit -p buzz-search --quiet— 16 passed, 25 ignoredcargo test -p buzz-relay --lib --quiet -- --test-threads=1— 906 passed, 48 ignoredThe default-parallel relay run passed once; subsequent runs exposed the existing load-sensitive
api::mesh_demo::tests::demo_join_forwarded_arm_round_trips_echo504 at the end of the suite. That test passes in isolation and the full relay suite passes serially.Independent exact-head review workstation:
buzz-tornquist-db-pressure-observability-review(2013067). Formatting, the same all-target clippy command, datastore instrumentation tests, DB unit tests, source privacy guards, and diff/non-goal audits passed; no review findings.Generated with Codex