Skip to content

Count connection pool busy time - #1

Merged
tycooon merged 3 commits into
masterfrom
codex/busy-seconds
Oct 30, 2026
Merged

tycooon merged 3 commits into
masterfrom
codex/busy-seconds

Conversation

@tycooon

@tycooon tycooon commented Oct 30, 2026 •

Copy link
Copy Markdown
Member

What

Add sequel_connection_pool_busy_seconds_total with db/server tags and sum aggregation. Its rate gives the average number of connections in use; dividing by sequel_connection_pool_size gives 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

  • Time genuine checkouts. Start after private acquire returns and finish at private release, 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.
  • Keep nested queries free of bookkeeping. There is no hold wrapper 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 per sample_interval, rather than on every checkout.
  • Drop stale time safely. A successful acquisition overwrites any stale timer for that owner/server. Unconfigured acquisitions clear stale timers too, so configuring Yabeda before release cannot report an earlier checkout. PID changes discard inherited state. Every metric write goes through Yabeda::Sequel.safely.
Sequel path Busy-time treatment
Normal release, including a block raising a normal error Finalize once at release; threaded pools write the counter on the next sampler tick. Nested holds bypass the instrumentation entirely.
Disconnect paths bypassing release Drop the checkout's duration. Its stale timer is overwritten on that owner's next acquisition; the new checkout starts at its own acquisition time.
DatabaseDisconnectError in sharded_threaded This implementation calls release, so its duration is counted normally.
connection_validator / connection_expiration rejecting a connection during acquisition No busy timer exists until the outer acquire returns a usable connection. Discarded attempts are excluded from busy time; acquisition retries remain part of wait time.
Dead Thread/Fiber reclamation Drop abandoned timers during the next eligible acquisition, at most once per sample interval.
temporarily_release_connection No dedicated support or wrappers. The extension's calls to release and acquire naturally 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.
Fork during a hold Discard inherited timer state in the child; fresh child checkouts are measured normally.

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: false continues 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 --check and git 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 single and sharded_single pools. 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.

@tycooon tycooon left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 hold wrapper and the hold-scope stack. Start the timer in acquire, finish it in release.
  • A disconnect path that bypasses release leaves a stale entry. The owner's next acquire overwrites 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 tycooon-review-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI reviewer: Codex. Reviewed fa1ec89.

Comment thread lib/yabeda/sequel/checkout_timers.rb Resolved
@tycooon

tycooon commented Oct 30, 2026

Copy link
Copy Markdown
Member Author

Codex: Addressed the P1 hot-path finding in 96072e1.

  • Removed the hold wrapper, scope stack, and per-checkout structs. Nested holds now take no bookkeeping lock; only genuine acquire/release calls touch timers.
  • Successful acquisitions overwrite stale owner/server timestamps. Unconfigured acquisitions also clear stale timers, preventing an old increment if Yabeda is configured before release. Disconnect paths that bypass release are dropped.
  • Dead-owner pruning runs at most once per sample_interval; the PID reset remains.
  • No dedicated temporary-release handling remains. Regression specs verify that the extension's acquire/release calls exclude the released interval and that a disconnect after reacquisition cannot contaminate the next checkout.
  • Updated the README and reduced the 0.2.0 changelog to one user-facing line. Also fixed the initial Sequel 5.60 CI failures by probing for the temporary-release extension before running its specs.

Validation: bundle exec rspec passed with 199 examples, 0 failures; bundle exec rubocop --no-server passed with 25 files and no offenses. The nested-lock and pruning-cadence regressions failed against the previous implementation and pass with the fix. I am waiting for CI on this head and checking for further review feedback; no merge will be performed.

@tycooon tycooon left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 tycooon-review-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI reviewer: Codex. Reviewed 96072e1.

@tycooon tycooon left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 tycooon-review-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI reviewer: Codex. Reviewed 855374b.

@tycooon
tycooon merged commit be36e06 into master Oct 30, 2026
10 checks passed
@tycooon
tycooon deleted the codex/busy-seconds branch October 30, 2026 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants