xds/internal/client: Add async gauge metrics for Connected and Resources (A78) #8807

mbissa · 2026-01-07T01:18:21Z

This PR leverages the async gauge framework and implements the xdsclient metrics to report number of xds resources and whether or not the xDS client currently has a working ADS stream to the xDS server along with the required labels as part of A78

RELEASE NOTES:

xds/internal/client: Add async gauge metrics for "grpc.xds_client.connected" and "grpc.xds_client.resources" as part of A78.

codecov · 2026-01-07T01:22:10Z

Codecov Report

❌ Patch coverage is 74.22680% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.23%. Comparing base (c6d5e5e) to head (ec83a4e).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
internal/xds/xdsclient/clientimpl.go	25.92%	19 Missing and 1 partial ⚠️
internal/xds/clients/xdsclient/xdsclient.go	92.42%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8807      +/-   ##
==========================================
- Coverage   83.26%   83.23%   -0.03%     
==========================================
  Files         418      418              
  Lines       33004    33101      +97     
==========================================
+ Hits        27480    27551      +71     
- Misses       4109     4135      +26     
  Partials     1415     1415

Files with missing lines	Coverage Δ
internal/xds/clients/xdsclient/ads_stream.go	`85.06% <100.00%> (+0.21%)`	⬆️
internal/xds/clients/xdsclient/xdsclient.go	`86.61% <92.42%> (+2.03%)`	⬆️
internal/xds/xdsclient/clientimpl.go	`73.72% <25.92%> (-11.74%)`	⬇️

... and 29 files with indirect coverage changes

🚀 New features to boost your workflow:

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

easwars · 2026-01-09T00:27:00Z

internal/xds/clients/xdsclient/metrics/metrics.go

+// Value is 1 if connected, 0 otherwise.
+type XDSClientConnected struct {
+	ServerURI string
+	Value     int64


Can this not be of type bool?

easwars · 2026-01-09T00:29:58Z

internal/xds/clients/xdsclient/metrics/metrics.go

+	Authority    string
+	ResourceType string
+	CacheState   string
+	Count        int64


I understand that the previously defined metric structs did not document the fields. But I feel we should do one of the following:

trialing line comments for all of the fields

a package level comment that talks about every single field

a package level comment pointing to the section in A78 that contains a description of the labels, which is what these fields correspond to

easwars · 2026-01-09T00:30:49Z

internal/xds/clients/xdsclient/metrics/metrics.go

 	ServerURI string
 }
+
+// XDSClientConnected reports the connectivity state of the xDS stream.


Nit: s/xDS stream/ADS stream/

FYI: The xDS client establishes two streams, ADS and LRS.

easwars · 2026-01-09T00:36:17Z

internal/xds/xdsclient/clientimpl.go

 	})
