stats: add DelayedPickComplete and follow correct semantics #8465
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8465 +/- ##
==========================================
+ Coverage 82.42% 82.54% +0.12%
==========================================
Files 414 414
Lines 40434 40464 +30
==========================================
+ Hits 33326 33400 +74
+ Misses 5752 5718 -34
+ Partials 1356 1346 -10
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
LGTM other than a couple of minor comments. I've linked the associated issue in the description. Thank you for fixing this!
FYI @vinothkumarr227.
stats/stats.go
Outdated
| // PickerUpdated indicates that the RPC is unblocked following a delay in | ||
| // selecting a connection for the call. | ||
| // | ||
| // Deprecated: will be removed in a future release. |
There was a problem hiding this comment.
Should we also specify that DelayedPickComplete should be used instead?
There was a problem hiding this comment.
Yes. Related: I think technically we're not supposed to put "Deprecated" on this until after DelayedPickComplete exists for a release. But since this package is experimental, I think we can skip that and call it deprecated immediately.
| @@ -481,6 +482,11 @@ func (a *csAttempt) getTransport() error { | |||
| if a.trInfo != nil { | |||
| a.trInfo.firstLine.SetRemoteAddr(a.transport.RemoteAddr()) | |||
| } | |||
| if pick.blocked { | |||
| for _, sh := range a.statsHandlers { | |||
| sh.HandleRPC(a.ctx, &stats.DelayedPickComplete{}) | |||
There was a problem hiding this comment.
Should we also call stats.PickerUpdated to keep the previous behaviour until PickerUpdated is removed?
There was a problem hiding this comment.
It's a type alias so the stats handler will get one event that matches both of the events.
test/end2end_test.go
Outdated
| if pickerUpdatedCount != 1 { | ||
| t.Fatalf("sh.pickerUpdated count: %v, want: %v", pickerUpdatedCount, 2) | ||
| if delayedPickCompleteCount != 1 { | ||
| t.Fatalf("sh.delayedPickComplete count: %v, want: %v", delayedPickCompleteCount, 2) |
There was a problem hiding this comment.
We compare delayedPickCompleteCount to 1, but the log mentions 2. I think the log message needs to be fixed.
Fixes: #8453
This is part of a two-step change to eliminate the potentially-multiple calls for
PickerUpdatedas we do today. Other languages only have a maximum of one event for this, when the pick finally completed, but in Go we implemented it as a call every time the picker was updated. The plan is to implement the full change as follows:RELEASE NOTES:
DelayedPickCompleteevent, a type alias ofPickerUpdated. This (combined) event will now be emitted only once per call, when a transport is successfully selected for the attempt. OpenTelemetry metrics will no longer have multiple "Delayed LB pick complete" events in Go, matching other gRPC languages. A future release will deletePickerUpdated.