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 52.63158% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.50%. Comparing base (e60a04b) to head (c0e523c).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
stats/opentelemetry/client_tracing.go	52.63%	6 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8342      +/-   ##
==========================================
- Coverage   81.64%   81.50%   -0.15%     
==========================================
  Files         413      413              
  Lines       40621    40693      +72     
==========================================
- Hits        33167    33166       -1     
- Misses       5991     6000       +9     
- Partials     1463     1527      +64     

Files with missing lines	Coverage Δ
stats/opentelemetry/opentelemetry.go	50.94% <ø> (-26.97%)	⬇️
stats/opentelemetry/trace.go	41.07% <ø> (-47.82%)	⬇️
stats/opentelemetry/client_tracing.go	55.31% <52.63%> (-32.19%)	⬇️

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

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.

[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

[dfawley]

Sorry for the delays in review here. LGTM after this one small change.

[dfawley]

Let's make this a non-pointer type and then we don't need the new here or the chance to get nil panics.

[vinothkumarr227]

Done

[dfawley]

Please delete this line. This is the zero value so it doesn't need explicit initialization.

[vinothkumarr227]

Done