transport: Add values to the grpc.disconnect_error label for grpc.subchannel.disconnections metric (A94)#8973
Conversation
mbissa
added
Type: Feature
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8973 +/- ##
==========================================
+ Coverage 83.04% 83.08% +0.03%
==========================================
Files 411 411
Lines 32892 32918 +26
==========================================
+ Hits 27316 27349 +33
+ Misses 4181 4178 -3
+ Partials 1395 1391 -4
🚀 New features to boost your workflow:
|
mbissa
changed the title
mbissa
force-pushed
the
subchannel-disconnection-unknown-reason
branch
from
dedb4b2 to
06fb986
Compare
mbissa
force-pushed
the
subchannel-disconnection-unknown-reason
branch
from
215dc6c to
de97023
Compare
|
/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 successfully implements gRFC A94 by adding more granular grpc.disconnect_error labels to the grpc.subchannel.disconnections metric. The changes are well-implemented, introducing a disconnectError field in addrConn and a disconnectErrorString helper to map various error conditions to the new labels. The transport layer modifications to propagate the necessary error details are correct. The addition of comprehensive end-to-end tests is a great way to ensure the new labels are correctly reported in different disconnection scenarios. I have one minor suggestion to clean up a duplicated comment.
internal/transport/http2_client.go
Outdated
| t.goAwayDebugMessage = fmt.Sprintf("code: %s", f.ErrCode) | ||
| if len(f.DebugData()) > 0 { | ||
| t.goAwayDebugMessage += fmt.Sprintf(", debug data: %q", string(f.DebugData())) |
There was a problem hiding this comment.
Why do we need this change?
There was a problem hiding this comment.
not needed. reverted this part.
clientconn.go
Outdated
|
|
||
| localityLabel string | ||
| backendServiceLabel string | ||
| disconnectError string |
There was a problem hiding this comment.
Nit: Can we add a Label suffix to stay consistent with existing field names.
clientconn.go
Outdated
| } | ||
|
|
||
| func disconnectErrorString(r transport.GoAwayReason, goAwayCode http2.ErrCode, err error) string { | ||
| if r != transport.GoAwayInvalid { |
There was a problem hiding this comment.
Can we use a switch statement here. See: https://go.dev/doc/effective_go#switch
mbissa
force-pushed
the
subchannel-disconnection-unknown-reason
branch
from
1200625 to
5584fb4
Compare
mbissa
force-pushed
the
subchannel-disconnection-unknown-reason
branch
from
f219fe9 to
88cd619
Compare
|
fixed the comments, one test flaked once due to timing of how the connection was closed, so changed the test to be more deterministic. Master branch had new tests which were failing now, so couple of minor changes for that as well. |
2 participants
This PR implements granular grpc.disconnect_error labels for the grpc.subchannel.disconnections metric, as defined in gRFC A94.
RELEASE NOTES: