Skip to content

Commit 4a68e10

Browse files
committed
fix(bigtable): address ingestMetadata review nits
- Drop the paragraph in ingestMetadata's doc that recounted the pre- race-fix code (stashed MD on the attempt, csAttempt.finish vs transport reader collision, issue #20152 link). The comment now reads forward-only. - Add a nested comment on the peerInfoExtracted latch explaining why the gate is peerInfo != nil rather than the err == nil pattern used for locationExtracted: extractPeerInfo returns (nil, nil) on missing header (not an error), so gating on err == nil alone would latch on the header call and kill the trailer fallback. No behaviour change. Verified: go build ./... → clean go vet ./bigtable/internal/metrics/... → clean go test -race -count=1 ./bigtable/internal/metrics/... → pass
1 parent 05d910a commit 4a68e10

1 file changed

Lines changed: 6 additions & 8 deletions

File tree

bigtable/internal/metrics/tracer.go

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -531,14 +531,6 @@ func (mt *Tracer) RecordAttemptStart() {
531531
// those primitives, so stats.End (which may dispatch on a different
532532
// goroutine under cancel/deadline/GOAWAY) never touches the raw MD.
533533
//
534-
// The old code stored ev.Header / ev.Trailer on the attempt and re-read
535-
// them inside RecordAttemptCompletion. When csAttempt.finish raced the
536-
// transport reader, End and InTrailer collided on both the MD field
537-
// and the underlying map — the race dump on
538-
// https://github.com/googleapis/google-cloud-go/issues/20152 (and
539-
// several sibling FlakyBot P1 issues from the same nightly run) is the
540-
// downstream symptom.
541-
//
542534
// extractLocation / extractPeerInfo / extractServerLatency all check
543535
// header first then trailer, so passing one MD at a time and gating on
544536
// the *Extracted booleans preserves the header-preferred-trailer-fallback
@@ -594,6 +586,12 @@ func (mt *Tracer) ingestMetadata(headerMD, trailerMD metadata.MD) {
594586
// metadata (populated by the server when the PeerInfo feature
595587
// flag is negotiated on). Feeds attempt_latencies2 only; other
596588
// metrics stay on the classic label set.
589+
//
590+
// Latch peerInfoExtracted only when extractPeerInfo returned a
591+
// parsed *PeerInfo (which implies err == nil). Bare
592+
// (nil, err) — a base64 or proto-decode failure — and bare
593+
// (nil, nil) — the key is absent — both leave the latch off so
594+
// the trailer still gets a chance to populate.
597595
if peerInfo, _ := extractPeerInfo(headerMD, trailerMD); peerInfo != nil {
598596
a.transportType = TransportTypeName(peerInfo.GetTransportType())
599597
a.transportRegion = peerInfo.GetApplicationFrontendRegion()

0 commit comments

Comments
 (0)