transport: Add values to the grpc.disconnect_error label for grpc.subchannel.disconnections metric (A94) by mbissa · Pull Request #8973 · grpc/grpc-go

mbissa · 2026-03-13T07:31:35Z

This PR implements granular grpc.disconnect_error labels for the grpc.subchannel.disconnections metric, as defined in gRFC A94.

RELEASE NOTES:

transport: Add disconnection reason to the grpc.disconnect_error label for grpc.subchannel.disconnections metric as defined in gRFC A94.

codecov · 2026-03-13T07:35:16Z

Codecov Report

❌ Patch coverage is 94.28571% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.06%. Comparing base (12e91dd) to head (e30f27d).
⚠️ Report is 15 commits behind head on master.

Files with missing lines	Patch %	Lines
clientconn.go	93.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8973      +/-   ##
==========================================
+ Coverage   83.04%   83.06%   +0.01%     
==========================================
  Files         411      411              
  Lines       32892    32988      +96     
==========================================
+ Hits        27316    27402      +86     
- Misses       4181     4191      +10     
  Partials     1395     1395

Files with missing lines	Coverage Δ
internal/transport/http2_client.go	`92.37% <100.00%> (-0.67%)`	⬇️
internal/transport/transport.go	`89.06% <ø> (-2.09%)`	⬇️
clientconn.go	`90.74% <93.33%> (-0.30%)`	⬇️

... and 38 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mbissa · 2026-03-13T09:52:50Z

/gemini review

gemini-code-assist

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.

mbissa · 2026-03-17T07:40:05Z

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.

mbissa · 2026-03-25T15:15:34Z

I realize the tests are not very idiomatic - I will structure them into a table and push one more commit.

easwars · 2026-03-26T23:02:23Z

+
+func (s) TestDisconnectLabel(t *testing.T) {
+	// 1. Valid GOAWAY
+	// Server GracefulStop sends GOAWAY with active streams = 0.


What does active streams have to do with anything that is happening with regards to this test?

This is just to explain that we straightaway go to close the stream and expect GOAWAY. It does not have specific bearing on the value of the label itself.

Can we please have descriptive comments for each of the three subtests.

And there are no active streams because the runDisconnectLabelTest only performs a unary RPC before invoking the triggerFunc. Yeah, some of things would be nice to be clearly explained in the comments.

My philosophy with tests is that they have to be as readable as possible, so that when someone lands on it (either because they are debugging a failed test or because they are trying to understand how the code being tested works), they should be able to very quickly understand what the test is doing and what it is expecting. The "how" part is usually less important, and as long as the "what"s are clearly documented, the reader will have a much easier time.

easwars · 2026-03-26T23:08:12Z

 	return gotMetrics
 }
