Conversation
…loop Both were running every Tick (1-second cadence), but they're periodic-snapshot metrics — nothing changes meaningfully at 1 s granularity. sampleActiveUptimes calls sessionUptime.Record on every Ready session per Tick, so at 100 sessions this was ~6000 OTel histogram writes / min + the accompanying toOtelMetricAttrs allocations (visible in heap profiles as multi-MB residence). Matches java-bigtable's session.uptime cadence at MetricsImpl.java: 193 — scheduleAtFixedRate(this::recordAsyncSessionMetrics, 1, 1, TimeUnit.MINUTES). Changes: - Drop the two calls from Tick's body (session_pool_scaling.go). - Add slowMetricsInterval = 1 minute and startSlowMetricsLoop, mirroring the existing startSweepStuckSessionsLoop / start AfePruneLoop patterns (dedicated goroutine per concern). - Wire into Start() alongside the other periodic loops. Tick keeps its 1-s cadence for sizing decisions (still needed for reactive scale-up on burst arrivals — Java handles that via event-driven hooks, but Go's current shape uses Tick).
product-auto-label
Bot
added
api: bigtable
![gemini-code-assist[bot]](/web-api/iframe-proxy?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request decouples the metrics collection (recordTimeSeries and sampleActiveUptimes) from the 1-second Tick loop, moving them to a separate 1-minute interval loop (startSlowMetricsLoop) to reduce OTel histogram write overhead. The reviewer noted that this introduces a 1-minute delay for the initial metrics collection, which could lead to missing metrics in short-lived processes, and suggested executing an initial collection immediately upon starting the loop.
4 tasks
sushanb
added a commit
that referenced
this pull request
Jul 31, 2026
…0285) > Stacked on **#20283** (\`fix/bigtable-uptime-sample-cadence\`). Do not merge before that lands. ## Summary After #20283 moved \`recordTimeSeries\` and \`sampleActiveUptimes\` to a dedicated 1-min loop, \`Tick\`'s body is pure sizing (\`sizer.Decide\` + \`createSession\` fan-out). Every meaningful pool-state transition already fires \`spawnTickOnce\` so a background timer is redundant. ## Event coverage audit | Trigger | Site | Fires | |---|---|---| | Session became Ready | \`onActive\` | \`signalFree\` (wakes waiters) — no Tick needed since a Ready-count increment doesn't want more sessions | | Session leaving Ready | \`onClosing\` (\`session_pool_lifecycle.go:326\`) | \`spawnTickOnce\` — kicks a replacement decision | | Waiter park on empty pool | \`CheckoutSession\` (\`session_pool.go:256\`) | \`spawnTickOnce\` — kicks a cold-start / burst-scale decision | | Server-driven config change | \`UpdateConfig\` (**new** in this PR) | \`spawnTickOnce\` — covers the one path where none of the above events fire (e.g., \`MinSessionCount\` bumped on an idle pool) | | Pool cold start | \`Start()\` (\`session_pool_lifecycle.go\`) | Pre-start \`spawnTickOnce\` seeds MinSessions | \`spawnTickOnce\` is CAS-guarded via \`tickPending\`, so a burst of listener fires coalesces to one Tick body. ## Java parity Matches java-bigtable's fully event-driven \`poolSizer\`: no periodic timer for sizing. Sizing decisions ride on the events that would trigger them anyway. ## Impact - **Idle pool steady-state:** was ~1 Tick / sec (\`sizer.Decide\` + \`p.mu\` bracket + \`sl\` walk) = ~200-500 ns of CPU + lock contention per pool per second. **Now zero** between events. - **Under load:** unchanged — same event-driven scale-up paths fire. - **Under bursty saturation:** unchanged — \`CheckoutSession\`'s empty-pool kick already covered burst reaction, not the periodic Tick. ## Changes - Delete \`startTickLoop\` + \`tickInterval\` const (\`session_pool_lifecycle.go\`). - Drop \`p.startTickLoop(ctx)\` from \`Start()\`; keep the pre-start \`spawnTickOnce\` that seeds MinSessions. - Add \`p.spawnTickOnce(p.poolCtx)\` at the end of \`UpdateConfig\` (\`session_pool.go\`). Net: +9 / -25 lines. ## Test plan - [x] \`go build ./...\` clean. - [x] \`go vet ./internal/transport/\` clean. - [x] \`go test -race -count=5 -short -timeout=180s -run 'TestSessionPool|TestCheckoutSession|TestPool_' ./internal/transport/\` — 5× stress pass. - [x] \`go test -race -count=1 -short ./internal/session/\` — pass. ## What this does NOT change - \`Tick\` itself (\`SessionPoolImpl.Tick\`) is unchanged — it just no longer runs on a timer. \`spawnTickOnce\` / \`tickOnce\` still wrap it with debounce + panic recovery. - \`sweepStuckSessions\` (its own 30 s loop), AFE prune (its own loop), slow-metrics (its own 1-min loop from #20283) — all unchanged. - \`Close\` teardown — \`spawns\` WaitGroup semantics unchanged since \`spawnTickOnce\` still bumps \`spawns.Add\` before the goroutine.
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Both `recordTimeSeries` and `sampleActiveUptimes` were running every `Tick` (1-second cadence), but they're periodic-snapshot metrics — nothing changes meaningfully at 1s granularity. `sampleActiveUptimes` calls `sessionUptime.Record` on every Ready session per Tick, so at 100 sessions this was ~6,000 OTel histogram writes / min plus the accompanying `toOtelMetricAttrs` allocations (visible in heap profiles as multi-MB residence).
Matches java-bigtable's `session.uptime` cadence at `MetricsImpl.java:193`: `scheduleAtFixedRate(this::recordAsyncSessionMetrics, 1, 1, TimeUnit.MINUTES)`.
Changes
`Tick` keeps its 1-s cadence for sizing decisions (still needed for reactive scale-up on burst arrivals — Java handles that via event-driven hooks, but Go's current shape uses Tick).
Impact estimate
For a pool with N sessions:
At the 100-session sandbox pod: from ~6,000 histogram writes/min → 100. Heap-profile residency for `toOtelMetricAttrs` should drop correspondingly.
Test plan
Verification (post-merge)