Skip to content

feat(bigtable): add session.Config.EnableDebug to gate sessionz debug state - #20247

Merged
sushanb merged 1 commit into
googleapis:mainfrom
sushanb:feat/bigtable-enable-client-debug
Jul 28, 2026
Merged

feat(bigtable): add session.Config.EnableDebug to gate sessionz debug state#20247
sushanb merged 1 commit into
googleapis:mainfrom
sushanb:feat/bigtable-enable-client-debug

Conversation

@sushanb

@sushanb sushanb commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

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

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 if !p.debugEnabled { return } / if !s.debugEnabled { return }:

  • session_pool_debug.gorecordPickDecision, recordLifetime, recordTimeSeries, recordSlowVRpc
  • session_pool_scaling.gorecordScaling
  • session_debug.goSession.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 field 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

Pure atomic counters (msgsSent, msgsRecv, retries, okRpcs, errorRpcs) — a branch check costs more than the atomic. Documented in session_pool.go.

Scope note

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 session integration lands, that follow-up PR can plumb bigtable.ClientConfig.EnableClientDebug into session.Config.EnableDebug directly.

Test plan

  • go test ./bigtable/internal/transport/ ./bigtable/internal/session/ -count=1 -race -short -timeout=180s
  • New TestSessionPool_DebugDisabled_NoRecorderState + TestSession_DebugDisabled_NoRingBufferState pin the zero-alloc contract — drive every gated recorder with debugEnabled=false and assert every ring/map stays at len=0.
  • Existing newTestSession helper enables debug by default (tests exercise ring buffers) so no other test needs to change.
  • NewSessionPoolImpl + picker-constructor test callers bulk-updated to pass true so 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)

  • Public bigtable.ClientConfig.EnableClientDebug + wiring path (needs top-level bigtable.Client session integration first).
  • Compile-time exclusion via //go:build sessionz for prod images.
  • Sampled snapshots (1-in-N) middle mode.
  • Noop provider vs nil-return refactor.
  • Picker options struct to replace the trailing recordCandidates bool on three constructors.

… 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).
@sushanb
sushanb requested review from a team as code owners July 28, 2026 21:35
@product-auto-label product-auto-label Bot added the api: bigtable Issues related to the Bigtable API. label Jul 28, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread bigtable/internal/transport/session_pool_debug_disabled_test.go
@sushanb
sushanb merged commit ce74c31 into googleapis:main Jul 28, 2026
19 checks passed
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: bigtable Issues related to the Bigtable API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants