feat(bigtable): add Session primitives (AttemptOutcome, vRPC ctx, msgtype) - #20116
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces several internal utilities and tracing mechanisms for virtual RPCs (vRPC) in the Bigtable client, including error classification, a debug tracer with OTel metrics, session message type classification, and context metadata helpers. The review feedback correctly identifies a critical data race on the package-level debugTagCounter variable in debug_tracer.go due to concurrent reads and writes without synchronization. To resolve this, it is recommended to use atomic.Value to safely store and load the OTel counter.
| // debugTagCounter is the OTel Int64Counter registered inside | ||
| // InitializeSessionMetrics. Nil until initialization runs (or if | ||
| // initialization was called with a nil meter provider) — every | ||
| // emission path nil-checks it, so the tracer is safe to call before | ||
| // InitializeSessionMetrics or in tests that don't wire OTel at all. | ||
| debugTagCounter metric.Int64Counter |
There was a problem hiding this comment.
The package-level variable debugTagCounter is read concurrently in recordDebugTagAt and written in registerDebugTagCounter without any synchronization. Since interface values in Go are not word-sized and assignments to them are not atomic, this introduces a data race that can lead to panics or undefined behavior under concurrent execution.
To resolve this, use atomic.Value to store and load the metric.Int64Counter safely.
| // debugTagCounter is the OTel Int64Counter registered inside | |
| // InitializeSessionMetrics. Nil until initialization runs (or if | |
| // initialization was called with a nil meter provider) — every | |
| // emission path nil-checks it, so the tracer is safe to call before | |
| // InitializeSessionMetrics or in tests that don't wire OTel at all. | |
| debugTagCounter metric.Int64Counter | |
| // debugTagCounter is the OTel Int64Counter registered inside | |
| // InitializeSessionMetrics. Nil until initialization runs (or if | |
| // initialization was called with a nil meter provider) — every | |
| // emission path nil-checks it, so the tracer is safe to call before | |
| // InitializeSessionMetrics or in tests that don't wire OTel at all. | |
| debugTagCounter atomic.Value |
109ad40 to
e93047b
Compare
Renames transportTypeName -> TransportTypeName in
bigtable/internal/metrics/util.go and updates its one caller in
tracer.go. Zero behavior change.
Motivation: the upcoming Session core PR needs the same
PeerInfo.TransportType -> short-label mapping ("cloudpath",
"session_directpath", ...) in the transport package's session tracer,
session_debug, and session_lifecycle. Exporting the existing helper is
strictly cheaper than duplicating the 15-line switch in a second
package. No import cycle (metrics does not import transport, and
transport does not currently import metrics).
Part 1/3 of the Session core PR split.
…type) Adds the standalone types that the Session struct and its lifecycle / vRPC / debug halves all depend on. Each file is self-contained (no cross-references) so this PR compiles and passes tests on its own. - attempt_outcome.go — AttemptState (StateUncommitted / StateTransportFailure / StateServerResult), tagErr, TagErr, ClassifyErr. Models Java's VRpc.VRpcResult.State so the RetryingVRpc interceptor (later PR) can classify errors the same way as java-bigtable. - vrpc.go — ctx-metadata helpers (WithVRpcMetadata, WithAttempt, VRpcAttempt, VRpcMethod, WithPrevAttemptErr, PrevAttemptErr). Session Invoke reads these from its ctx. - session_msgtype.go — reqMsgType / respMsgType enums + classifyReq / classifyResp helpers. Used by the debug surface + tracer to bucket Session request/response types. Part 3a/3c of the Session core sub-split (a follow-up to the original Session core PR googleapis#20112, which is being reshaped into three thinner PRs). Stacks on googleapis#20115 (metrics.TransportTypeName export) and googleapis#20114 (debug tag counter); each of those can merge in any order.
e93047b to
c3a7f28
Compare
| func (t reqMsgType) String() string { | ||
| switch t { | ||
| case reqMsgOpenSession: | ||
| return "OpenSession" |
There was a problem hiding this comment.
nit: should we rename this OpenSessionRequest and the resp "OpenSessionResponse" ?
There was a problem hiding this comment.
Done in 7cd7678 — reqMsgOpenSession now stringifies to OpenSessionRequest and respMsgOpenSession to OpenSessionResponse. Left the other labels alone since they're unambiguous within a single oneof group. Also snuck in VirtualRpc → VirtualRPC on the two constants to unblock golint (the vet job was failing on those).
…alRpc → VirtualRPC Address review nits on googleapis#20116: - reqMsgOpenSession / respMsgOpenSession stringify to "OpenSessionRequest" / "OpenSessionResponse" so a reader of a debug page or log line can tell request from response at a glance (per mutianf). - reqMsgVirtualRpc → reqMsgVirtualRPC and respMsgVirtualRpc → respMsgVirtualRPC to satisfy golint's initialism rule and unblock the vet check.
…ine (#20185) ## Summary Builds on the vRPC primitives merged in #20116 by adding the two building blocks that consumers need to actually run interceptor pipelines with retries: - **`ChainInterceptors`** (`internal/transport/chain.go`) — composes multiple `Interceptor`s around a base `Handler`. First interceptor in the list is outermost, so it sees setup before and teardown after the rest. - **`RetryingVRpc`** (`internal/transport/retrying.go`) — an `Interceptor` that retries failed virtual RPCs with exponential backoff. Default classification is Java-parity: - `StateUncommitted` → always retry (server never saw the frame). - `StateTransportFailure` → retry only when `Idempotent` is true (server may have applied). - `StateServerResult` → retry only if the server attached `errdetails.RetryInfo`; a bare server-explicit error (even `Unavailable` / `Aborted` / `DeadlineExceeded`) does NOT retry without explicit server go-ahead. - Callers with bespoke policies can set `ShouldRetry` to override the default entirely. Per-attempt tracing goes through `VRpcTracer`; the previous attempt's error is tagged onto the next attempt's context via `WithPrevAttemptErr` for downstream metrics/logging. ## Test plan - [x] `go build ./internal/transport/...` - [x] `go vet ./internal/transport/...` - [x] `go test ./internal/transport/... -run 'Retrying|Chained|Interceptor' -count=1 -race -timeout=90s` — 12 new tests pass: - `TestChainedInterceptors_Success` — interceptor call ordering (outer→inner→base→inner→outer) - `TestRetryingVRpc_SuccessOnRetry` — recovers on attempt 3, tracer sees all 3 starts/completes - `TestRetryingVRpc_NonRetryableError` — `InvalidArgument` aborts after 1 attempt - `TestRetryingVRpc_HonorServerRetryDelay` — server `RetryInfo` overrides client backoff - `TestRetryingVRpc_MaxAttemptsExceeded` — stops at `MaxAttempts` - `TestRetryingVRpc_NonGrpcError` — raw Go errors are non-retryable - `TestRetryingVRpc_UncommittedAlwaysRetries` — `StateUncommitted` retries even when `Idempotent=false` - `TestRetryingVRpc_TransportFailureIdempotent` — `StateTransportFailure` retries with `Idempotent=true` - `TestRetryingVRpc_TransportFailureNonIdempotentNoRetry` — no retry when `Idempotent=false` - `TestRetryingVRpc_ServerResultNotRetriedByDefault` — bare `ServerResult` never retries (5 codes) - `TestRetryingVRpc_ServerDeadlineExceededNoRetryByDefault` — server `DEADLINE_EXCEEDED` alone is not retried - `TestRetryingVRpc_ServerDeadlineExceededRetriesWithRetryInfo` — `DEADLINE_EXCEEDED` + `RetryInfo` is retried - [x] `goimports -d` clean - [x] `golint` clean
🤖 I have created a release *beep* *boop* --- ## [1.51.0](bigtable/v1.50.0...bigtable/v1.51.0) (2026-07-23) ### Features * **bigtable:** Add ChainInterceptors and RetryingVRpc for vRPC pipeline ([#20185](#20185)) ([c7a832a](c7a832a)) * **bigtable:** Add ClientConfigurationManager ([#19986](#19986)) ([3a8f927](3a8f927)) * **bigtable:** Add debug tag counter (recordDebugTag / assertDebugTag) ([#20114](#20114)) ([3c97590](3c97590)) * **bigtable:** Add lazyPool helper for on-demand session pool opening ([#20182](#20182)) ([f6ae3fb](f6ae3fb)) * **bigtable:** Add PeakEwma continuous time-decay latency tracker ([#20187](#20187)) ([9d124ef](9d124ef)) * **bigtable:** Add PoolSizer for server-driven session pool capacity ([#20189](#20189)) ([57ebbeb](57ebbeb)) * **bigtable:** Add session package with SessionClient + SessionTableAPI interfaces ([#20180](#20180)) ([4b82fd2](4b82fd2)) * **bigtable:** Add Session primitives (AttemptOutcome, vRPC ctx, msgtype) ([#20116](#20116)) ([e1011e2](e1011e2)) * **bigtable:** Add Session state enum ([#19981](#19981)) ([0748972](0748972)) * **bigtable:** Add SessionThrottler / AdaptiveSessionThrottler for OpenSession pacing ([#20184](#20184)) ([02e3c6d](02e3c6d)) * **bigtable:** Add SessionThrottler / AdaptiveSessionThrottler for OpenSession pacing ([#20184](#20184)) ([29be83e](29be83e)) * **bigtable:** Add sessionTracer for per-Session lifecycle + vRPC metrics ([#20190](#20190)) ([a466345](a466345)) * **bigtable:** Enable new auth library and JWT for instance admin client ([#20013](#20013)) ([21c4a44](21c4a44)) * **bigtable:** Modularize channel priming behind a ChannelPrimer interface ([#20027](#20027)) ([5214ab7](5214ab7)) * **bigtable:** Modularize Direct Access compatibility check ([#19987](#19987)) ([a25e93d](a25e93d)) * **o11y:** Regenerate clients for LRO tracing ([#20107](#20107)) ([779074e](779074e)) ### Bug Fixes * **bigtable:** Default cluster/zone in toOtelMetricAttrs to avoid Monitoring reject ([#20178](#20178)) ([14493f4](14493f4)) * **bigtable:** Eliminate stats-handler MD race in internal/metrics tracer ([#20158](#20158)) ([c387066](c387066)) --- 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 standalone types that the Session struct and its lifecycle / vRPC / debug halves all depend on. Each file is self-contained (no cross-references) so this PR compiles and passes tests on its own.
attempt_outcome.go—AttemptState(StateUncommitted/StateTransportFailure/StateServerResult),tagErr,TagErr,ClassifyErr. Models Java'sVRpc.VRpcResult.Stateso theRetryingVRpcinterceptor (later PR) can classify errors the same way as java-bigtable.vrpc.go— ctx-metadata helpers (WithVRpcMetadata,WithAttempt,VRpcAttempt,VRpcMethod,WithPrevAttemptErr,PrevAttemptErr).Session.Invokereads these from its ctx.session_msgtype.go—reqMsgType/respMsgTypeenums +classifyReq/classifyResphelpers. Used by the debug surface + tracer to bucket Session request/response types.Part 3a of 3 in the Session core sub-split (a follow-up to the original Session core PR #20112, which is being reshaped into three thinner PRs). Stacks on #20115 (
metrics.TransportTypeNameexport) and #20114 (debug tag counter); each of those can merge in any order. Sub-split order:Test plan
go build ./bigtable/...go test ./bigtable/internal/transport/ -count=1 -short— passes (4.0s)gofmt -l bigtable/internal/transport/— clean