stats/opentelemetry: record retry attempts from clientStream #8342

vinothkumarr227 · 2025-05-19T09:14:38Z

RELEASE NOTES:

stats/opentelemetry: Retry attempts (grpc.previous-rpc-attempts) are now recorded as span attributes for non-transparent client retries.

codecov · 2025-05-19T09:19:34Z

Codecov Report

Attention: Patch coverage is 71.42857% with 8 lines in your changes missing coverage. Please review.

Project coverage is 82.29%. Comparing base (f2d3e11) to head (ba08688).
Report is 21 commits behind head on master.

Files with missing lines	Patch %	Lines
stats/opentelemetry/client_tracing.go	42.85%	3 Missing and 1 partial ⚠️
stats/opentelemetry/trace.go	78.94%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8342      +/-   ##
==========================================
- Coverage   82.41%   82.29%   -0.13%     
==========================================
  Files         419      419              
  Lines       42025    42087      +62     
==========================================
- Hits        34637    34636       -1     
- Misses       5944     5995      +51     
- Partials     1444     1456      +12

Files with missing lines	Coverage Δ
stats/opentelemetry/client_metrics.go	`85.16% <100.00%> (+0.08%)`	⬆️
stats/opentelemetry/opentelemetry.go	`77.90% <ø> (ø)`
stats/opentelemetry/server_tracing.go	`74.19% <100.00%> (+0.86%)`	⬆️
stats/opentelemetry/client_tracing.go	`83.09% <42.85%> (-4.41%)`	⬇️
stats/opentelemetry/trace.go	`85.07% <78.94%> (-3.82%)`	⬇️

... and 43 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

stream.go

stats/opentelemetry/trace.go

stats/opentelemetry/client_tracing.go

purnesh42H

I remember we had separate tests for retries. This change should only affect that test. This change shouldn't affect tests which are doing only single attempt.

stats/opentelemetry/client_tracing.go

stats/opentelemetry/trace.go

stats/opentelemetry/e2e_test.go

purnesh42H

@vinothkumarr227 have you tested this to ensure its working correctly? I was under impression that TestTraceSpan_WithRetriesAndNameResolutionDelay will need changes for expected values. I think its not working as expected because you are not setting the count back to ctx after incrementing.

stats/opentelemetry/client_tracing.go

stats/opentelemetry/opentelemetry.go

stats/opentelemetry/trace.go

stats/opentelemetry/opentelemetry.go

stats/opentelemetry/e2e_test.go

stats/opentelemetry/client_metrics.go

stats/opentelemetry/e2e_test.go

stats/opentelemetry/opentelemetry.go

stats/opentelemetry/trace.go

stats/opentelemetry/client_tracing.go

stats/opentelemetry/e2e_test.go

stats/opentelemetry/client_tracing.go

stats/opentelemetry/e2e_test.go

purnesh42H · 2025-06-05T19:01:08Z

stats/opentelemetry/e2e_test.go

