feat(db): expose connection setup metrics - #7286
Conversation
🔐 Codex Security Review
|
f58e948 to
2c3e30d
Compare
Record operation checkout starts and bounded writer connection setup steps in metrics and lifecycle logs. Co-authored-by: Ravneet Arora <rarora@squareup.com> Signed-off-by: Ravneet Arora <rarora@squareup.com>
2c3e30d to
32d6e43
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
No blocking findings at head 32d6e43176f7341104f39773e5e0e107200737f2 against base 44316ff72f5f7de014c66b01cbf534298a70c249. This is a review comment, not approval.
The composed production after_connect hook retains the safety SQL, bindings, and error propagation. Initial construction and later successful physical connections use the same instrumentation; setup failures stop before ready. Checkout starts preserve the existing terminal/waiter accounting. The exported ceilings reconcile: 198 operation-aware checkout series and 71 connection-step series, without sensitive or connection-ordinal labels.
Two non-blocking followups:
- Clarify which pools the evidence covers.
main.rs:38-47constructs the audit pool without a lifecycle observer, whilemain.rs:259supplies the observer for the main pool. Both emit the same writer metrics. Please clarifydeploy/charts/buzz/README.md:217-221: with audit enabled, metric totals include main + audit pools, but lifecycle receipts describe only the main pool. They cannot be reconciled one-for-one. - Tighten production-path regression evidence and the testing claim. The new huddle acquire at
crates/buzz-db/src/store/event.rs:240-244is correctly wired, but its test at:2806-2842asserts returned links, not telemetry; the operation-label matrix does not invoke this method. Add an isolated non-emptyDb::huddle_started_linkscall asserting its own start/terminal deltas. Separately, connection cancellation is exercised by directly dropping a guard (connection_observability.rs:534-573), not by cancelling the actual connection-setup future. Narrow the PR’s cancellation-coverage claim or add that production-path case. Neither gap establishes a current runtime defect.
Validation: source/diff review of the production paths and SQLx 0.9 connection/retry behavior, integrated with three independent review lanes; clean worktree and git diff --check. Existing exact-head Rust, PostgreSQL, relay/backend integration, and Helm checks passed. No local suite or staging workflow was rerun. The reported staging deployment used pre-rebase head f58e9480a4f068db0c591f604fd6800fdd4bfc45, not this SHA.
Signed-off-by: Ravneet Arora <rarora@squareup.com>
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Requesting changes for one production-path reliability issue. The connection observer is synchronous and the shipped implementation performs locked stderr I/O from SQLx connection setup. Observability must not become a dependency of database availability.
| // SQLx 0.9 exposes no callback immediately before each raw | ||
| // physical dial. Entering `after_connect` is the truthful | ||
| // point at which DNS/network/TLS/authentication succeeded. | ||
| record_milestone( |
There was a problem hiding this comment.
🤖 [P2] Keep lifecycle I/O out of the connection critical path. record_milestone synchronously reaches DbConnectionObserver::record; the relay's production observer ultimately takes stderr().lock() and writes JSON (crates/buzz-relay/src/lifecycle.rs:188-195). If the container logging pipe backpressures, this after_connect future cannot progress, an async timeout cannot interrupt the blocked write, and startup or pool replenishment can stall. We reproduced this boundary with the production sink: an emission remained blocked beyond a 100 ms Tokio timeout and completed only after the stderr pipe was drained (~755 ms). Route runtime receipts through a bounded nonblocking queue with explicit drop accounting, isolate observer panics, and add a blocked/panicking sink regression proving connection setup remains bounded.
There was a problem hiding this comment.
🤖 Addressed at ffb5fcb114a9c986ffd0cb2cfc9d413a32d5b1aa by removing the database lifecycle observer/receipt path entirely rather than adding a queue. SQLx connection setup now emits metrics only; the synchronous stderr bridge, observer callbacks, connection ordinals, and receipt-specific tests/docs are gone. Exact-head package and live PostgreSQL regressions pass, and a release relay completed health/readiness plus channel create → send → read-back while emitting zero database lifecycle receipts. Please re-review this simplified path.
Signed-off-by: Ravneet Arora <rarora@squareup.com>
Signed-off-by: Ravneet Arora <rarora@squareup.com>
bradseiler
left a comment
There was a problem hiding this comment.
Reviewed current head c8ccc8cb8e6c12c523c10dfd7c088a6775d0e486 against main, including the outstanding requested-change thread.
No blocking findings.
The prior P2 is addressed: commit ffb5fcb114a9c986ffd0cb2cfc9d413a32d5b1aa removes the database lifecycle observer/receipt path rather than putting synchronous lifecycle I/O behind a queue. The current production after_connect path now performs only bounded-label metrics updates around the existing asynchronous SQL setup; DbConnectionObserver, LifecycleRecorder, connection ordinals, and the synchronous stderr().lock() bridge are absent from the PR diff. This removes the logging-pipe backpressure dependency that motivated the change request.
The implementation otherwise matches the description: it adds checkout-start telemetry, instruments writer-pool construction and sequential session-safety phases, preserves the existing SQL and error propagation, wires huddle history through the typed acquire wrapper, freezes cardinality, and documents the operator model. The earlier non-blocking huddle telemetry gap is now covered by a production-path regression, and the audit/main-pool metric aggregation is documented. Current required checks are green, including Rust, PostgreSQL, relay/backend integration, desktop, security, and DCO.
Utility: high. These metrics distinguish pool saturation from physical/session setup failures during startup and replenishment without introducing request-controlled labels or per-connection logging.
Why
Current pool metrics show checkout outcomes and pool state after the fact. They do not show when a checkout began or which writer-connection setup step failed. During startup and pool growth, operators need to distinguish pool saturation from a slow or unsafe connection setup.
What
physical_connect,created_at_floor,session_timeouts,isolation, andready.How
The existing typed checkout wrapper records starts and current waiters. The huddle-history path uses that wrapper instead of a raw pool checkout.
The production SQLx
after_connecthook records start, terminal outcome, and duration for each sequential safety step. A drop guard records cancellation once if setup exits before a terminal result. Fixed enums keep every label bounded.For a phase,
started_total - sum(attempts_total)is the number of in-progress attempts on that pod. Because setup is sequential, a later phase starting also proves the earlier phases succeeded.The database path emits metrics only. This PR intentionally does not add per-connection lifecycle receipts or direct
stderrwrites.Risk
Medium. This changes the production writer-pool
after_connecthook and adds one counter update to instrumented checkout paths. It does not change the database safety statements or their failure behavior. The telemetry has fixed labels and no per-connection identifiers.Testing
At exact head
ffb5fcb114a9c986ffd0cb2cfc9d413a32d5b1aa, a release relay was started against isolated local PostgreSQL, Redis, and MinIO services. The main health endpoint returnedok, readiness returned{"status":"ready"}, the connection-step metrics were exported, and no database lifecycle receipts were emitted. A release CLI then created a channel, sent a message, and read the same event back successfully.The earlier staging deployment used pre-rebase head
f58e9480a4f068db0c591f604fd6800fdd4bfc45. The deployed multi-architecture image came from GitHub Actions run 33780888255, manifestsha256:b9351fa644e08376cbe1999f9bee311d33d1799a68eadef7929c4f862a832fec.The staging deployment brought both pods in ReplicaSet
buzz-6c8758bd7dto Ready with zero restarts, and the connection metrics produced data in the rollout dashboard. That staging image predates the cleanup that removed per-connection lifecycle logs; the metric schema and database safety statements are unchanged.Verification
cargo test -p buzz-db -- --test-threads=1: 128 tests passed across the package and integration target; 255 opt-in tests remained ignored.buzz-acpfailures: one timing-sensitive keepalive test and two environment-default tests.Bigger picture
This is the database-metrics part of the startup and rollout observability work. The early-startup lifecycle foundation merged in #7258, so the rebase removed that duplicate commit from this PR.
Originating discussion: buzz://message?channel=6ac85131-70cd-4bda-a031-38d34114934e&id=fa2bed181c092697210a60bb6eedc55a665d5c1c6cabc1413a04686647011f71
Generated with Codex