stats/otel: Add client side metrics for retries (A96) - #9317
Conversation
Fixed a race condition in pickfirst metrics tests where the assertions would immediately poll the stats.TestMetricsRecorder. Depending on the goroutine scheduling, asynchronous metrics like 'grpc.subchannel.open_connections' would occasionally not be fully recorded before the test checked them, causing flaky failures. This introduces an awaitMetric helper to wait for the metrics to reach their expected values.
… in TestMetricsRecorder
… in TestMetricsRecorder
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9317 +/- ##
==========================================
+ Coverage 83.12% 83.27% +0.14%
==========================================
Files 422 423 +1
Lines 35044 35180 +136
==========================================
+ Hits 29131 29296 +165
+ Misses 4410 4392 -18
+ Partials 1503 1492 -11
🚀 New features to boost your workflow:
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements OpenTelemetry client-side retry metrics, including retries, transparent retries, hedges, and retry delays, along with corresponding unit and end-to-end tests. The feedback highlights a potential race condition in client_metrics.go between the end of one attempt and the start of the next when calculating retry delays, suggesting the use of a mutex to protect these fields. Additionally, suggestions were made to update the expected attributes in the unit tests to include the grpc.status attribute.
| // Accumulate retry delay. | ||
| active := ci.activeAttempts.Add(1) | ||
| if active == 1 { | ||
| lastEndTime := ci.lastAttemptEndTime.Swap(0) | ||
| if lastEndTime > 0 { | ||
| delay := st.BeginTime.Sub(time.Unix(0, lastEndTime)) | ||
| if delay > 0 { | ||
| ci.retryDelay.Add(int64(delay)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a potential race condition between the stats.End of attempt N (executed by the transport reader goroutine) and the stats.Begin of attempt N+1 (executed by the client-call goroutine).
If stats.Begin of the next attempt runs and increments activeAttempts before stats.End of the previous attempt has finished storing lastAttemptEndTime, the next attempt will swap lastAttemptEndTime and get 0 (or an older value), missing the retry delay calculation. Then, the previous attempt's stats.End will store its end time after the swap, corrupting the state for subsequent attempts.
Consider protecting these retry tracking fields with a sync.Mutex in callInfo to ensure thread-safe state transitions across different goroutines.
| Data: metricdata.Histogram[int64]{ | ||
| DataPoints: []metricdata.HistogramDataPoint[int64]{ | ||
| { | ||
| Attributes: attribute.NewSet(attribute.String("grpc.method", "test-method"), attribute.String("grpc.target", "test-target")), |
There was a problem hiding this comment.
Update the expected attributes to include grpc.status with value "OK" to match the updated retry metrics implementation.
| Attributes: attribute.NewSet(attribute.String("grpc.method", "test-method"), attribute.String("grpc.target", "test-target")), | |
| Attributes: attribute.NewSet(attribute.String("grpc.method", "test-method"), attribute.String("grpc.target", "test-target"), attribute.String("grpc.status", "OK")), |
| Data: metricdata.Histogram[int64]{ | ||
| DataPoints: []metricdata.HistogramDataPoint[int64]{ | ||
| { | ||
| Attributes: attribute.NewSet(attribute.String("grpc.method", "test-method"), attribute.String("grpc.target", "test-target")), |
There was a problem hiding this comment.
Update the expected attributes to include grpc.status with value "OK" to match the updated retry metrics implementation.
| Attributes: attribute.NewSet(attribute.String("grpc.method", "test-method"), attribute.String("grpc.target", "test-target")), | |
| Attributes: attribute.NewSet(attribute.String("grpc.method", "test-method"), attribute.String("grpc.target", "test-target"), attribute.String("grpc.status", "OK")), |
3 participants
This PR implements retry metrics for A96
RELEASE NOTES: