googleapis
diff --git a/‎bigtable/docs/specs/CLIENT_SIDE_METRICS_SPEC.md‎
Lines changed: 3 additions & 2 deletions b/‎bigtable/docs/specs/CLIENT_SIDE_METRICS_SPEC.md‎
Lines changed: 3 additions & 2 deletions
diff --git a/‎bigtable/docs/specs/SESSION_CLIENT_SPEC.md‎
Lines changed: 8 additions & 8 deletions b/‎bigtable/docs/specs/SESSION_CLIENT_SPEC.md‎
Lines changed: 8 additions & 8 deletions
diff --git a/‎bigtable/docs/specs/SESSION_COMPONENT_SPEC.md‎
Lines changed: 1 addition & 1 deletion b/‎bigtable/docs/specs/SESSION_COMPONENT_SPEC.md‎
Lines changed: 1 addition & 1 deletion
diff --git a/‎bigtable/internal/metrics/tracer.go‎
Lines changed: 15 additions & 5 deletions b/‎bigtable/internal/metrics/tracer.go‎
Lines changed: 15 additions & 5 deletions
@@ -55,8 +55,9 @@ The session path bypasses gRPC's per-call stats handler (one bidi stream carries
 
 **`server_latencies`** (`MetricNameServerLatencies`).
 - **Classic source:** `ExtractServerLatency(headers, trailers)` reads `x-goog-cbt-*` server-timing metadata; a t4t7 fallback tracker fills in when the header is absent (`internal/metrics/tracer.go:822-828`).
-- **Session source:** `result.Stats.BackendLatency` — a typed `google.protobuf.Duration` on `SessionRequestStats` (part of the vRPC response frame). `stampAttempt` converts and stamps: `att.SetServerLatency(ConvertToMs(result.Stats.GetBackendLatency().AsDuration()))` (`internal/session/table.go:245-247`).
-- **Nil-guard:** stamp is skipped when `Stats == nil` OR `Stats.BackendLatency == nil` — server MAY omit either (typically on error frames). The session path has NO t4t7-equivalent fallback; if a fallback becomes necessary, add it here rather than on the classic side.
+- **Session source:** `stampAttempt` on the session path (`internal/session/table.go`) does NOT stamp `server_latencies` — the field is not set. `result.Stats.BackendLatency` (a typed `google.protobuf.Duration` on `SessionRequestStats`) captures server-side compute time, but it is NOT wire-comparable to classic's server-timing-header value — session-path AFE overhead is measured separately via `transport_latencies`. Populating `server_latencies` from BackendLatency would produce a wrong-shape sample that dashboards cross-comparing session/classic would misread.
+- **connectivity_error_count interlock:** because session-path `serverLatency` is never stamped (default 0, `serverLatencyErr` nil), the `connectivity_error_count` classifier at `internal/metrics/tracer.go` MUST additionally check `transportType != ""` (populated by `stampAttempt` from `result.PeerInfo`). PeerInfo presence proves the `OpenSessionResponse` arrived, i.e., we reached a server; without this prong every session attempt would misclassify. See the tracer's connectivity_error block for the three-prong OR gate (PeerInfo || server-timing header || location header).
+- **Classic-path side-effect (intended):** the same three-prong OR runs on classic attempts. A classic attempt with sideband PeerInfo but no server-timing header now correctly counts as "reached server" instead of "connectivity error" — the sideband is definitive proof the server responded. Any tests that pinned the old two-prong classifier need updating.
 
 **`transport_type` / `transport_region` / `transport_zone` / `transport_subzone`** (per-attempt labels on `attempt_latencies2`).
 - Fully covered by **invariant #1**. Recap: `stampAttempt` reads `result.PeerInfo` (a pointer to `Session.peerInfo`, set once by `handleOpenSession`) and calls `SetTransportType`/`SetTransportRegion`/`SetTransportZone`/`SetTransportSubZone` (`internal/session/table.go:248-253`). Same PeerInfo pointer on every attempt on session S — semantic difference from classic where `grpc.Peer` can vary per attempt.
 