+	xdsClientConnectedMetric = estats.RegisterInt64AsyncGauge(estats.MetricDescriptor{
+		Name:        "grpc.xds_client.connected",
+		Description: "Experimental. 1 if the xDS stream is connected, 0 otherwise.",


Why are only the newly added metrics "experimental"? Have the other three already been stabilized?

easwars · 2026-01-09T00:39:00Z

internal/xds/xdsclient/clientimpl.go

+	xdsClientResourcesMetric = estats.RegisterInt64AsyncGauge(estats.MetricDescriptor{
+		Name:        "grpc.xds_client.resources",
+		Description: "Experimental. Number of xDS resources currently cached.",
+		Unit:        "{resource}",


What does it mean to have a {resource} unit?

easwars · 2026-02-06T19:12:30Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces asynchronous gauge metrics for tracking xDS client connectivity and resource counts, which is a valuable addition for observability. The implementation looks mostly solid, but I've found a critical issue in the connectivity metric logic that causes it to report incorrect status. I've also included a couple of medium-severity suggestions to improve test coverage and code clarity. Please see the detailed comments below.

gemini-code-assist · 2026-02-06T19:16:00Z

internal/xds/clients/xdsclient/xdsclient.go

+		if cs.channel.ads != nil && cs.channel.ads.fc != nil && !cs.channel.ads.fc.isStopped() {
+			val = 1
+		}


The logic to determine if the xDS client is connected is incorrect. The condition !cs.channel.ads.fc.isStopped() checks if the adsStreamImpl has been stopped, which only happens when the xdsChannel is closed (e.g., when XDSClient.Close() is called).

An xdsChannel can be active while the underlying ADS stream is down (e.g., during connection backoff after a stream failure). In such cases, this logic will incorrectly report Value: 1 (connected), even though there is no active stream to the management server.

The metric's description is "1 if the xDS stream is connected, 0 otherwise." The implementation should reflect the actual state of the underlying stream.

To fix this, adsStreamImpl would need to maintain a state indicating whether a stream is currently established, and reportConnectedState should query that state. This would likely involve adding an atomic boolean to adsStreamImpl that is updated as the stream is created and terminated.

This is correct.

Here is where the ADS stream is created:

grpc-go/internal/xds/clients/xdsclient/ads_stream.go

Line 241 in 0f69b6e

stream, err := s.transport.NewStream(ctx, "/envoy.service.discovery.v3.AggregatedDiscoveryService/StreamAggregatedResources")

And here is where we know the previously created stream is no longer active:

grpc-go/internal/xds/clients/xdsclient/ads_stream.go

Line 265 in 0f69b6e

if s.recv(stream) {

Also, note that if the underlying connection is broken, new stream creation will fail:

grpc-go/internal/xds/clients/xdsclient/ads_stream.go

Line 243 in 0f69b6e

s.logger.Warningf("Failed to create a new ADS streaming RPC: %v", err)

gemini-code-assist · 2026-02-06T19:16:00Z

internal/xds/clients/xdsclient/test/metrics_test.go

+	if err := tmr.waitForSpecificMetric(ctx, &metrics.ServerFailure{ServerURI: mgmtServer.Address}); err != nil {
+		t.Fatal(err.Error())
+	}


The test verifies that the XDSClientConnected metric reports a value of 1 when connected. However, after the management server is stopped and a ServerFailure metric is received, the test does not verify that the XDSClientConnected metric subsequently reports 0 for the disconnected state.

Adding this check would make the test more comprehensive and would have helped identify the issue in reportConnectedState where the disconnected state is not being reported correctly.

Suggested change

if err := tmr.waitForSpecificMetric(ctx, &metrics.ServerFailure{ServerURI: mgmtServer.Address}); err != nil {

t.Fatal(err.Error())

}

if err := tmr.waitForSpecificMetric(ctx, &metrics.ServerFailure{ServerURI: mgmtServer.Address}); err != nil {

t.Fatal(err.Error())

}

tmr.triggerAsyncMetrics()

if err := tmr.waitForSpecificMetric(ctx, &metrics.XDSClientConnected{ServerURI: mgmtServer.Address, Value: 0}); err != nil {

t.Fatal(err.Error())

}

I haven't reviewed the tests yet and don't plan to review them until the non-test code looks good. So, please check if this comment is valid and act on it if required.

gemini-code-assist · 2026-02-06T19:16:00Z

internal/xds/xdsclient/clientimpl.go

+	switch m := metric.(type) {
+	case *metrics.XDSClientConnected:
+		// Record: grpc.xds_client.connected
+		// Labels: grpc.target, grpc.xds.authority


The comment here states that one of the labels is grpc.xds.authority. However, the xdsClientConnectedMetric is defined with the label grpc.xds.server, and the code correctly passes m.ServerURI for it. To avoid confusion, the comment should be updated to match the metric definition.

Suggested change

// Labels: grpc.target, grpc.xds.authority

// Labels: grpc.target, grpc.xds.server

easwars · 2026-02-07T06:08:33Z

internal/xds/clients/config.go

 	ReportMetric(metric any)
+
+	// RegisterAsyncReporter registers a reporter to produce metric values for
+	// only the listed descriptors. The returned function must be called when


Where are these descriptors listed? Can that be specified in this docstring?

easwars · 2026-02-07T06:10:59Z

internal/xds/clients/config.go

+	RegisterAsyncReporter(reporter AsyncReporter) func()
+}
+
+// AsyncReporter is an interface for types that record metrics asynchronously.


Nit: Can this line be replaced with:
// AsyncReporter allows implementations to record metrics asynchronously

The fact that it is an interface is clear by the definition of the type. So, it need not be specified in the docstring.

easwars · 2026-02-07T06:17:14Z

internal/xds/clients/config.go

+
+	// RegisterAsyncReporter registers a reporter to produce metric values for
+	// only the listed descriptors. The returned function must be called when
+	// the metrics are no longer needed, which will remove the reporter.


which will remove the reporter

Does this guarantee that once the returned cancel func is called, the Report method on the registered AsyncReporter is never called? If so, does it make sense to mention that in the docstring.

FYI: Even though this is an internal package, this xDS client is used by non-grpc folks, and we might fork this off to a separate repo or sub-module at some point in time. So, the requirements for docstrings here should be the same as that of any public APIs in our repo.

easwars · 2026-02-07T06:19:17Z

internal/xds/clients/config.go

+	// ReportMetric reports a metric. The metric will be one of the predefined
+	// set of types in the metrics.go file.
+	ReportMetric(metric any)


What happens if this method is called and passed a metric that is not supposed to be reported asynchronously?

easwars · 2026-02-07T06:29:53Z

internal/xds/xdsclient/clientimpl.go

+// RegisterAsyncReporter adapts the generic clients.AsyncReporter to the
+// estats.AsyncMetricReporter interface and registers it.
+func (mr *metricsReporter) RegisterAsyncReporter(reporter clients.AsyncReporter) func() {
+	if mr.recorder == nil {


Should we handle the case of reporter being nil here as well?

easwars · 2026-02-07T06:54:07Z

internal/xds/clients/xdsclient/test/metrics_test.go

+	if err := tmr.waitForSpecificMetric(ctx, &metrics.ServerFailure{ServerURI: mgmtServer.Address}); err != nil {
+		t.Fatal(err.Error())
+	}


I haven't reviewed the tests yet and don't plan to review them until the non-test code looks good. So, please check if this comment is valid and act on it if required.

easwars · 2026-02-07T06:55:13Z

internal/xds/xdsclient/clientimpl.go

+	switch m := metric.(type) {
+	case *metrics.XDSClientConnected:
+		// Record: grpc.xds_client.connected
+		// Labels: grpc.target, grpc.xds.authority


easwars · 2026-02-07T06:56:44Z

internal/xds/clients/xdsclient/xdsclient.go

+
+	// Schedule the operation.
+	// If the serializer is closed/context canceled, the second func (onFailure) runs.
+	a.xdsClientSerializer.ScheduleOr(op, func() {


Any reason why the TrySchedule method on the serializer cannot be used here?

easwars · 2026-02-07T06:59:49Z

internal/xds/clients/xdsclient/xdsclient.go

+	default:
+		// Fallback for initialization states
+		return "requested"
+	}


I don't see this being specified in the gRFC. Am I missing something?

easwars · 2026-02-07T07:00:20Z

internal/xds/clients/xdsclient/xdsclient.go

+	switch r.md.Status {
+	case xdsresource.ServiceStatusRequested:
+		return "requested"
+


Please don't add newlines between case statements. That is not Go style. It makes the whole switch statement unnecessarily long.

a78-xds-client-metrics-connected-resources

ec83a4e

mbissa added this to the 1.79 Release milestone Jan 7, 2026

mbissa requested a review from easwars January 7, 2026 01:18

mbissa self-assigned this Jan 7, 2026

mbissa added Type: Feature New features or improvements in behavior Area: xDS Includes everything xDS related, including LB policies used with xDS. labels Jan 7, 2026

mbissa assigned easwars Jan 7, 2026

Pranjali-2501 modified the milestones: 1.79 Release, 1.80 Release Jan 21, 2026

easwars reviewed Feb 5, 2026

View reviewed changes

gemini-code-assist bot reviewed Feb 6, 2026

View reviewed changes

easwars reviewed Feb 7, 2026

View reviewed changes

easwars removed their assignment Feb 7, 2026

	// Labels: grpc.target, grpc.xds.authority
	// Labels: grpc.target, grpc.xds.server

xds/internal/client: Add async gauge metrics for Connected and Resources (A78) #8807

Are you sure you want to change the base?

xds/internal/client: Add async gauge metrics for Connected and Resources (A78) #8807

Uh oh!

Conversation

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

Uh oh!

Uh oh!

codecov bot commented Jan 7, 2026

Codecov Report

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!

easwars commented Feb 6, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Feb 6, 2026

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mbissa commented Jan 7, 2026 •

edited

Loading