xds: Add counter and gauge metrics by DNVindhya · Pull Request #11661 · grpc/grpc-java · GitHub

This PR implements xDS client defined in A78.

Counters

grpc.xds_client.server_failure
grpc.xds_client.resource_updates_valid
grpc.xds_client.resource_updates_invalid

Gauges

grpc.xds_client.connected
grpc.xds_client.resources

The grpc.xds.authority label is missing, and will be added in a later PR.

Sending what I have.

…it tests

Is this working or non-errored?

I used working as it was used in metric description of grpc.xds_client.connected.
Also, I think it is more nuanced than non-errored, because if there is an error on ADS stream and / or close before receiving a response, it is an error and value will remain false until ADS stream receives a response to say it has a working stream to communicate with server.
Added link to A78 which has definition for working stream. Let me know what you think.

It isn't the dictionary definition of "working", but you're right that it is the proposal's definition. I'd be happier if you put quotes around working, but it's up to you.

Added quotes around working.

… signature and addressed review comments.

…mments

…MetricReporterImpl and addressed review comments

Given that the created response handler just calls the methods in this class without doing any processing, what is the point of creating a new anonymous class and object instead of just passing this?

Counter of xDS servers going from healthy to unhealthy metric (grpc.xds_client.server_failure) is reported as part of handleStreamClosed and the metric needs grpc.xds.server as an attribute value.
Now with the help of anonymous class we are able to create a XdsResponseHandler instance for every ControlPlaneClient and provide ControlPlaneClient's ServerInfo to XdsClientImpl.this.handleStreamClosed.

…a.cached value

… and addressed review comments

private or package-private? Or just inline it? It doesn't get any benefit from being here instead of the parent class.

Updated to package-private.

… Nacked resource

@ejona86 and @larry-safran thanks for both of your approvals. I have addressed all your comments. If you don't have any new/pending comments can I go ahead and merge?

Once you have approvals from everyone, you can go ahead a merge.

…

On Mon, Nov 25, 2024 at 3:06 PM Vindhya Ningegowda ***@***.***> wrote: @ejona86 <https://github.com/ejona86> and @larry-safran <https://github.com/larry-safran> thanks for both of your approvals. I have addressed all your comments. If you don't have any new/pending comments can I go ahead and merge? — Reply to this email directly, view it on GitHub <#11661 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AZQMCXTO7DVWXV7XYA4NHNT2COUQFAVCNFSM6AAAAABRBI57B6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIOJZGIYTINBWGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Adds the following xDS client metrics defined in [A78](https://github.com/grpc/proposal/blob/master/A78-grpc-metrics-wrr-pf-xds.md#xdsclient). Counters - grpc.xds_client.server_failure - grpc.xds_client.resource_updates_valid - grpc.xds_client.resource_updates_invalid Gauges - grpc.xds_client.connected - grpc.xds_client.resources

DNVindhya added 3 commits November 1, 2024 18:10

Rebase master

d085aca

Rebase grpc#2

ef7bfcd

fix javadoc

f60c985

DNVindhya requested review from ejona86 and larry-safran November 2, 2024 02:38

ejona86 reviewed Nov 5, 2024

View reviewed changes

Instead of storing MetricRecorder, pass it as is for XdsClient creation

4f01651

ejona86 reviewed Nov 11, 2024

View reviewed changes

Comment thread xds/src/main/java/io/grpc/xds/XdsClientMetricReporterImpl.java Outdated

Comment thread xds/src/test/java/io/grpc/xds/XdsClientMetricReporterImplTest.java Outdated

Move interfaces for reporting gauge values to XdsClient and update un…

5881cb6

…it tests

larry-safran reviewed Nov 12, 2024

View reviewed changes

Updated ServerConnectionCallback#reportServerConnectionGauge method…

4831431

… signature and addressed review comments.

ejona86 reviewed Nov 12, 2024

View reviewed changes

Inject value from XdsClientMetricReporterImpl and addressed review co…

cbe2659

…mments

ejona86 reviewed Nov 14, 2024

View reviewed changes

DNVindhya added 2 commits November 19, 2024 17:11

Moved resource count metric reporting from XdsClientImpl to XdsClient…

3a283e4

…MetricReporterImpl and addressed review comments

fix typo

fb71c60

larry-safran approved these changes Nov 20, 2024

View reviewed changes

DNVindhya added 2 commits November 20, 2024 13:23

address review comment for javadoc

80a9c40

Updated verifyResourceMetadataNacked method to verify ResourceMetadat…

e04164b

…a.cached value

ejona86 approved these changes Nov 20, 2024

View reviewed changes

Return Future instead of SettableFuture for reportServerConnections()…

7dd4402

… and addressed review comments

ejona86 reviewed Nov 21, 2024

View reviewed changes

Added unit test to verify ResourceMetadata.isCached() set to false on…

fc60ef9

… Nacked resource

DNVindhya merged commit 20d09ce into grpc:master Nov 26, 2024

DNVindhya mentioned this pull request Nov 27, 2024

xds: Add grpc.target value for xDS enabled server metrics #11715

Merged

DNVindhya deleted the xds-client-metrics-structure-1 branch November 27, 2024 18:17

DNVindhya mentioned this pull request Dec 17, 2024

xds: Remove xds authority label from metric registration #11760

Merged

DNVindhya mentioned this pull request Jan 14, 2025

xds: Remove xds authority label from metric registration (v1.69.x backport) #11826

Merged

github-actions Bot locked as resolved and limited conversation to collaborators Feb 26, 2025

                 /**
-                 * Reports whether xDS client has a working ADS stream to xDS server. Reporting is done through
-                 * {@link CallbackMetricReporter}.
+                 * Reports whether xDS client has a working ADS stream to xDS server.

                       xdsTransport,
                       serverInfo,
                       bootstrapInfo.node(),
-                      this,

                   // TODO(dnvindhya): include the "authority" label once xds.authority is available.
-                  @Override
                   public void reportResourceCountGauge(long resourceCount, String cacheState,

xds: Add counter and gauge metrics by DNVindhya · Pull Request #11661 · grpc/grpc-java · GitHub

Uh oh!

xds: Add counter and gauge metrics - #11661

xds: Add counter and gauge metrics #11661DNVindhya merged 13 commits intogrpc:mastergrpc/grpc-java:masterfrom DNVindhya:xds-client-metrics-structure-1DNVindhya/grpc-java:xds-client-metrics-structure-1Copy head branch name to clipboard

Conversation

DNVindhya commented Nov 2, 2024

Uh oh!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

DNVindhya commented Nov 25, 2024

Uh oh!

larry-safran commented Nov 26, 2024 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

xds: Add counter and gauge metrics #11661
DNVindhya merged 13 commits into
grpc:masterfrom
DNVindhya:xds-client-metrics-structure-1