@@ -16,17 +16,17 @@
 
 ### 1. One `Client` owns exactly one `SessionClient`; one `SessionClient` owns many session pools, all lazily created
 
-**`Client` → `SessionClient` is strictly 1:1.** `bigtable.Client` holds `sessionImpl session.SessionClient` as a single field (`client.go:53`). When `EnableSessionPool` is set on `ClientConfig`, `NewClientWithConfig` calls `session.NewSessionClient(ctx, project, instance, appProfile, metricsProvider, o...)` **exactly once** during construction and stores the returned handle (`client.go:224`). There is no per-request `SessionClient`; every session-eligible operation on that `Client` fans through the same `SessionClient` instance. Standalone `NewSessionClient` consumers likewise get one `SessionClient` per logical client — the type is not intended to be pooled or shared across logical clients.
+**`Client` → `SessionClient` is strictly 1:1.** `bigtable.Client` holds `sessionImpl session.Client` as a single field (`client.go:53`). When `EnableSessionPool` is set on `ClientConfig`, `NewClientWithConfig` calls `session.NewClient(ctx, project, instance, appProfile, metricsProvider, o...)` **exactly once** during construction and stores the returned handle (`client.go:224`). There is no per-request `SessionClient`; every session-eligible operation on that `Client` fans through the same `SessionClient` instance. Standalone `NewSessionClient` consumers likewise get one `SessionClient` per logical client — the type is not intended to be pooled or shared across logical clients.
 
-**`SessionClient` → many `*SessionPoolImpl` is 1:N**, keyed by `{resource, direction}`. The `SessionClient` maintains an internal `pools map[string]*ManagedPool` guarded by `poolsMu`. Each `OpenSessionTable` / `OpenAuthorizedView` / `OpenMaterializedView` call registers up to two entries (read + write; MaterializedView is read-only, no write entry). Keys use fixed prefixes so different resource kinds cannot collide:
+**`SessionClient` → many `*SessionPoolImpl` is 1:N**, keyed by `{resource, direction}`. The `SessionClient` maintains an internal `sessionPools map[string]*managedSessionPool` guarded by `sessionPoolsMu`. Each `OpenTable` / `OpenAuthorizedView` / `OpenMaterializedView` call registers up to two entries (read + write; MaterializedView is read-only, no write entry). Keys use fixed prefixes so different resource kinds cannot collide:
 
 | Call | Key(s) written |
 |---|---|
-| `OpenSessionTable("t")` | `"table:<full-name>:read"`, `"table:<full-name>:write"` |
+| `OpenTable("t")` | `"table:<full-name>:read"`, `"table:<full-name>:write"` |
 | `OpenAuthorizedView("t","v")` | `"av:<table>:<view>:read"`, `"av:<table>:<view>:write"` |
 | `OpenMaterializedView("v")` | `"mv:<view>:read"` |
 