+
+func (s) TestDisconnectLabel(t *testing.T) {


There seem to be more disconnect reason labels than what are being tested here. Are looks like they are covered in otel e2e tests? Why do we cover only a subset here?

The goal here is to just check that the plumbing works, e2e tests verify all scenarios.

Can we please add a docstring that mentions this.

easwars · 2026-03-26T23:31:29Z

+	default:
+		var sysErr syscall.Errno
+		if errors.As(err, &sysErr) {
+			return "socket error"
+		}
+		return "unknown"


Nit: If you defined the sysErr variable at the top of the switch, you could add a case for it, instead of folding it into the default case

switch { // Existing cases case errors.As(err, &sysErr): return "socket error" default: return "unknown" }

Not sure if you missed this or decided not to implement it.

missed pushing that change, done now.

easwars

LGTM, modulo some minor comments

easwars · 2026-03-30T22:30:50Z

 	return gotMetrics
 }
+
+func (s) TestDisconnectLabel(t *testing.T) {


Can we please add a docstring that mentions this.

easwars · 2026-03-30T22:36:05Z

+
+func (s) TestDisconnectLabel(t *testing.T) {
+	// 1. Valid GOAWAY
+	// Server GracefulStop sends GOAWAY with active streams = 0.


Can we please have descriptive comments for each of the three subtests.

And there are no active streams because the runDisconnectLabelTest only performs a unary RPC before invoking the triggerFunc. Yeah, some of things would be nice to be clearly explained in the comments.

My philosophy with tests is that they have to be as readable as possible, so that when someone lands on it (either because they are debugging a failed test or because they are trying to understand how the code being tested works), they should be able to very quickly understand what the test is doing and what it is expecting. The "how" part is usually less important, and as long as the "what"s are clearly documented, the reader will have a much easier time.

easwars · 2026-03-30T22:41:35Z

+	default:
+		var sysErr syscall.Errno
+		if errors.As(err, &sysErr) {
+			return "socket error"
+		}
+		return "unknown"


Not sure if you missed this or decided not to implement it.

easwars · 2026-03-30T22:45:08Z

+	// If the connection was closed by a GOAWAY frame, this will usually be a
+	// connection error that describes the connection closing.
+	Err error


I don't completely understand this last sentence. If the transport is being closed becasue of the receipt of a GOAWAY, I see that we usually don't set this field. Is that not true? Can this comment be made more easily understandable. Thanks.

correct, I have fixed the comment.

easwars · 2026-03-30T22:46:32Z

+	if ac.disconnectErrorLabel == "" {
+		ac.disconnectErrorLabel = "subchannel shutdown"
+	}


The above comment was supposed to be on top the line that sets the conenctivity state to Shutdown. Can we continue to retain it that way.

mbissa added Type: Feature New features or improvements in behavior Area: Observability Includes Stats, Tracing, Channelz, Healthz, Binlog, Reflection, Admin, GCP Observability labels Mar 13, 2026

mbissa added this to the 1.81 Release milestone Mar 13, 2026

mbissa changed the title ~~transport: Add values for grpc.disconnect_error label for grpc.subchannel.disconnections metric (A94)~~ transport: Add values to the grpc.disconnect_error label for grpc.subchannel.disconnections metric (A94) Mar 13, 2026

mbissa force-pushed the subchannel-disconnection-unknown-reason branch from dedb4b2 to 06fb986 Compare March 13, 2026 07:42

disconnect reason for subchannel metrics (A94)

de97023

mbissa force-pushed the subchannel-disconnection-unknown-reason branch from 215dc6c to de97023 Compare March 13, 2026 09:36

gemini-code-assist Bot reviewed Mar 13, 2026

View reviewed changes

Comment thread internal/transport/http2_client.go

revie comment

4bdc057

mbissa requested a review from easwars March 13, 2026 10:41

easwars self-assigned this Mar 16, 2026

easwars reviewed Mar 16, 2026

View reviewed changes

Comment thread internal/transport/http2_client.go Outdated

Comment thread clientconn.go Outdated

Comment thread clientconn.go Outdated

easwars assigned mbissa and unassigned easwars Mar 16, 2026

mbissa and others added 2 commits March 17, 2026 06:24

review comments

dcaf507

Merge branch 'grpc:master' into subchannel-disconnection-unknown-reason

5584fb4

mbissa force-pushed the subchannel-disconnection-unknown-reason branch from 1200625 to 5584fb4 Compare March 17, 2026 06:47

mbissa added 2 commits March 17, 2026 06:49

fixing merge failures

8eab968

make the test deterministic by ensuring correct listener behaviour

88cd619

mbissa force-pushed the subchannel-disconnection-unknown-reason branch from f219fe9 to 88cd619 Compare March 17, 2026 07:32

mbissa assigned easwars and unassigned mbissa Mar 17, 2026

mbissa and others added 2 commits March 18, 2026 13:54

Merge branch 'grpc:master' into subchannel-disconnection-unknown-reason

2dacd03

changing existing tests to adapt for new changes in race

8907a8e

easwars reviewed Mar 23, 2026

View reviewed changes

easwars assigned mbissa and unassigned easwars Mar 23, 2026

mbissa added 4 commits March 24, 2026 19:02

handling fallback socket errors

62a1651

bundling disconnection error type and reasons into a single struct

d475eab

fixing the test

b2a448a

review comments

27c8365

mbissa assigned easwars and unassigned mbissa Mar 25, 2026

making the test more idiomatic

4e836ea

mbissa assigned easwars and unassigned easwars Mar 25, 2026

easwars reviewed Mar 26, 2026

View reviewed changes

easwars assigned mbissa and unassigned easwars Mar 26, 2026

review comments

788d889

mbissa assigned easwars and unassigned mbissa Mar 30, 2026

easwars approved these changes Mar 30, 2026

View reviewed changes

easwars assigned mbissa and unassigned easwars Mar 30, 2026

review comments

e30f27d

mbissa merged commit 34da8d0 into grpc:master Mar 31, 2026
13 of 14 checks passed

Conversation

mbissa commented Mar 13, 2026

Uh oh!

codecov Bot commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mbissa commented Mar 13, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mbissa commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mbissa commented Mar 25, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbissa Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

easwars left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented Mar 13, 2026 •

edited

Loading

mbissa commented Mar 17, 2026 •

edited

Loading

mbissa Mar 30, 2026 •

edited

Loading