stats/opentelemetry: record retry attempts from clientStream

[vinothkumarr227]

Fixes: #8299

RELEASE NOTES:

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

[codecov]

Codecov Report

❌ Patch coverage is 70.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.57%. Comparing base (85240a5) to head (992343e).
⚠️ Report is 11 commits behind head on master.

Files with missing lines	Patch %	Lines
stats/opentelemetry/client_tracing.go	64.70%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8342      +/-   ##
==========================================
- Coverage   82.48%   81.57%   -0.92%     
==========================================
  Files         413      413              
  Lines       40518    40546      +28     
==========================================
- Hits        33422    33076     -346     
- Misses       5738     6047     +309     
- Partials     1358     1423      +65     

Files with missing lines	Coverage Δ
stats/opentelemetry/client_metrics.go	85.16% <100.00%> (+0.08%)	⬆️
stats/opentelemetry/opentelemetry.go	56.38% <ø> (-21.53%)	⬇️
stats/opentelemetry/trace.go	85.71% <ø> (-3.18%)	⬇️
stats/opentelemetry/client_tracing.go	77.50% <64.70%> (-10.00%)	⬇️

... and 44 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]

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]

@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.

[arjan-bal]

LGTM, adding a second reviewer. Please address the minor comment.

[arjan-bal]

Please add the (s) back.

[vinothkumarr227]

Done

[arjan-bal]

@vinothkumarr227 there is a failure in the updated test reported by codecov: https://github.com/grpc/grpc-go/actions/runs/16616024027/job/47008766111?pr=8342

PTAL and ensure the test is not flaky.

[vinothkumarr227]

@vinothkumarr227 there is a failure in the updated test reported by codecov: https://github.com/grpc/grpc-go/actions/runs/16616024027/job/47008766111?pr=8342

PTAL and ensure the test is not flaky.

Sure, I’ll check it out.

[vinothkumarr227]

@vinothkumarr227 there is a failure in the updated test reported by codecov: https://github.com/grpc/grpc-go/actions/runs/16616024027/job/47008766111?pr=8342

PTAL and ensure the test is not flaky.
Hi @arjan-bal , I tested the test locally and everything passed consistently. I’ve now merged the latest changes from master, and all test cases are passing.

[dfawley]

Why is this a TODO?

Isn't this simply?

attribs := []attribute.KeyValue{...Bool("Client", rs.Client), /* etc */}
if ai.previousRPCAttempts != nil {
	attribs = append(attribs, attribute.Int64("prev...", ..), ...)
}

[vinothkumarr227]

Thanks for the suggestion. Per @arjan-bal recommendation, this will be handled in a separate PR, which is why it's a TODO. I've created an issue for reference: #8430

[dfawley]

I'm not sure I understand why. If these attributes are not supposed to be present, then wouldn't it be incorrect to include them.

[vinothkumarr227]

Thanks Doug. I've made changes and pushed the fix.

[dfawley]

Oh it was already setting these things?

Also what is this about "Go was always setting these attributes even though they aren't in the spec"?

Are we intentionally not implementing the spec? Why are we mentioning the OC spec in the OT plugin? I'm kind of lost here, TBH.

[vinothkumarr227]

Hi Doug, regarding this line "Go was always setting these attributes even though they aren't in the spec"?, I see that this one are already there, but I'm not familiar with this one. Could you please clarify that?

[dfawley]

Right. I don't understand this either. Something is wrong.

Either way, this function should probably not have client/server-specific code in it, as it does not. It should only contain things we do commonly for both. We have separate client and server handlers for this specific reason. If we need to delete this function or rework it so it behaves differently, then that's probably worth doing.

grpc-go/stats/opentelemetry/client_tracing.go

Line 135 in 57b69b4

populateSpan(rs, ri.ai)

[vinothkumarr227]

Thanks for the feedback, Doug. I've refactored the code to address your concerns. The client- and server-specific logic has been moved out of populateSpan and into their dedicated handlers. The populateSpan function now only contains the common logic.

[dfawley]

Is it possible for this to be false?

And is there any reason this is a pointer instead of just a value? Then we wouldn't need nil checks, or to construct it explicitly.

[vinothkumarr227]

It’s not possible for it to be nil — we always initialize it in getOrCreateCallInfo with ci.previousRPCAttempts = new(atomic.Uint32). I’ve removed the nil check as well. The pointer is used so that updates in ai are automatically reflected in ci.

[dfawley]

Oh this is in the attemptInfo. Why are we keeping it here instead of just loading it out of the callInfo?

Also I realized that with hedging, the previous attempts accounting is racy:

• Attempt 1 starts
• Attempt 2 starts
• Attempt 3 starts
• All attempts do Begin simultaneously

It's possible for any of them to have any value now. You need to instead increment and read together:

previousAttempts := previousRPCAttempts.Add(1) - 1 // Add returns the new value; we need the old value

Then they will all see unique values.

(We don't implement hedging yet, but we need to keep in mind that we will one day.)

[vinothkumarr227]

previousRPCAttempts represents the number of retries before the current attempt. We should record the existing retry count in attemptInfo before incrementing it. After that, we increment the retry count.

[vinothkumarr227]

Also I realized that with hedging, the previous attempts accounting is racy:

• Attempt 1 starts
• Attempt 2 starts
• Attempt 3 starts
• All attempts do Begin simultaneously

It's possible for any of them to have any value now. You need to instead increment and read together:

previousAttempts := previousRPCAttempts.Add(1) - 1 // Add returns the new value; we need the old value

Then they will all see unique values.

(We don't implement hedging yet, but we need to keep in mind that we will one day.)
Sure, I’ll keep that in mind; we’ll do it in a future update.

[dfawley]

previousRPCAttempts represents the number of retries before the current attempt. We should record the existing retry count in attemptInfo before incrementing it. After that, we increment the retry count.

No, we need to atomically read and increment it, as I explained.

[vinothkumarr227]

Hi Doug, I’m a bit confused. Initially, I had the increment here — https://github.com/grpc/grpc-go/blob/master/stats/opentelemetry/client_metrics.go#L78 — but after the review comments, I’m moved to trace. Could you clarify what exactly you’d like to change here?

[dfawley]

If we do the load and add separately, there is a race if multiple attempts are happening at once, with hedging. So we need to do the add and use the value it returns. It returns the incremented value, so we need to subtract 1 from it to get the original value. This way if multiple attempts are happening at the same time, then each one will get a unique value.

I think we should also see about removing the previousRPCAttempts pointer from the attemptInfo, and instead read it out of the callInfo, which is stored in the context too, right?

[vinothkumarr227]

Thanks for the suggestion! I'll update the code accordingly. I'll also double-check how it's stored in the context.

[vinothkumarr227]

If we do the load and add separately, there is a race if multiple attempts are happening at once, with hedging. So we need to do the add and use the value it returns. It returns the incremented value, so we need to subtract 1 from it to get the original value. This way if multiple attempts are happening at the same time, then each one will get a unique value.

I think we should also see about removing the previousRPCAttempts pointer from the attemptInfo, and instead read it out of the callInfo, which is stored in the context too, right?

Thanks for the feedback! I've updated the code.

[dfawley]

Why are we loading this here but only using it inside the if below? Why not load only when needed?

[vinothkumarr227]

No need for the condition — I’ve removed the nil check as well.

[dfawley]

Let's remove these now since they are not part of the spec

[vinothkumarr227]

Done