-Two `OpenSessionTable("t")` calls dedup on this key and return handles backed by the same underlying pool pair. The dedup is per-`SessionClient`; two different `SessionClient` instances would open independent pools even for the same resource — which is why the 1:1 `Client → SessionClient` rule matters (it's what guarantees one Bigtable `Client` never fans out to duplicate session infra for the same table).
+Two `OpenTable("t")` calls dedup on this key and return handles backed by the same underlying pool pair. The dedup is per-`SessionClient`; two different `SessionClient` instances would open independent pools even for the same resource — which is why the 1:1 `Client → SessionClient` rule matters (it's what guarantees one Bigtable `Client` never fans out to duplicate session infra for the same table).
 
 **`SessionClient` does NOT cache the returned `SessionTableApi`.** The consumer does — mixed-mode `bigtable.Client` holds `sessionTables map[string]session.SessionTableApi` guarded by `sessionTablesMu` (`client.go:59-64`). Rationale: `NewSessionClient` returns fresh `SessionTableApi` handles that share the underlying pool via the `SessionClient`-level pool cache; the per-`Client` cache is only for user-facing identity (same `Open*` call returns the same handle so callers can compare by pointer). Standalone `NewSessionClient` consumers MUST implement equivalent caching if they need handle identity — the pool dedup is only inside `SessionClient`.
 
@@ -38,7 +38,7 @@ Two `OpenSessionTable("t")` calls dedup on this key and return handles backed by
 **Consequences worth calling out.**
 - **Deferred failure surface:** marshal failures on session payloads surface on first `ReadRow`/`Apply`, not at `OpenTable`. Callers that expected eager failure (matching classic Bigtable's behavior) will see it later.
 - **Zero-traffic Clients open zero streams.** A `Client` that never issues a session-eligible call opens exactly zero `OpenSession` streams. Only `GetClientConfiguration` polls run from client construction time (they travel through the shared session channel pool — `SESSION_CLIENT_SPEC.md #2`).
-- **Config listener registration is deferred to pool open.** Per-pool `UpdateConfig` listeners (`SESSION_CLIENT_SPEC.md #3`) only register after the pool is actually opened by `getOrCreatePool` (`internal/session/client.go:536+`). Config polls run from `SessionClient` construction — the first poll's result is stored on the `ClientConfigurationManager` and used to seed the pool at open time.
+- **Config listener registration is deferred to pool open.** Per-pool `UpdateConfig` listeners (`SESSION_CLIENT_SPEC.md #3`) only register after the pool is actually opened by `getOrCreateSessionPool` (`internal/session/client.go:536+`). Config polls run from `SessionClient` construction — the first poll's result is stored on the `ClientConfigurationManager` and used to seed the pool at open time.
 
 ### 2. All Session Pools on a `SessionClient` share one channel pool
 
@@ -89,13 +89,13 @@ The wire-level `bigtable.v2.OpenSessionRequest` is deliberately resource-agnosti
 
 **`Permission` is baked into the pool key** (`SESSION_CLIENT_SPEC.md #1`). For Table and AuthorizedView, `Permission ∈ {PERMISSION_READ, PERMISSION_WRITE}` is encoded into the inner proto at pool construction time. This is *the* reason read and write pools are separate `*SessionPoolImpl` instances (`SESSION_POOL_SPEC.md #1`) — the `OpenSessionRequest.Payload` bytes for the read pool encode `Permission=READ` and the write pool encodes `Permission=WRITE`, so the server issues a session scoped to the right permission. MaterializedView has no `Permission` field on its inner proto and no write pool — attempting `MutateRow` on a MatView returns `ErrWriteNotSupported` client-side without any wire traffic (`SESSION_POOL_SPEC.md #1`).
 
-**Payload construction happens once per pool.** `createPoolForPayload` (`internal/session/client.go:481-531`) marshals the inner proto into `Payload` bytes at pool creation time and stashes the resulting `OpenSessionRequest` on the pool as its `handshake`. The same `handshake` is re-used **verbatim** for every session that pool opens over its lifetime — the payload bytes are immutable per pool. This is what keeps `OpenSession` cheap: no per-session proto encoding.
+**Payload construction happens once per pool.** `createSessionPoolForPayload` (`internal/session/client.go:481-531`) marshals the inner proto into `Payload` bytes at pool creation time and stashes the resulting `OpenSessionRequest` on the pool as its `handshake`. The same `handshake` is re-used **verbatim** for every session that pool opens over its lifetime — the payload bytes are immutable per pool. This is what keeps `OpenSession` cheap: no per-session proto encoding.
 
-**Routing metadata is derived from the same inner proto** via `SessionDescriptor.MetadataFn` (`internal/transport/session_descriptors.go:79-172`). The per-resource `MetadataFn` walks the inner proto to build `open_session.payload.*` header values, URL-escapes each, and joins them into the `x-goog-request-params` header alongside the `x-goog-request-params` resource prefix. This is what lets AFEs route the `OpenSession` stream without deserializing `Payload` — the resource identity is duplicated as headers so the transport tier can route on them directly. Headers and payload MUST agree; they are constructed from the same proto instance, so drift is not possible unless one code path bypasses `createPoolForPayload`.
+**Routing metadata is derived from the same inner proto** via `SessionDescriptor.MetadataFn` (`internal/transport/session_descriptors.go:79-172`). The per-resource `MetadataFn` walks the inner proto to build `open_session.payload.*` header values, URL-escapes each, and joins them into the `x-goog-request-params` header alongside the `x-goog-request-params` resource prefix. This is what lets AFEs route the `OpenSession` stream without deserializing `Payload` — the resource identity is duplicated as headers so the transport tier can route on them directly. Headers and payload MUST agree; they are constructed from the same proto instance, so drift is not possible unless one code path bypasses `createSessionPoolForPayload`.
 
 **`SessionType` (`session_descriptors.go:38-77`) is the compile-time discriminator** carried on `*SessionPoolImpl` and each `Session` so the internal transport tier can tag its debug output and log names — e.g., `"OpenTablePool-3 (sushanb) [READ]"` — without re-parsing the payload bytes. The enum is `{SessionTypeTable, SessionTypeAuthorizedView, SessionTypeMaterializedView}`, set when the pool is constructed from the descriptor. `SessionDescriptor.ProtoName()` returns the bare inner-proto name (`"OpenTable"` / `"OpenAuthorizedView"` / `"OpenMaterializedView"`) used to bake the human-readable pool identifier.
 
-**A marshal failure on the inner proto is fatal to that specific pool open**, not to the SessionClient. `createPoolForPayload` returns `fmt.Errorf("proto.Marshal session payload: %w", err)`; `lazyPool.get()` surfaces it to the caller and does NOT cache the failure (`SESSION_CLIENT_SPEC.md #1`). The `SessionClient` itself, its channel pool, and its config manager are untouched — other pools remain openable.
+**A marshal failure on the inner proto is fatal to that specific pool open**, not to the SessionClient. `createSessionPoolForPayload` returns `fmt.Errorf("proto.Marshal session payload: %w", err)`; `lazyPool.get()` surfaces it to the caller and does NOT cache the failure (`SESSION_CLIENT_SPEC.md #1`). The `SessionClient` itself, its channel pool, and its config manager are untouched — other pools remain openable.
 
 **`SessionRefreshConfig.OptimizedOpenRequest`** (`session.pb.go:2764+`) is the server's mechanism for supplying a *replacement* `OpenSessionRequest` the client should use on session refresh — an AFE may pre-encode a cheaper handshake (e.g., a pre-planned query for BTQL) and hand it to the client via the refresh path. When set, it overrides the pool's cached `handshake` for the next `OpenSession` call on that specific replacement session. This does not violate the "payload bytes are immutable per pool" rule — the refresh config replaces the handshake atomically on receipt, and subsequent sessions minted by the same pool use the new bytes.
 
 
@@ -219,7 +219,7 @@ Every rule is a MUST. Violations are bugs, not preferences.
 | Per-attempt transport peer labels (classic) | grpc.Peer at attempt time | can vary across attempts within an operation |
 | Per-attempt transport peer labels (session) | `InvokeResult.PeerInfo` (== `Session.peerInfo`, set once) | fixed for the session's lifetime; all attempts on session S share it (spec #16) |
 | Per-attempt `client_blocking_latencies` (session path) | `InvokeResult.SentAt` (captured by `Session.Invoke` immediately before `s.Send`, `session_vrpc.go:124-127`) | stamped once by `sessionTable.stampAttempt` (`internal/session/table.go:242-243`); MUST NOT be recomputed from any other timestamp; classic path uses `blockingLatencyTracker.messageSentNanos` instead (`CLIENT_SIDE_METRICS_SPEC.md #2`) |
-| Per-attempt `server_latencies` (session path) | `InvokeResult.Stats.BackendLatency` (typed proto duration on the vRPC response frame) | stamped once by `sessionTable.stampAttempt` (`internal/session/table.go:245-247`); MUST NOT be read from `x-goog-cbt-*` headers on this path; classic path uses `ExtractServerLatency` + t4t7 fallback (`CLIENT_SIDE_METRICS_SPEC.md #2`) |
+| Per-attempt `server_latencies` (session path) | NOT stamped on session path — `sessionTable.stampAttempt` no longer calls `SetServerLatency`; the field is left at its zero value (`internal/session/table.go`) | `InvokeResult.Stats.BackendLatency` is deliberately unused — it's not wire-comparable to classic's server-timing value; session AFE overhead lives in `transport_latencies` separately. The tracer's emission gate at `internal/metrics/tracer.go:682` still records the zero-value sample today; a follow-up will gate emission on a `serverLatencySet` flag so the histogram truly reflects session-path absence. The `connectivity_error_count` classifier gates on `transportType != ""` (PeerInfo prong), so the zero-value serverLatency does not misclassify session attempts — see `CLIENT_SIDE_METRICS_SPEC.md #2` "server_latencies" + interlock bullets. |
 | Session-tracer OTel histograms (`session.durations`, `session.open_latencies`, `session.uptime`, `transport_latencies`) | `sessionTracer` (`internal/transport/session_tracer.go:121`); registered once by `InitializeSessionMetrics` (`:67-114`) | Sole writer; `transport_latencies` recorded only from `session_pool.go:650` under the positive-delta gate at `:646-647`; `session_name` label is pool-scoped (bounded), NOT `Session.LogName` (unbounded); do NOT record from any other site or add a second registration path (`CLIENT_SIDE_METRICS_SPEC.md #3`) |
 | Debug snapshot lock discipline | `SessionPoolImpl`/`Session`/etc. — snapshot methods only | MUST take at most RLock, release before returning value; z-pages hold no lock across HTTP write (spec #15) |
 
 
@@ -686,13 +686,23 @@ func (mt *Tracer) RecordAttemptCompletion(err error) {
 	// Record connectivity_error_count
 	connErrCountAttrs, _ := mt.toOtelMetricAttrs(MetricNameConnErrCount)
 	// Determine if connection error should be incremented.
-	// A true connectivity error occurs only when we receive NO server-side signals.
-	// 1. Server latency (from server-timing header) is a signal, but absent in DirectPath.
-	// 2. Location (from x-goog-ext header) is a signal present in both paths.
-	// Therefore, we only count an error if BOTH signals are missing.
+	// A true connectivity error occurs only when we receive NO
+	// server-side signals from ANY of the three sources:
+	// 1. PeerInfo (transportType non-empty) — session-path signal;
+	//    populated by stampAttempt from Session.peerInfo. Presence
+	//    means the OpenSessionResponse arrived, so we definitively
+	//    reached a server. Session-path serverLatency is 0 by design
+	//    (see CLIENT_SIDE_METRICS_SPEC.md #2) so without this prong
+	//    every session attempt would misclassify as a connectivity
+	//    error.
+	// 2. Server latency (from server-timing header) — classic unary
+	//    signal; absent in DirectPath and always 0 on session path.
+	// 3. Location (from x-goog-ext header) — both paths.
+	// Any one of the three = we reached a server; NOT a connectivity error.
+	hasPeerInfo := mt.currOp.currAttempt.transportType != ""
 	isServerLatencyEffectivelyEmpty := mt.currOp.currAttempt.serverLatencyErr != nil || mt.currOp.currAttempt.serverLatency == 0
 	isLocationEmpty := mt.currOp.currAttempt.clusterID == defaultCluster
-	if isServerLatencyEffectivelyEmpty && isLocationEmpty {
+	if !hasPeerInfo && isServerLatencyEffectivelyEmpty && isLocationEmpty {
 		// This is a connectivity error: the request likely never reached Google's network.
 		mt.instrumentConnErrCount.Add(mt.ctx, 1, metric.WithAttributeSet(connErrCountAttrs))
 	} else {