feat(bigtable): add session.Config.EnableDebug to gate sessionz debug state - #20247
Merged
sushanb merged 1 commit intoJul 28, 2026
Merged
Conversation
… state
The session pool records ~30 sites of debug/snapshot state on every
vRPC and state transition (pick candidates, per-session events/latency
rings, pool-wide latency histograms, per-AFE pick counts, slow-vRPC
log, scaling history, session-lifetime buckets, per-session cluster
counts, time-series sparklines). Even when nobody has wired the
debugview handler and no scraper will ever read the snapshots, the
recorders still allocate on the hot path.
Add a single opt-in session.Config.EnableDebug bool — default false —
that turns off every allocating debug recorder while preserving the
feature-complete debug surface when the flag is on.
Threading:
session.Config.EnableDebug
→ sessionClient.enableDebug
→ NewSessionPoolImpl(debugEnabled bool)
→ SessionPoolImpl.debugEnabled
→ Session.debugEnabled (via new WithSessionDebugEnabled option,
wired at createSession time so every session in a pool inherits
the pool's flag)
Gated recorders (early-return on !p.debugEnabled / !s.debugEnabled):
- session_pool_debug.go: recordPickDecision, recordLifetime,
recordTimeSeries, recordSlowVRpc
- session_pool_scaling.go: recordScaling
- session_debug.go: Session.recordEvent, Session.recordLatency,
Session.recordCluster
- session_pool.go Invoke: pool-wide totalLatencyHist /
backendLatencyHist / transportLatencyHist bumps
(RecordTransportOverhead — customer-facing OTel metric — is NOT
gated; only the debug-only histogram inserts are)
Picker:
Each of the three AfePicker impls (SimpleAfePicker,
LeastInFlightAfePicker, LeastLatencyAfePicker) gains a
recordCandidates bool set at construction. kChoiceMinCost skips the
per-pick []PickCandidate slice allocation when false; PickDecision
still populates Winner+Reason for the caller. pickerFromLoadBalancing
threads p.debugEnabled through so every picker in a pool matches.
Nil-return contract:
sessionClient.SessionDebug() returns nil when !sc.enableDebug so a
downstream debugview handler can render a "not enabled" panel
instead of empty snapshots.
Ungated intentionally (branch check ≥ cost of the work; noted in
session_pool.go comment):
msgsSent/msgsRecv atomic counters, retries/okRpcs/errorRpcs, and
the dual-purpose fields (outstanding, remoteAddr, lastConnected).
Tests:
- New bigtable/internal/transport/session_pool_debug_disabled_test.go
pins the gate itself: constructs a pool + session with
debugEnabled=false, drives every gated recorder, asserts every
ring/map stays at len=0. Without this test a future refactor that
inverts one of the gates would land green.
- Existing newTestSession helper enables debug by default (tests
exercise ring buffers) — WithSessionDebugEnabled godoc explicitly
calls out the test-author footgun.
- Existing NewSessionPoolImpl + picker-constructor test callers
updated (bulk) to pass true so the current debug-on assertions
still run.
Scope note:
This PR is scoped to the internal gate mechanics; NewClient does not
yet accept EnableDebug as a positional argument because it has no
external caller upstream today (would be a dead knob). Once the
top-level bigtable.Client wiring lands, that follow-up PR can plumb
bigtable.ClientConfig.EnableClientDebug into session.Config.EnableDebug
directly.
Deferred follow-ups (non-blocking, noted per reviewer feedback):
- Public bigtable.ClientConfig.EnableClientDebug + wiring path
(needs top-level bigtable.Client session integration first).
- Debugview "not enabled" banner (debugview package not upstream yet).
- Compile-time exclusion via //go:build sessionz — build-system work.
- Sampled snapshots (1-in-N) middle mode.
- Noop provider vs nil-return refactor.
- Picker options struct to replace the trailing recordCandidates bool.
Reviewers cleared: session-reviewer, session-component-review,
igor-reviewer (all in parallel per the CLAUDE.md hook).
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces an EnableDebug configuration option to conditionally disable per-pool and per-session debug recorders, eliminating hot-path allocation overhead when debug features are not needed. It propagates this flag to the session client, pool implementation, AFE pickers, and individual sessions, and adds a test suite to verify the zero-allocation behavior when debug is disabled. The review feedback correctly identifies a package name mismatch in the newly added test file session_pool_debug_disabled_test.go, which should be updated to package transport to allow accessing unexported fields and prevent compilation errors.
mutianf
approved these changes
Jul 28, 2026
sushanb
pushed a commit
that referenced
this pull request
Aug 3, 2026
🤖 I have created a release *beep* *boop* --- ## [1.52.0](bigtable/v1.51.0...bigtable/v1.52.0) (2026-08-03) ### Features * **bigtable:** Add AFE picker (Simple / LeastInFlight / LeastLatency) ([#20204](#20204)) ([bcbf714](bcbf714)) * **bigtable:** Add ClientConfig.DisableSession to opt out of session backend ([#20297](#20297)) ([7ee5e44](7ee5e44)) * **bigtable:** Add getClientConfigDirectAccessChecker for session pools ([#20209](#20209)) ([3b8d30a](3b8d30a)) * **bigtable:** Add NoOpChannelPrimer for session channel pools ([#20208](#20208)) ([d055a8a](d055a8a)) * **bigtable:** Add per-AFE sessionList for the two-tier session pool ([#20224](#20224)) ([dbf0c3f](dbf0c3f)) * **bigtable:** Add protoRowToRow conversion helper for TableShim ([#20257](#20257)) ([1297143](1297143)) * **bigtable:** Add Session debug surface (observability fields + methods) ([#20211](#20211)) ([d8d3e16](d8d3e16)) * **bigtable:** Add Session lifecycle (Start, Close, ForceClose, readLoop, heartBeatLoop) ([#20215](#20215)) ([b9e53c6](b9e53c6)) * **bigtable:** Add Session struct + state machine ([#20117](#20117)) ([09acbb3](09acbb3)) * **bigtable:** Add session.Config.EnableDebug to gate sessionz debug state ([#20247](#20247)) ([ce74c31](ce74c31)) * **bigtable:** Add SessionClient + SessionTable + lazyPool ([#20228](#20228)) ([ab2c96c](ab2c96c)) * **bigtable:** Add SessionPoolImpl (two-tier pool + scaling + debug) ([#20225](#20225)) ([683eda8](683eda8)) * **bigtable:** Rename session pool display to <resource-id>-<PERM> ([#20248](#20248)) ([35e146e](35e146e)) * **bigtable:** Route Client.Open()-returned *Table through the Diverter ([#20273](#20273)) ([2b81c7d](2b81c7d)) * **bigtable:** State-based classification for abnormal session close ([#20243](#20243)) ([f2905b7](f2905b7)) * **bigtable:** TableShim fallback to classic on session UNIMPLEMENTED ([#20269](#20269)) ([36540af](36540af)) * **bigtable:** TTL-on-idle cache for per-resource session.TableAPI ([#20263](#20263)) ([00b2a49](00b2a49)) * **bigtable:** Wire Diverter on Client and route Open* via TableShim ([#20256](#20256)) ([b32fbd7](b32fbd7)) ### Bug Fixes * **bigtable:** AFE picker latency signal — subtract poolWait and compute TransportLatency = wire − backend at source ([#20281](#20281)) ([bb8c4d5](bb8c4d5)) * **bigtable:** Guard NewStream OnFinish against grpc-go double-fire ([#20295](#20295)) ([b51da29](b51da29)) * **bigtable:** Real per-resource pool teardown on sessionTable.Close + cache close-race gate ([#20264](#20264)) ([599aea9](599aea9)) * **bigtable:** Session.durations / session.uptime — set explicit histogram bucket boundaries ([#20276](#20276)) ([97eee22](97eee22)) * **bigtable:** SessionTableHandle self-heals across cache eviction ([#20296](#20296)) ([0dd98cd](0dd98cd)) * **bigtable:** Translate ctx errors to gRPC status on session vRPC ([#20299](#20299)) ([0f3b2a5](0f3b2a5)) * **bigtable:** Treat PingAndWarm NotFound as a successful prime ([#20219](#20219)) ([a1557ad](a1557ad)) ### Performance Improvements * **bigtable:** Delete periodic Tick loop; sizing is event-driven ([#20285](#20285)) ([2c096bd](2c096bd)) * **bigtable:** Drop pick_lost_race debug tag from CheckoutSession hot path ([#20280](#20280)) ([bd0e400](bd0e400)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
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
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
Add a single opt-in
session.Config.EnableDebug bool(default false) that gates every allocating debug/snapshot recorder in the session pool. When off, the hot path is zero-allocation on the recording side; when on, the existing feature-complete debug surface (sessionz / afez / loadz) is preserved.Motivation: the session pool records ~30 sites of debug state on every vRPC and state transition (per-session events/latency rings, pool-wide latency histograms, per-AFE pick counts, slow-vRPC log, scaling history, per-pick candidate slices, etc.) — even when nobody has wired the debugview handler and no scraper will ever read the snapshots.
Threading
Gated recorders
Early-return
if !p.debugEnabled { return }/if !s.debugEnabled { return }:session_pool_debug.go—recordPickDecision,recordLifetime,recordTimeSeries,recordSlowVRpcsession_pool_scaling.go—recordScalingsession_debug.go—Session.recordEvent,Session.recordLatency,Session.recordClustersession_pool.go Invoke— pool-widetotalLatencyHist/backendLatencyHist/transportLatencyHistbumpsRecordTransportOverhead (customer-facing OTel metric) is NOT gated; only the debug-only histogram inserts are.
Picker
Each of the three
AfePickerimpls (SimpleAfePicker,LeastInFlightAfePicker,LeastLatencyAfePicker) gains arecordCandidates boolfield set at construction.kChoiceMinCostskips the per-pick[]PickCandidateslice allocation when false;PickDecisionstill populatesWinner+Reasonfor the caller.pickerFromLoadBalancingthreadsp.debugEnabledthrough so every picker in a pool matches.Nil-return contract
sessionClient.SessionDebug()returns nil when!sc.enableDebugso a downstream debugview handler can render a "not enabled" panel instead of empty snapshots.Ungated intentionally
Pure atomic counters (
msgsSent,msgsRecv,retries,okRpcs,errorRpcs) — a branch check costs more than the atomic. Documented insession_pool.go.Scope note
NewClientdoes NOT yet acceptEnableDebugas a positional argument because it has no external caller upstream today (would be a dead knob). Once the top-levelbigtable.Clientsession integration lands, that follow-up PR can plumbbigtable.ClientConfig.EnableClientDebugintosession.Config.EnableDebugdirectly.Test plan
go test ./bigtable/internal/transport/ ./bigtable/internal/session/ -count=1 -race -short -timeout=180sTestSessionPool_DebugDisabled_NoRecorderState+TestSession_DebugDisabled_NoRingBufferStatepin the zero-alloc contract — drive every gated recorder withdebugEnabled=falseand assert every ring/map stays at len=0.newTestSessionhelper enables debug by default (tests exercise ring buffers) so no other test needs to change.NewSessionPoolImpl+ picker-constructor test callers bulk-updated to passtrueso existing debug-on assertions still run.Reviewers
Three subreviewers cleared: session-reviewer (behavioral), session-component-review (Part B + Part C boundaries), igor-reviewer (persona). All ran in parallel per the repo's session-file hook.
Follow-ups (not in this PR)
bigtable.ClientConfig.EnableClientDebug+ wiring path (needs top-levelbigtable.Clientsession integration first).//go:build sessionzfor prod images.recordCandidates boolon three constructors.