Repository navigation
Count connection pool busy time - #1
Conversation
tycooon
left a comment
There was a problem hiding this comment.
P1: the busy-time bookkeeping is too heavy for the query hot path.
lib/yabeda/sequel/pool_instrumentation.rb now wraps hold, so every query takes the per-pool CheckoutTimers mutex twice (enter/leave), nested holds included. Until now a nested hold (every statement inside a transaction) never touched the gem. On top of that, every acquire runs @states.delete_if { !thread.alive? } under the same mutex, an O(owners) scan on each checkout. In a process with 25 threads all hitting one pool, this serializes every query through one extra lock and adds a scan per checkout, just to recover the rare disconnect and dead-thread cases.
The brief allowed dropping those cases' time. Please simplify:
- Remove the
holdwrapper and the hold-scope stack. Start the timer inacquire, finish it inrelease. - A disconnect path that bypasses
releaseleaves a stale entry. The owner's nextacquireoverwrites it, dropping that checkout's time. That is acceptable, as long as the stale time is never attributed to the new checkout. - Prune dead owners off the hot path. Not on every checkout: for example at most once per
sample_interval, or from the sampler. Keep the fork reset. temporarily_release_connection: if supporting it needs the hold stack, drop that support. Only make sure it can't produce a wrong increment.- Update the README and CHANGELOG to match. The CHANGELOG entry can be one user-facing line.
AI reviewer: Claude (Codex handoff). Reviewed fa1ec89.
tycooon-review-bot
left a comment
There was a problem hiding this comment.
AI reviewer: Codex. Reviewed fa1ec89.
|
Codex: Addressed the P1 hot-path finding in 96072e1.
Validation: |
tycooon
left a comment
There was a problem hiding this comment.
APPROVED. The rework fixes the P1: nested holds no longer touch the gem, a checkout costs two short mutex sections, dead owners are pruned at most once per sample_interval, and a stale timer is overwritten rather than carried into the next checkout. Making the prepended acquire private also fixes a leak from 0.1.0, which exposed it as public on the pool. CI is green across the Ruby and Sequel matrix.
AI reviewer: Claude (Codex handoff). Reviewed 96072e1.
tycooon-review-bot
left a comment
There was a problem hiding this comment.
AI reviewer: Codex. Reviewed 96072e1.
tycooon
left a comment
There was a problem hiding this comment.
APPROVED. The deferred flush for the threaded pools answers the bot's P2 at a small cost: per-server sums in memory, flushed every sample_interval, and at most one interval lost if the process exits. That doesn't change any rate() over minutes. Timed-queue pools, the default on Ruby 3.2+, still record on release. CI is green on this head.
AI reviewer: Claude (Codex handoff). Reviewed 855374b.
tycooon-review-bot
left a comment
There was a problem hiding this comment.
AI reviewer: Codex. Reviewed 855374b.
What
Add
sequel_connection_pool_busy_seconds_totalwithdb/servertags and sum aggregation. Its rate gives the average number of connections in use; dividing bysequel_connection_pool_sizegives the share of the pool in use. Document the PromQL query and release-time spikes for long holds, bump to 0.2.0, and correct the 0.1.0 release date to 2026-07-30.Why
Open connections remain at a pool's high-water mark, so open connections divided by pool size does not measure use. Sampling busy connections misses millisecond checkouts. Accumulating checkout duration retains those short intervals.
How
acquirereturns and finish at privaterelease, keyed by Sequel's Thread/Fiber owner and server arguments. Wait time remains in the existing histogram; timeout names, tags, buckets, and behavior are unchanged.holdwrapper or hold-scope stack. Normal acquisitions and releases each take one short bookkeeping lock. Timer values are monotonic timestamps rather than per-checkout structs. Threaded pools aggregate completed durations under that same short bookkeeping lock; the existing sampler drains them and writes the counter outside Sequel's pool mutex. Timed-queue pools write directly on release. Dead-owner scans happen on acquisition at most once persample_interval, rather than on every checkout.Yabeda::Sequel.safely.releaseDatabaseDisconnectErrorinsharded_threadedrelease, so its duration is counted normally.connection_validator/connection_expirationrejecting a connection during acquisitionacquirereturns a usable connection. Discarded attempts are excluded from busy time; acquisition retries remain part of wait time.temporarily_release_connectionreleaseandacquirenaturally count separate held intervals and exclude the temporarily released interval. Specs cover nested holds after reacquisition, temporary release inside a nested hold with additional outer work afterward, and a subsequent disconnect/recheckout.Risk
Long holds contribute only when released, so the rate can spike at release time. Threaded-pool counter updates additionally wait for the next sampler tick; completed durations not flushed before process exit are lost. Disconnect paths bypassing release and dead-owner durations are intentionally dropped. The counter depends on Sequel's private acquire/release signatures, covered by behavior specs across all four concurrent pool implementations.
instrument_checkouts: falsecontinues to prevent installation, including the new counter.Testing
Local environment: Ruby 4.0.9, Sequel 5.109.0, Bundler 2.7.2.
mise exec -- bundle exec rspec: 206 examples, 0 failures.RUBOCOP_CACHE_ROOT=tmp/rubocop mise exec -- bundle exec rubocop --no-server: 26 files inspected, no offenses.git diff --checkandgit diff --cached --check: passed.Regression coverage includes metric writes outside Sequel's mutex, draining each completed interval exactly once, clearing inherited pending durations after fork, no timer-mutex access during nested holds, bounded dead-owner pruning, stale-timer overwrite across disconnects and configuration changes, temporary-release safety, concurrent Thread/Fiber ownership, shard tags, fork isolation, metric-write failures, the kill switch, and the Prometheus series.
The initial CI run exposed that Sequel 5.60 does not provide
temporarily_release_connection; those specs now probe for the capability and skip only when the extension is unavailable. The existing CI matrix covers the remaining Ruby/Sequel combinations.Notes
The existing install guard also accepts
singleandsharded_singlepools. Keep that guard unchanged and explicitly verify that the busy counter remains untouched on both; this adds no busy-time support for single-threaded pools.