+		want := wantSpanInfos[matchedIndex]
+		// Check that the attempt span has the correct status.
+		if want.status != otelcodes.Unset {
+			got, want := span.Status.Code, want.status


nit: got, want can be assigned in the same as the if check

purnesh42H · 2025-06-05T19:07:18Z

stats/opentelemetry/server_tracing.go

@@ -41,6 +41,7 @@ func (h *serverTracingHandler) initializeTraces() {
 // TagRPC implements per RPC attempt context management for traces.
 func (h *serverTracingHandler) TagRPC(ctx context.Context, _ *stats.RPCTagInfo) context.Context {
 	ctx, ai := getOrCreateRPCAttemptInfo(ctx)
+	ai.ctx = ctx
 	ctx, ai = h.traceTagRPC(ctx, ai)


this is probably not the updated ctx as that's being updated in traceTagRPC. Either update the ai.ctx in traceTagRPC at the end or after it.

purnesh42H · 2025-06-05T19:11:34Z

stats/opentelemetry/client_tracing.go

@@ -121,6 +121,10 @@ func (h *clientTracingHandler) HandleConn(context.Context, stats.ConnStats) {}
 // TagRPC implements per RPC attempt context management for traces.
 func (h *clientTracingHandler) TagRPC(ctx context.Context, info *stats.RPCTagInfo) context.Context {
 	ctx, ai := getOrCreateRPCAttemptInfo(ctx)
+	if ci := getCallInfo(ctx); ci != nil {


can ci be ever nil at the attempt stage? I don't we need to check for nil here. If its nil, we shouldn't proceed further and return early logging an error.

purnesh42H · 2025-06-05T19:12:53Z

stats/opentelemetry/trace.go

+		// Increment retry count for the next attempt if not a transparent
+		// retry.
+		if !rs.IsTransparentRetryAttempt {
+			if ci := getCallInfo(ai.ctx); ci != nil {


same here. I don't think ci should be nil at this stage.

purnesh42H · 2025-06-05T19:17:32Z

stats/opentelemetry/e2e_test.go

+							},
+						},
+					},
+					// RPC attempt #3


why RPC attempt 3 doesn't have a corresponding server span?

Yeah, I missed it earlier. I've added it now.

purnesh42H · 2025-06-05T19:18:40Z

stats/opentelemetry/e2e_test.go

+							{Name: delayedResolutionEventName},
+						},
+					},
+					// RPC attempt #3


same question about corresponding server span

arjan-bal · 2025-06-11T08:44:42Z

stats/opentelemetry/client_tracing.go

+		return ctx
+	}
+	ai.previousRPCAttempts = uint32(ci.previousRPCAttempts.Load())
+	ai.ctx = ctx
 	ctx, ai = h.traceTagRPC(ctx, ai, info.NameResolutionDelay)


traceTagRPC mutates the attempt info pointer that is passed to it, making the returned attempt info redundant. In the case of context, returning a new context makes sense since contexts are immutable. Seeing an attempt info being returned may lead readers to (incorrectly) assume the passed attemptInfo is not mutated. Sometimes arguments are mutated and returned for chaining function calls, but here that's not possible since there are two values.

Can you please remove the returned attempt info and document that the passed argument is mutated in-place?

arjan-bal · 2025-06-11T08:55:44Z

stats/opentelemetry/opentelemetry.go

@@ -179,6 +179,9 @@ type callInfo struct {
 	// nameResolutionEventAdded is set when the resolver delay trace event
 	// is added. Prevents duplicate events, since it is reported per-attempt.
 	nameResolutionEventAdded atomic.Bool
+	// previousRPCAttempts holds the count of RPC attempts that have happened
+	// before current attempt. Transparent retries are excluded.
+	previousRPCAttempts atomic.Int32


Can you instead make this a pointer, i.e. *atomic.Int32? Then we can copy the pointer into attemptInfo. Since both will be pointing to the same underlying Int32, you will not need to update callInfo at the end of each attempt.

Fixed retry attempts in HandleRPC

3ba457e

purnesh42H reviewed May 19, 2025

View reviewed changes

stream.go Outdated Show resolved Hide resolved

purnesh42H assigned vinothkumarr227 May 19, 2025

purnesh42H added Type: Bug Area: Observability labels May 19, 2025

purnesh42H added this to the 1.73 Release milestone May 19, 2025

vinothkumarr227 added 2 commits May 20, 2025 06:55

Fixed the review changes

5d19779

Fixed vet issues

4245950

vinothkumarr227 requested a review from purnesh42H May 20, 2025 07:09

eshitachandwani assigned purnesh42H and unassigned vinothkumarr227 May 20, 2025

purnesh42H reviewed May 20, 2025

View reviewed changes

stats/opentelemetry/trace.go Outdated Show resolved Hide resolved

stats/opentelemetry/client_tracing.go Outdated Show resolved Hide resolved

stats/opentelemetry/client_tracing.go Outdated Show resolved Hide resolved

purnesh42H assigned vinothkumarr227 and unassigned purnesh42H May 20, 2025

Fixed the review changes

5347db1

vinothkumarr227 requested a review from purnesh42H May 21, 2025 06:08

purnesh42H reviewed May 21, 2025

View reviewed changes

stats/opentelemetry/client_tracing.go Outdated Show resolved Hide resolved

stats/opentelemetry/trace.go Outdated Show resolved Hide resolved

stats/opentelemetry/trace.go Outdated Show resolved Hide resolved

stats/opentelemetry/e2e_test.go Outdated Show resolved Hide resolved

Fixed the review changes

99e88d8

vinothkumarr227 requested a review from purnesh42H May 22, 2025 07:51

purnesh42H reviewed May 26, 2025

View reviewed changes

vinothkumarr227 added 2 commits May 26, 2025 12:58

Fixed the review changes

586cf63

Fixed the test cases

1bdad7e

eshitachandwani assigned purnesh42H and unassigned vinothkumarr227 May 27, 2025

eshitachandwani requested a review from purnesh42H May 27, 2025 09:37

purnesh42H reviewed May 28, 2025

View reviewed changes

purnesh42H assigned vinothkumarr227 and unassigned purnesh42H May 28, 2025

Fixed the review changes

39c5f0d

small tweaks

11523b6

purnesh42H reviewed Jun 2, 2025

View reviewed changes

vinothkumarr227 added 5 commits June 3, 2025 12:53

Fixed the review changes

3720f4e

Fixed the test cases

b97a2da

Fixed the test cases pick issues

a0ef86c

Fixed the event ignore issues

ac79ad2

Fixed the picker event issues

10c6a90

vinothkumarr227 requested a review from purnesh42H June 4, 2025 03:59

eshitachandwani assigned purnesh42H and unassigned vinothkumarr227 Jun 4, 2025

Fixed the test cases

1048040

eshitachandwani modified the milestones: 1.73 Release, 1.74 Release Jun 5, 2025

purnesh42H reviewed Jun 5, 2025

View reviewed changes

purnesh42H assigned vinothkumarr227 and unassigned purnesh42H Jun 5, 2025

vinothkumarr227 added 2 commits June 6, 2025 05:09

Fixed the review changes

05e2cc8

small tweaks

ba08688

vinothkumarr227 requested a review from purnesh42H June 6, 2025 05:49

vinothkumarr227 removed their assignment Jun 10, 2025

arjan-bal self-assigned this Jun 11, 2025

arjan-bal reviewed Jun 11, 2025

View reviewed changes

arjan-bal assigned vinothkumarr227 and unassigned arjan-bal Jun 11, 2025

stats/opentelemetry: record retry attempts from clientStream #8342

Are you sure you want to change the base?

stats/opentelemetry: record retry attempts from clientStream #8342

Conversation

vinothkumarr227 commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

purnesh42H left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

purnesh42H left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vinothkumarr227 commented May 19, 2025 •

edited

Loading

codecov bot commented May 19, 2025 •

edited

Loading