Skip to content

Commit 1a1d1b3

Browse files
sushanbsushanb
committed
refactor(bigtable): drop dead session-attempt debug tags + upstream chain
Addresses sushanb inline nits on PR #20228 at debug_tracer.go:259 ("why is this needed?") and :267 ("please clean this up"). Both tags were emitted only from stampAttempt call sites that Batch 1 removed; the catalog constants are dead. Removed: - TagSessionAttemptNilClusterInfo + TagSessionAttemptEmptyClusterID const declarations (dead — no emitters after Batch 1). - DebugTag type + RecordDebugTag(DebugTag) exported function (only existed to type-parameterize the RecordDebugTag call from internal/session's stampAttempt; that caller is gone, so no out-of-package consumers remain). - tagSessionPoolCheckoutFailedCINil const + its emission at SessionPoolImpl.Invoke's checkout-failure exit — this tag's whole rationale was "attribute downstream nil-ClusterInfo to pool checkout failure"; that rationale collapsed with the downstream tag removal. Preserved: - The p.recordCheckoutFailure(...) call on the checkout-failure exit, so pool-exhaustion incidents still surface in sessionz's slow-vRPC table + latency histograms. - The unexported recordDebugTag helper — still used by the other in-package tags in the catalog. Reviewed by session-reviewer + session-component-review + igor-reviewer — all pass.
1 parent 5023e0c commit 1a1d1b3

2 files changed

Lines changed: 0 additions & 49 deletions

File tree

‎bigtable/internal/transport/debug_tracer.go‎

Lines changed: 0 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -134,15 +134,6 @@ const (
134134
// the penalty window.
135135
tagSessionPoolNoBudget = "session_pool_no_budget"
136136

137-
// tagSessionPoolCheckoutFailedCINil fires on SessionPoolImpl.Invoke's
138-
// early return when CheckoutSession failed — pool returns
139-
// InvokeResult{} with nil ClusterInfo, so stampAttempt downstream
140-
// records TagSessionAttemptNilClusterInfo without any session ever
141-
// being picked. Empirically dominates the nil-ClusterInfo population
142-
// during pool cold-start (waiters ctx.Done before first session
143-
// reaches Ready) and pool-close bursts (drainWaitersWithErr).
144-
tagSessionPoolCheckoutFailedCINil = "session_pool_checkout_failed_ci_nil"
145-
146137
// sessionList bookkeeping violations.
147138
//
148139
// tagSessionListRefcountUnderflow fires when OnSessionClosed would
@@ -242,41 +233,6 @@ func setDebugTagLevelFloor(l debugLevel) {
242233
debugTagLevelFloor.Store(int32(l))
243234
}
244235

245-
// DebugTag is the typed form for tag names exposed across package
246-
// boundaries. Callers must pass a catalog constant (e.g.,
247-
// TagSessionAttemptNilClusterInfo below) rather than a raw string —
248-
// the type prevents arbitrary literals from drifting off the catalog.
249-
type DebugTag string
250-
251-
const (
252-
// TagSessionAttemptNilClusterInfo fires when a session-path attempt
253-
// completes with no ClusterInformation on the InvokeResult — either
254-
// the attempt failed with a transport error (no server response) or
255-
// the server response omitted ClusterInformation. Downstream, the
256-
// attempt's cluster_id label defaults to <unspecified> because
257-
// stampAttempt has nothing to stamp AND session path has no per-vRPC
258-
// gRPC headers for ExtractLocation to fall back on.
259-
TagSessionAttemptNilClusterInfo DebugTag = "session_attempt_nil_cluster_info"
260-
261-
// TagSessionAttemptEmptyClusterID fires when ClusterInformation is
262-
// present on the InvokeResult but ClusterId is empty — a server
263-
// contract violation (server should always populate ClusterId on
264-
// vRPC responses per CLIENT_SIDE_METRICS_SPEC #1). Companion to
265-
// TagSessionAttemptNilClusterInfo; distinct so ops can tell
266-
// "server didn't respond" from "server responded without cluster".
267-
TagSessionAttemptEmptyClusterID DebugTag = "session_attempt_empty_cluster_id"
268-
)
269-
270-
// RecordDebugTag is the exported form for other packages under
271-
// bigtable/internal that need to fire tags from their own layer
272-
// (e.g., internal/session's stampAttempt observing missing
273-
// ClusterInformation on session-path attempts). Same semantics as
274-
// recordDebugTag; the DebugTag typing forces callers to use a catalog
275-
// constant rather than an ad-hoc string.
276-
func RecordDebugTag(t DebugTag) {
277-
recordDebugTag(string(t))
278-
}
279-
280236
// recordDebugTag increments the debug_tags counter for `name` at Warn.
281237
// Safe to call before InitializeSessionMetrics — only the in-memory
282238
// map increments until the OTel counter is registered.

‎bigtable/internal/transport/session_pool.go‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -499,11 +499,6 @@ func (p *SessionPoolImpl) Invoke(ctx context.Context, desc VRpcDescriptor, req i
499499
// Record checkout failure so pool-exhaustion incidents show up
500500
// in sessionz's slow-vRPC table and latency histograms.
501501
p.recordCheckoutFailure(checkoutStart, desc, err)
502-
// Attributes the resulting downstream TagSessionAttemptNilClusterInfo
503-
// to the pool checkout-failure exit — otherwise the nil at
504-
// stampAttempt is indistinguishable from "session picked but
505-
// returned nil". Dominates during pool cold-start warmup.
506-
recordDebugTag(tagSessionPoolCheckoutFailedCINil)
507502
return InvokeResult{}, err
508503
}
509504
// poolWait is the queue-time spent inside CheckoutSession waiting

0 commit comments

Comments
 (0)