xds: Add counter and gauge metrics #11661
Conversation
xds/src/test/java/io/grpc/xds/XdsClientMetricReporterImplTest.java
Outdated
Show resolved
Hide resolved
| throw new UnsupportedOperationException(); | ||
| } | ||
|
|
||
| /** | ||
| * 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. |
There was a problem hiding this comment.
Is this working or non-errored?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Added quotes around working.
xds/src/test/java/io/grpc/xds/XdsClientMetricReporterImplTest.java
Outdated
Show resolved
Hide resolved
… signature and addressed review comments.
…MetricReporterImpl and addressed review comments
| @@ -394,7 +394,27 @@ public ControlPlaneClient getOrCreateControlPlaneClient(ServerInfo serverInfo) { | |||
| xdsTransport, | |||
| serverInfo, | |||
| bootstrapInfo.node(), | |||
| this, | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
… and addressed review comments
| @@ -203,7 +200,6 @@ static final class MetricReporterCallback implements ResourceCallback, | |||
| } | |||
|
|
|||
| // TODO(dnvindhya): include the "authority" label once xds.authority is available. | |||
| @Override | |||
| public void reportResourceCountGauge(long resourceCount, String cacheState, | |||
There was a problem hiding this comment.
private or package-private? Or just inline it? It doesn't get any benefit from being here instead of the parent class.
There was a problem hiding this comment.
Updated to package-private.
|
@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:
***@***.***>
|
This PR implements xDS client defined in A78.
Counters
Gauges
The
grpc.xds.authoritylabel is missing, and will be added in a later PR.