Skip to content

feat(bigtable): add Session primitives (AttemptOutcome, vRPC ctx, msgtype) - #20116

Merged
sushanb merged 3 commits into
googleapis:mainfrom
sushanb:bigtable-session-primitives
Jul 10, 2026
Merged

feat(bigtable): add Session primitives (AttemptOutcome, vRPC ctx, msgtype)#20116
sushanb merged 3 commits into
googleapis:mainfrom
sushanb:bigtable-session-primitives

Conversation

@sushanb

@sushanb sushanb commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

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.goAttemptState (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.goreqMsgType / respMsgType enums + classifyReq / classifyResp helpers. 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.TransportTypeName export) and #20114 (debug tag counter); each of those can merge in any order. Sub-split order:

  1. 3a — this PR: Session primitives (~337 LOC).
  2. 3b — TBD: Session struct + tracer + debug surface + picker (~1.1k LOC).
  3. 3c — reshaped [5/5] feat(bigtable): add Session lifecycle, vRPC, and tests #20112: Session lifecycle + vRPC + tests (~2k LOC).

Test plan

  • go build ./bigtable/...
  • go test ./bigtable/internal/transport/ -count=1 -short — passes (4.0s)
  • gofmt -l bigtable/internal/transport/ — clean
  • CI: presubmit

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

Comment on lines +163 to +168
// 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

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.

high

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.

Suggested change
// 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

Comment thread bigtable/internal/transport/debug_tracer.go Outdated
Comment thread bigtable/internal/transport/debug_tracer.go Outdated
@sushanb sushanb changed the title feat(bigtable): add Session primitives (AttemptOutcome, vRPC ctx, msgtype) [3/5] feat(bigtable): add Session primitives (AttemptOutcome, vRPC ctx, msgtype) Jul 9, 2026
@sushanb
sushanb force-pushed the bigtable-session-primitives branch 3 times, most recently from 109ad40 to e93047b Compare July 9, 2026 20:15
sushanb added 2 commits July 9, 2026 20:32
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.
@sushanb
sushanb force-pushed the bigtable-session-primitives branch from e93047b to c3a7f28 Compare July 9, 2026 20:32
@sushanb sushanb changed the title [3/5] feat(bigtable): add Session primitives (AttemptOutcome, vRPC ctx, msgtype) feat(bigtable): add Session primitives (AttemptOutcome, vRPC ctx, msgtype) Jul 9, 2026
func (t reqMsgType) String() string {
switch t {
case reqMsgOpenSession:
return "OpenSession"

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.

nit: should we rename this OpenSessionRequest and the resp "OpenSessionResponse" ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 7cd7678reqMsgOpenSession 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 VirtualRpcVirtualRPC 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.
@sushanb
sushanb merged commit e1011e2 into googleapis:main Jul 10, 2026
19 checks passed
sushanb added a commit that referenced this pull request Jul 22, 2026
…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
hongalex pushed a commit that referenced this pull request Jul 23, 2026
🤖 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>
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