stats/opentelemetry: record retry attempts from clientStream #8342

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

Fixes: #8299

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

All modified and coverable lines are covered by tests ✅

Project coverage is 82.31%. Comparing base (f2d3e11) to head (99e88d8).
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8342      +/-   ##
==========================================
- Coverage   82.41%   82.31%   -0.11%     
==========================================
  Files         419      419              
  Lines       42025    42052      +27     
==========================================
- Hits        34637    34615      -22     
- Misses       5944     5980      +36     
- Partials     1444     1457      +13

Files with missing lines	Coverage Δ
stats/opentelemetry/client_tracing.go	`88.88% <100.00%> (+1.38%)`	⬆️
stats/opentelemetry/opentelemetry.go	`78.53% <100.00%> (+0.62%)`	⬆️
stats/opentelemetry/server_tracing.go	`74.19% <100.00%> (+0.86%)`	⬆️
stats/opentelemetry/trace.go	`89.65% <100.00%> (+0.76%)`	⬆️

... and 24 files with indirect coverage changes

🚀 New features to boost your workflow:

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

purnesh42H · 2025-05-19T09:37:52Z

stream.go

@@ -430,6 +432,7 @@ func (cs *clientStream) newAttemptLocked(isTransparent bool) (*csAttempt, error)
 			IsServerStream:            cs.desc.ServerStreams,
 			IsTransparentRetryAttempt: isTransparent,
 		}
+		ctx = context.WithValue(ctx, clientStreamKey{}, cs.numRetries)


as @dfawley suggested, we should just compute the retry attempts within otel interceptors #8299 (comment). While incrementing in trace.go, we should just skip if transparent retry is true. Re-using cs.numRetries is not advisable.

Thanks! Updated to compute retry attempts in the otel interceptor (TagRPC) and no longer using cs.numRetries

purnesh42H · 2025-05-20T09:48:07Z

stats/opentelemetry/trace.go

 		// Note: Go always added Client and FailFast attributes even though they are not
 		// defined by the OpenCensus gRPC spec. Thus, they are unimportant for
 		// correctness.
 		span.SetAttributes(
 			attribute.Bool("Client", rs.Client),
 			attribute.Bool("FailFast", rs.FailFast),
 			attribute.Int64("previous-rpc-attempts", int64(ai.previousRPCAttempts)),
+			attribute.Int64("retry-attempts", int64(atomic.LoadUint32(&ai.explicitRetryCount))),


we shouldn't be adding a new attribute. previous-rpc-attempts should be enough to depict retries. If > 0, that means there were retries. You should be doing the increment here if transparent-retry is false.

See https://github.com/grpc/proposal/blob/master/A72-open-telemetry-tracing.md#tracing-information. We should be confined to attributes defined here.

purnesh42H · 2025-05-20T10:01:05Z

stats/opentelemetry/client_tracing.go

@@ -121,6 +122,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 ai.previousRPCAttempts > 0 {
+		atomic.AddUint32(&ai.explicitRetryCount, 1)


Retry attempts need to be counted by storing data in the context in a per-call interceptor. If you store it in the attemptInfo, it will get reset every attempt.

purnesh42H · 2025-05-20T10:02:16Z

stats/opentelemetry/client_tracing.go

+	if ai.previousRPCAttempts > 0 {
+		atomic.AddUint32(&ai.explicitRetryCount, 1)
+	}
+	atomic.AddUint32(&ai.previousRPCAttempts, 1)


you can maintain single attribute. Either previousRPCAttempts or explicitRetryCount. Why do we need both?

Thanks. I've removed explicitRetryCount and now use only previousRPCAttempts to track retries.

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.

purnesh42H · 2025-05-21T06:17:39Z

stats/opentelemetry/client_tracing.go

 // 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)
+	var counter *int32
+	if val := ctx.Value(retryCountKey{}); val != nil {
+		counter = val.(*int32)


this again seems wrong. We should create the retry count key in the interceptor call which is per RPC. TagRPC is for per attempt. Here we should only retrieve the current retry count value (if needed)

Thanks for the clarification. I've updated the code accordingly — the retry count key is now created within the interceptor to ensure it's per-RPC

purnesh42H · 2025-05-21T06:19:11Z

stats/opentelemetry/trace.go

+		if !rs.IsTransparentRetryAttempt {
+			if val := ai.ctx.Value(retryCountKey{}); val != nil {
+				// Atomic increment and get new value
+				retryCount = uint32(atomic.AddInt32(val.(*int32), 1))


this value needs to be set back to context somewhere? otherwise we have lost the count.

purnesh42H · 2025-05-21T06:24:35Z

stats/opentelemetry/trace.go

 		// Note: Go always added Client and FailFast attributes even though they are not
 		// defined by the OpenCensus gRPC spec. Thus, they are unimportant for
 		// correctness.
 		span.SetAttributes(
 			attribute.Bool("Client", rs.Client),
 			attribute.Bool("FailFast", rs.FailFast),
-			attribute.Int64("previous-rpc-attempts", int64(ai.previousRPCAttempts)),
+			attribute.Int64("previous-rpc-attempts", int64(retryCount)),


Please note as per https://github.com/grpc/proposal/blob/master/A72-open-telemetry-tracing.md#tracing-information, previousRPCAttempts is the count of retries before current, so we should set here the existing retryCount value (before incrementing it). However, we should increment retry count after and set back to context for future.

purnesh42H · 2025-05-21T06:26:08Z

stats/opentelemetry/e2e_test.go

@@ -928,7 +928,7 @@ func (s) TestMetricsAndTracesOptionEnabled(t *testing.T) {
 				},
 				{
 					Key:   "previous-rpc-attempts",
-					Value: attribute.IntValue(0),
+					Value: attribute.IntValue(1),


It should be correctly 0 because it counts previous rpc attempts which means before current so for the first attempt it should be 0.

Fixed retry attempts in HandleRPC

3ba457e

purnesh42H reviewed May 19, 2025

View reviewed changes

purnesh42H assigned vinothkumarr227 May 19, 2025

purnesh42H added Type: Bug Area: Observability Includes Stats, Tracing, Channelz, Healthz, Binlog, Reflection, Admin, GCP 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

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

Fixed the review changes

99e88d8

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

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

Uh oh!

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!

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!

purnesh42H left a comment

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