feat(bigtable): add Session struct + state machine - #20117
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces internal transport and metrics components for the Bigtable Go client, including session lifecycle management, vRPC dispatching, attempt outcome classification, and extensive observability tracing. The review feedback is highly constructive and identifies critical improvements: resolving a potential data race on the global debugTagCounter using atomic.Value, preventing alignment panics on 32-bit architectures by migrating raw int64 fields in SessionHandle to typed atomic.Int64, and eliminating an unnecessary heap allocation on the hot path in recordCluster by checking the map with a lock-free Load first.
3412c25 to
faf9c2f
Compare
… in recordCluster Addresses gemini review on googleapis#20117: - Rename picker.go -> session_handle.go (only holds SessionHandle; the picker itself lives in afe_picker.go). - SessionHandle: switch outstanding/lastActivity/picks from raw int64 + sync/atomic Load/Add/Store to typed atomic.Int64. Removes the 32-bit alignment footgun (SessionHandle has a pointer field first, so the raw int64 fields were not guaranteed 8-byte-aligned) and is more idiomatic on Go 1.19+. - Session.recordCluster: add a lock-free Load fast path so the common case (cluster already registered) no longer allocates a fresh atomic.Int64 that LoadOrStore would immediately discard.
…s#20117 CI) CI vet reported: bigtable/internal/transport/session.go:298:27: exported method AfeID returns unexported type internal.afeID, which can be annoying to use The accessor was already exported; the identifier value type was not. Renamed `type afeID int64` to `type AfeID int64` (documented semantics — signed 64-bit, zero is the "unknown" sentinel, mirrors Java's AutoValue `AfeId` — are preserved verbatim). Updated the single internal cast in session.go and the one test that constructs the value. No behavioral change: state machine, PeerInfo timing, hook ordering, retry oracle, and all metric labels are untouched. Both the session behavioral and component boundary reviewers pass.
| // via handleClose → cancelActiveRPCs (tagged StateTransportFailure). | ||
| ErrUnavailableGoAway = errors.New("bigtable: session unavailable: server sent GOAWAY") | ||
| // ErrUnavailableSessionError indicates the server reported a fatal | ||
| // session-level error (an ErrorResponse with rpc_id == 0). Delivered |
There was a problem hiding this comment.
Hmm, ErrorResponse is still a response for a vrpc i think? rpc_id == 0 would just mean it's the error response of the vrpc with id 0?
There was a problem hiding this comment.
Reserved-id convention (Java parity): SessionImpl.nextRpcId = new AtomicLong(1) (SessionImpl.java:115), so no live vRPC ever has id 0. Server uses rpc_id == 0 on ErrorResponse to mean "session-scoped fatal, not tied to a specific vRPC" — the routing lives in the follow-up PR's handleErrorResponse (splits GetRpcId() != 0 → handleVRPCErrorResponse, else → ForceClose with CLOSE_SESSION_REASON_ERROR).
[Edited: earlier version claimed the sentinel's doc-comment was expanded to spell this out; walked that back in 80b9565 — the convention belongs on handleErrorResponse in the follow-up, not on the sentinel. See follow-up.]
There was a problem hiding this comment.
Update — trimmed the doc-comment in 80b9565 down to just the semantics: "The session is torn down and any in-flight vRPCs are cancelled with this cause (via cancelActiveRPCs, tagged StateTransportFailure)."
Wire-protocol detail was misplaced on the sentinel — callers just errors.Is against it; they don't care how the server signaled it. The rpc_id == 0 convention + Java-parity note will live on handleErrorResponse in the follow-up PR that carries the actual routing code.
| // per-session lifetimes into the ring buffer. Zero for | ||
| // test-constructed handles that never went through OnActive — code | ||
| // paths that consume this must handle the zero-time case. | ||
| createdAt time.Time |
There was a problem hiding this comment.
what is this time gonna be used for? Do we really need it?
There was a problem hiding this comment.
We use to measure the lifetime of the Session.
We will invoke this on OnClosing().
There was a problem hiding this comment.
Consumed in the pool PR: SessionPoolImpl.recordLifetime(time.Since(sh.createdAt)) bins the session's lifetime into the sessionz per-pool ring, called from OnClosing (first out-of-Ready transition) and from Pool.Close's Phase-1 teardown loop. Without it we can't compute per-session age at retirement. Kept as a time.Time (not a Duration) so the two record sites can each snapshot time.Now() at their own moment without threading a start value through.
|
|
||
| // IncOutstanding increments outstanding calls. | ||
| func (h *SessionHandle) IncOutstanding() { | ||
| h.outstanding.Add(1) |
There was a problem hiding this comment.
why is last activity only updated when decrement outstanding?
There was a problem hiding this comment.
lastActivity semantically means "when did this session last become idle" — the interesting quantity for pool scale-down / stuck-sweep is now - lastCompletedActivity, not now - lastCheckedOut. Stamping on Inc would give the latter, which duplicates Picks() for "recent use frequency" and misses the actual idle-age signal. If the name reads ambiguously I can rename it to lastIdleAt in a follow-up — happy to do that if you'd prefer it clearer.
| sessionUptime metric.Float64Histogram | ||
| // transportLatencies is per-vRPC (not per-session-lifecycle), but shares | ||
| // the same meter + registration path so all session-adjacent metrics | ||
| // initialize together — matches java-bigtable's MetricRegistry layout. |
There was a problem hiding this comment.
remove "matches java-bigtable's MetricRegistry layout"
There was a problem hiding this comment.
Done in 05b9bd9 — dropped the phrase from transportLatencies's doc.
| meter := meterProvider.Meter(clientMeterName) | ||
|
|
||
| var err error | ||
| if sessionDurations, err = meter.Float64Histogram( |
There was a problem hiding this comment.
I think we need to align the buckets with java otherwise it'll get confusing. SessionDuration and OpenLatencies also should have different buckets because their expected latencies are pretty different.
There was a problem hiding this comment.
Done in 05b9bd9. Added two java-aligned bucket lists in session_tracer.go:
SessionDurationBounds— mirrorsClientSessionDuration.BUCKETS_MS([0] + generateGeometricSeq(1, 20min_ms)), wired tosession.durationsandsession.uptime(Java'sClientSessionUptime.BUCKETS_MSuses the same layout).SessionOpenLatencyBounds— mirrorsClientSessionOpenLatency.BUCKETS_MS(linear(0-3, 0.1) + linear(3-9, 1) + geometricSeq(10, 5min_ms)), wired tosession.open_latencies.
Split as you flagged — open buckets stop at ~2.7min, duration/uptime go to ~17.5min.
| // newSessionTracer starts the "open" timer. | ||
| func newSessionTracer(sessionType SessionType) *sessionTracer { | ||
| return &sessionTracer{ | ||
| startTime: time.Now(), |
There was a problem hiding this comment.
what's the difference between openedAt and startTime? why do we need both? when is the sessionTracer created?
There was a problem hiding this comment.
[Superseded — the tracer was consolidated to a single anchor in a7c2176; openedAt was removed. See follow-up on this thread. Original explanation below for context.]
sessionTraceris constructed insidesessionDebug.init, which runs fromNewSession— sostartTime = time.Now()at Session construction (beforeStart()sends theOpenSessionRequest).openedAtwas stamped byrecordOpen(ctx, err), called fromhandleOpenSessionat the Starting→Ready transition (after the server'sOpenResponseis received and PeerInfo is parsed).
Original rationale for both:
session.open_latencies=openedAt - startTime(how long the open took).session.durations=now - openedAtfor sessions that reached Ready, but falls back tonow - startTimefor sessions that died before ever opening.session.uptime(periodic sample) =now - openedAt— only defined for opened sessions.
The consolidation replaced openedAt time.Time with an opened bool label and moved all three metrics to msSince(startTime) uniformly — matches Java SessionTracerImpl.uptime.elapsed().
There was a problem hiding this comment.
Consolidated to a single anchor in a7c2176 — dropped sessionTracer.openedAt in favor of a bool opened flag, matching Java's SessionTracerImpl.uptime stopwatch pattern (one anchor, state == Ready as the label).
session.open_latencies=msSince(startTime)(unchanged).session.durations=msSince(startTime)always (was branching on openedAt);readylabel sourced fromsnap.opened. Matches Java'sSessionTracerImpl.onClosewhich usesuptime.elapsed()uniformly.session.uptime=msSince(startTime)(was openedAt);readylabel sourced fromsnap.opened.- Public
Session.OpenedAt()→Session.StartedAt()(lock-free — startTime is immutable afternewSessionTracer). The two follow-up-PR consumers (handleCloseevent log andInvokeslow-vRPC event) will switch to age-from-start; that adds sub-100ms of open-handshake time to the displayed age, negligible for debug post-mortems.
Semantic shift worth calling out: session.durations for a ready session now includes the open-handshake window (Java records the same value). Session-tracer bucket alignment from the earlier commit still holds — SessionDurationBounds mirrors Java's ClientSessionDuration.BUCKETS_MS so bucket boundaries don't shift.
Addresses mutianf review on googleapis#20117: - session.durations, session.uptime: adopt java-bigtable ClientSessionDuration.BUCKETS_MS ([0] + geometric doubling from 1ms to 20 minutes). Same layout java uses for both metrics so cross- language dashboards can graph them side-by-side without re-bucketing. - session.open_latencies: adopt java-bigtable ClientSessionOpenLatency.BUCKETS_MS (fine sub-3ms linear + 3-9ms linear + geometric doubling to 5 min). Open latency lives in a different range than session lifetime, so it gets its own bounds. Also clarify ErrUnavailableSessionError's doc-comment on the reserved `rpc_id == 0` convention (Java parity: SessionImpl.nextRpcId starts at 1, so no live vRPC ever uses id 0; server uses id 0 in ErrorResponse to mean "this fatal applies to the session, not to a specific vRPC"). The handleErrorResponse split lives in the follow-up PR. And drop the now-noisy "matches java-bigtable's MetricRegistry layout" phrase from transportLatencies's doc.
| // — see session_debug.go. Embedded so bare field access (s.okRpcs, | ||
| // s.recordEvent, s.tracer, …) still resolves. | ||
| type Session struct { | ||
| // nextRPCID is mutated exclusively via atomic ops. |
There was a problem hiding this comment.
say unique within this Session.
There was a problem hiding this comment.
Done in 5b9e725 — expanded to "nextRPCID is the monotonic vRPC id counter for this Session — unique within this Session only (a different Session starts fresh at 0). Mutated exclusively via atomic ops."
| // nextRPCID is mutated exclusively via atomic ops. | ||
| nextRPCID atomic.Int64 | ||
|
|
||
| sendMu sync.Mutex |
There was a problem hiding this comment.
Done in 5b9e725 — added "sendMu protects Send() — serializes writes to the bidi stream so concurrent producers can't interleave partial gRPC frames."
| // per-session lifetimes into the ring buffer. Zero for | ||
| // test-constructed handles that never went through OnActive — code | ||
| // paths that consume this must handle the zero-time case. | ||
| createdAt time.Time |
There was a problem hiding this comment.
We use to measure the lifetime of the Session.
We will invoke this on OnClosing().
…anchor Addresses mutianf ask on PR googleapis#20117 ("just keep one variable"): the tracer was carrying two timestamps (startTime, openedAt) but Java's SessionTracerImpl only needs one — its `uptime` stopwatch is started in onStart() and drives session.open_latencies, session.durations, and session.uptime uniformly, with `state == State.Ready` as the boolean "ready" label. Changes: - session_tracer.go: drop `openedAt time.Time` and its lock-guarded `openedAtSnapshot()` accessor. Add `opened bool` (protected by the same mu) as the ready flag, flipped in recordOpen. recordClose and sampleUptime now always anchor at msSince(startTime), matching Java's uptime.elapsed() semantics. `ready` label sourced from snap.opened. - session_debug.go: rename exported `Session.OpenedAt()` → `Session.StartedAt()` (lock-free — startTime is immutable after newSessionTracer). - session_lifecycle.go, session_pool.go: two callers updated. Their event fields (`age` in the close event, `SessionAge` in the slow-vRPC event) now count from construction instead of Ready-transition — for a healthy session that's a sub-100ms shift, and for a pre-open death the field is now populated instead of zero-valued. - CLIENT_SIDE_METRICS_SPEC.md: refresh invariant #3's metric roster for session.durations and session.uptime; note the Java-parity rationale. Semantic shift worth flagging: `session.durations` for a ready session now includes the open-handshake window (~sub-100ms). Java records the same value. `session.uptime` is unaffected in practice (pool's sampleActiveUptimes iterator already gated on StateReady). session-reviewer (behavioral, 4 specs) and session-component-review (boundaries, SESSION_COMPONENT_SPEC Part B/C) both came back CLEAN after the spec update. Full transport suite passes.
…anchor Addresses mutianf review on googleapis#20117 ("just keep one variable"): tracer was carrying two timestamps (startTime, openedAt) but Java's SessionTracerImpl only needs one — its `uptime` stopwatch is started in onStart() and drives session.open_latencies, session.durations, and session.uptime uniformly, with `state == State.Ready` as the boolean "ready" label. - session_tracer.go: drop `openedAt time.Time` and the lock-guarded `openedAtSnapshot()` accessor. Add `opened bool` (protected by the same mu) as the ready flag, flipped in recordOpen. recordClose and sampleUptime now always anchor at msSince(startTime), matching Java's uptime.elapsed() semantics. `ready` label sourced from snap.opened. - session_debug.go: rename exported `Session.OpenedAt()` → `Session.StartedAt()` (lock-free — startTime is immutable after newSessionTracer). - session_tracer_test.go: rewire assertions from `openedAt.IsZero()` to `opened bool`, rename `TestSessionTracer_RecordOpen_StampsOpenedAt` → `TestSessionTracer_RecordOpen_FlipsOpenedFlag`. Semantic shift worth flagging: session.durations for a ready session now includes the open-handshake window (~sub-100ms). Java records the same value. session.uptime is unaffected in practice. The identical code change on feat/bigtable-sessionz-debug (commit aef7d79) already passed session-reviewer (behavioral, 4 specs) and session-component-review (boundaries, SESSION_COMPONENT_SPEC Part B/C). Full transport suite passes here.
Addresses two self-review notes on PR googleapis#20117: - nextRPCID: note that IDs are unique WITHIN a single Session (a fresh Session starts back at 0). The "mutated via atomic ops" note stays. - sendMu: was undocumented; now says it protects Send() by serializing writes to the bidi stream so concurrent producers can't interleave partial gRPC frames. No behavioral change.
Follow-up to mutianf's review on googleapis#20117 and self-review: the previous doc inlined the wire-protocol dispatch detail (rpc_id == 0 convention + Java parity note) at the sentinel declaration. That's the wrong home — callers use errors.Is to detect this cause; they don't care how the server signaled it. The rpc_id == 0 split belongs on handleErrorResponse in the follow-up PR that carries the routing code. Trimmed to: "The session is torn down and any in-flight vRPCs are cancelled with this cause (via cancelActiveRPCs, tagged StateTransportFailure)." No behavioral change.
…d-ref Self-review comment on googleapis#20117 ("remove this line"): the Stream.Context() doc referenced sessionz and tcpz — z-page names that don't exist in this PR and only land in the sessionz-debug branch. Same anti-pattern as the ErrUnavailableSessionError trim in 80b9565. Trimmed to just the technical detail readers of this PR need: "Context is the stream's context; after Header() returns, remote peer info is available via peer.FromContext." No behavioral change; Stream.Context() itself stays (used downstream in session_lifecycle.go for PeerInfo capture).
| Recv() (*spb.SessionResponse, error) | ||
| Header() (metadata.MD, error) | ||
| // Context is the stream's context. After Header() returns, peer info | ||
| // (remote TCP addr) is available via peer.FromContext — sessionz uses |
There was a problem hiding this comment.
Done in 608aea7 — trimmed the doc-comment to "Context is the stream's context; after Header() returns, remote peer info is available via peer.FromContext." Dropped the sessionz/tcpz forward-reference (same anti-pattern as ErrUnavailableSessionError in 80b9565 — z-page names don't belong on a base interface in this PR since the z-pages live in the sessionz-debug branch). Stream.Context() itself stays — used downstream in session_lifecycle.go for PeerInfo capture.
Addresses mutianf review on googleapis#20117 ("why is last activity only updated when decrement outstanding?"): the field semantically means "when did this session last become idle" — the anchor pool scale-down / stuck- sweep use to age-out under-utilized sessions, NOT "when was it last touched." Renaming makes the semantic obvious at the callsite and removes the ambiguity that prompted the review. - Field: lastActivity → lastIdleTime - Method: GetLastActivity → GetLastIdleTime - Doc-comment on the field spells out the DecOutstanding-only stamp contract and why IncOutstanding does NOT touch it (Picks() already tracks checkout frequency; this metric is about idle-time). - Tests renamed to match. No behavioral change.
5ab0f29 to
4190a68
Compare
sushanb
left a comment
There was a problem hiding this comment.
Addressed the three "put this in session_state file" comments in 8688941 — moved transitionTo, isState, notState to session_state.go and their four tests to session_state_test.go. s.lastStateChangeNano stays as a field on Session so transitionTo can stamp it without pulling in the (follow-up) debug surface.
96b566a to
5c3709e
Compare
| const multiPlexingLimit = 1 | ||
|
|
||
| const ( | ||
| defaultHeartbeatInterval = 10 * time.Second |
There was a problem hiding this comment.
is this the heartbeat to tell if the session is still alive? 10 seconds is too long, let's set it to 100ms.
There was a problem hiding this comment.
Done in ee9ddc5 — dropped to 100ms per your ask. Also expanded the doc to spell out that this is the fallback cadence used only until the server sends SessionParametersResponse; once it arrives, handleSessionParameters overwrites heartbeatIntervalNano with the negotiated KeepAlive value (session_lifecycle.go:326 in the follow-up PR). Safe at 100ms because the watchdog is idle-gated — only enforced while a vRPC is in flight (session_lifecycle.go:557), so it can't tear down handshake-window sessions with nothing in flight.
| defaultHeartbeatInterval = 10 * time.Second | ||
| // initialHeartbeatGrace covers the OpenSession handshake; replaced by | ||
| // SessionParametersResponse. | ||
| initialHeartbeatGrace = 30 * time.Minute |
There was a problem hiding this comment.
what's the difference between this and defaultHeartbeatInterval?
There was a problem hiding this comment.
initialHeartbeatGrace is the initial value of nextHeartbeatDeadlineNano — set once at NewSession (session.go:184) and then extended to 3*heartbeatIntervalNano on every inbound/outbound frame (via resetHeartbeatDeadline, in the follow-up lifecycle PR). defaultHeartbeatInterval is the initial value of heartbeatIntervalNano — the fallback cadence used until handleSessionParameters overwrites it with the server-provided KeepAlive. Aligned both to 100ms in ee9ddc5 so a session that never receives SessionParameters trips within one interval instead of hanging on the wide bootstrap grace. Also expanded the doc block on both to spell out the distinction.
| return prev, false | ||
| } | ||
| if s.state.CompareAndSwap(int32(prev), int32(to)) { | ||
| s.lastStateChangeNano.Store(time.Now().UnixNano()) |
There was a problem hiding this comment.
I think there's a race condition updating this timestamp. but probably not critical? is this timestamp just used for observibility purpose?
There was a problem hiding this comment.
Yes — purely observability. Read only for the sessionz per-state dwell-time display (session_snapshot.go in the follow-up debug PR). The window between the CAS-success and the Store is nanoseconds; a racing transition can overwrite with a later timestamp, so the observed dwell is short by that tiny delta. Not worth packing state + timestamp into a single word for a display value.
Both defaultHeartbeatInterval and initialHeartbeatGrace shrink from 10s/30min to 100ms/100ms — the values only matter until the server sends SessionParametersResponse (which overwrites heartbeatIntervalNano and resets the deadline to 3×interval). After that first server frame, the constants are dead weight. Keeping them small removes a footgun: if SessionParameters never lands (server bug / stalled stream) the session used to sit with a 30-minute deadline before ever tripping MissedHeartbeat. The watchdog is idle- gated (only fires while a vRPC is in flight), so a short bootstrap grace is safe — no VRPC can be racing it during the handshake window. Ports the same constants change already committed on the PR-googleapis#20117 branch (bigtable-session-struct) at ee9ddc5 in response to mutianf's review.
Session is the per-stream lifecycle owner for a Bigtable Session: - Stream interface, SessionHooks (OnStart/OnActive/OnClosing/OnClose), vrpcResult (three-way tagged union), vrpcImpl. - Session struct: atomic state, lastStateChangeNano, sendMu, slotMu- guarded (activeRPC, currentCancel), heartbeat deadline atomics, quiescent channel, PeerInfo/RefreshConfig atomic pointers. - transitionTo(to, predicate) CAS-plus-retry loop, isState/notState predicate builders — placed in session_state.go alongside the State enum. - signalQuiescent, LogName/State/PeerInfo/AfeID/RefreshConfig accessors. - sessionErr + unavailable(cause, format, args...) wrapping codes.Unavailable with a sentinel cause so status.Code and errors.Is both work. - AfeID type (int64, exported for go vet). - Sentinel errors: ErrSessionNotActive, ErrUnavailableHeartBeatMissed, ErrUnavailableGoAway, ErrUnavailableSessionError. Scoped to session.go + session_state.go additions plus their tests. Observability surface (sessionDebug, sessionTracer) and SessionHandle land in follow-up PRs. lastStateChangeNano lives directly on Session so transitionTo can stamp it standalone. Tests: 14 covering defaults, CAS transitions (happy/rejected/ concurrent), predicate builders, quiescent channel, AfeID resolution, RefreshConfig accessor, vrpcResult union, unavailable() wrapping, and SessionHooks dispatch.
…eatGrace to 100ms Per review feedback, tighten both bootstrap heartbeat constants from 10s/30min to 100ms/100ms. Kept in lock-step so a session that never receives SessionParametersResponse trips within one interval instead of hanging on the wide bootstrap grace. Values are only used until the server sends SessionParametersResponse, which overwrites heartbeatIntervalNano with the negotiated cadence and the deadline shifts to 3×interval on every inbound/outbound frame (via resetHeartbeatDeadline in the follow-up lifecycle PR). Also expanded the doc comment on both constants to spell out the handshake window vs steady-state distinction, addressing the paired "what's the difference between these two?" thread.
Rebased onto upstream/main to pull in afe_snapshot.go (from PR googleapis#20204, merged 2026-07-23), which defines the AfeID type at the package level. The Session struct commit still declared its own AfeID inside session.go, causing a redeclaration error surfaced by CI. Removed the duplicate `type AfeID int64` from session.go and the doc comment; kept `Session.AfeID()` since AfeID is package-scoped and now resolves to afe_snapshot.go's declaration.
ee9ddc5 to
1cc83ef
Compare
…hods) Introduces sessionDebug — a struct bundling every observability concern for a Session: per-session counters (okRpcs/errorRpcs/msgs*), event ring buffer, backend-latency histogram, per-cluster response counts, session tracer, debug logger, per-channel index, and close-reason attribution. Embedded into Session so existing bare-field access (s.tracer, s.okRpcs, s.recordEvent, ...) will compile once the vRPC and lifecycle follow-ups land. `lastStateChangeNano` moves off Session onto sessionDebug (co-located with the rest of the observability plumbing) — transitionTo's swap-stamp keeps working via embedding. NewSession wires the embed via s.sessionDebug.init(sessionType), which sets the tracer + channelIndex + lastStateChangeNano defaults. No behavior change on any merged code path — every field added by this PR has zero writers on main today. The Read-side methods (OkRpcs/ErrorRpcs/HasOkRpcs/HasErrorRpcs/MsgsSent/MsgsRecv/CloseReason/ Retries/StartedAt/RemoteAddr/ChannelIndex) all return zero-values until the follow-up session_vrpc.go PR wires the write sites. Prereq: PR googleapis#20117 (merged) — needs the Session struct + State machine. Follow-ups (in dependency order): session_vrpc.go, session_lifecycle.go.
…hods) (#20211) ## Summary Introduces \`sessionDebug\` — a struct bundling the observability surface for a Session — and embeds it into \`Session\`. This is the first of three stacked PRs that layer per-Session behavior on top of the state machine from #20117. ### What lands **New file: \`session_debug.go\`** (~360 LOC) - Per-session atomic counters: \`okRpcs\`, \`errorRpcs\`, \`msgsSent\`, \`msgsRecv\`, per-frame-type breakdowns (\`msgsSentByType\`/\`msgsRecvByType\`), \`retries\`. - Debug event ring buffer (cap 64) + \`recordEvent\` / \`snapshotEvents\`. - Backend-latency histogram (256-sample ring) + \`recordLatency\` / \`snapshotLatencies\` + \`percentile\` helper. - Per-cluster response counts via \`sync.Map\` + \`recordCluster\` / \`snapshotClusters\`. - Close-reason attribution: \`setCloseReason\` / \`CloseReason\` (once-only stamp), \`poolCloseRecorded\` gate. - \`SessionTracer\` and \`log.Logger\` handles. - \`ChannelIndex\` + \`RemoteAddr\` accessors, \`peerInfoSummary\` helper. - \`SampleUptime\` / \`RecordTransportOverhead\` wrappers that delegate to the tracer. - Two \`SessionOption\` factories: \`WithSessionLogger\`, \`WithSessionPoolName\`. **Edits to \`session.go\`:** - \`lastStateChangeNano\` moves off \`Session\` onto \`sessionDebug\` (co-located with the rest of the observability plumbing). \`transitionTo\`'s swap-stamp continues to work through Go embedding. - \`sessionDebug\` embedded at the tail of the \`Session\` struct. - \`NewSession\` calls \`s.sessionDebug.init(sessionType)\` which sets the tracer, resets \`channelIndex\` to -1, and stamps \`lastStateChangeNano\`. ### Behavior on merged code paths **Zero.** Every field added by this PR has no writers on \`main\` today — the read-side accessors (\`HasOkRpcs\`, \`ErrorRpcs\`, \`MsgsSent\`, \`CloseReason\`, \`Retries\`, …) all return zero-values until the follow-up PRs wire the write sites. ### Stack 1. **This PR** — Session debug surface (\`session_debug.go\` + Session struct embed) 2. **Next** — \`session_vrpc.go\` (Session.Invoke + handleVRPC{Response,ErrorResponse} + cancelActiveRPCs; wires most of the write sites) 3. **Last** — \`session_lifecycle.go\` (Start/Close/ForceClose/readLoop/heartBeatLoop; wires the rest) ## Test plan - [x] \`go build ./internal/transport/\` passes - [x] \`go vet ./internal/transport/\` clean - [x] \`go test ./internal/transport/ -count=1 -short -timeout 90s\` passes (all existing tests; no new tests in this PR — \`session_debug_test.go\` lands with the vRPC follow-up when there are actual write sites to assert on)
🤖 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>
Summary
Adds the Session struct and its atomic state machine. Scope trimmed to just
session.go+session_test.goper reviewer feedback; observability (sessionDebug/sessionTracer) andSessionHandleland in follow-up PRs.session.go(~380 LOC) — Session struct + lifecycle types:Streaminterface,SessionHooks(withOnStart/OnActive/OnClosing/OnClose),vrpcResult(three-way tagged union),vrpcImpl.Sessionstruct with atomicstate,activeRPC,peerInfo,refreshConfig, heartbeat deadline fields, and quiescent-once channel.transitionTo(to, predicate)CAS-plus-retry loop for state transitions;isState/notStatepredicate builders.signalQuiescent,LogName,State,PeerInfo,AfeID,RefreshConfigaccessors.sessionErr+unavailable(cause, format, args...)— wrapscodes.Unavailablewith a sentinel cause sostatus.Code(err)anderrors.Is(err, sentinel)both work.AfeIDtype (exported per go vet).ErrSessionNotActive,ErrUnavailableHeartBeatMissed,ErrUnavailableGoAway,ErrUnavailableSessionError.lastStateChangeNanoinlined directly on Session sotransitionTocan stamp it without depending on the (follow-up) debug surface.session_test.go(~320 LOC) — 14 tests covering: defaults, CAS transitions (happy + rejected + concurrent), predicate builders, quiescent channel, AfeID resolution, RefreshConfig accessor, vrpcResult union, unavailable() wrapping, and SessionHooks dispatch.What was dropped from the prior revision
session_debug.go(+ test),session_tracer.go(+ test),session_handle.go(+ test) all move to follow-up PRs. Two things kept locally:AfeIDtype declaration stays here (referenced by the follow-up picker + the sessionDebug type).lastStateChangeNanoinlined as a direct field onSessionrather than embedded viasessionDebug, sotransitionTocan stamp it standalone.Test plan
go test ./bigtable/internal/transport/ -run 'TestSession|TestVrpc|TestUnavailable|TestIsState|TestNewSession' -count=1 -short→ 14/14 pass locally.go build ./bigtable/internal/transport/clean.go vet ./bigtable/internal/transport/clean.golint bigtable/internal/transport/session.go bigtable/internal/transport/session_test.goclean.