Add ClientTransportFilter

[joybestourous]

Problem
We're trying to add some stats on the number of connections opened, closed, and active on the client side for grpc-java. On the server side, ServerTransportFilter acts as a reasonable hook for this, but there doesn't seem to be anything comparable on the client side that I can find (if there is, please direct me to it!).

Closes #10647

Solution
Add a ClientTransportFilter. Similarly to ServerTransportFilter this will provide an observability hook and allows direct modification of the transport's attributes.
To wire this in, this PR also adds a filterTransport method to ManagedClientTransport.Listener which handles modifying the transport's attributes. This is called just before transportReady

[sergiitk]

Same as the other comment:

This should be a new ticket with "experimental API" label. Example: #10108.

I think it'd even make sense to use the same ticket to track both ClientTransportHook interface, and channel builder's addTransportHook().

@ejona86 - correct me if I'm wrong here

[joybestourous]

👋 @ejona86 any update on this PR? is this still on track for v61 release?

[ejona86]

Sorry I went silent on this. I had been hoping to sneak it into 1.60 but then issues with the release cropped up that ended up taking a lot of my time. (I actually didn't see your comment two days ago either...)

We discussed in our API review and the general verdict was "make it a filter like on server-side." Having it just be a hook caused a whole slew of naming/identity/purpose crises that are all solved by mirroring the existing server-side API. Before my time evaporated I was going to actually make the necessary plumbing change...

The listener could receive and return Attributes, just like is done on server-side. In OkHttpClientTransport, BinderTransport, InProcessTransport, CronetClientTransport look to be pretty easy to support (some places attributes would no longer be final; that's okay). NettyClientHandler could pass the Attributes through ClientTransportLifecycleManager.nofiyReady() and update the attributes on return.

But I think that would be inherently racy, in a way not present on server-side. transportReady() is when the client can start sending RPCs on the transport, so we'd need to synchronize the attribute update. On server-side, the server's transport thread can update the attributes before processing any RPCs from the connection.

So instead of adding Attributes to transportReady(), let's add a new method to ManagedClientTransport.Listener just to do this filtering. It can be called just before transportReady(). With a default implementation that just echos back the same attributes, I hope there's not even many tests that have to be updated.

[ejona86]

(FYI, the branch cut for 1.61 is a week earlier than normal, or maybe a bit more. So it is coming up soon. I'd be fine backporting this to the release, even though it is technically a feature, if we can do so at least a week before the release. If there's any bugs, seem they should only impact you and not destabilize others.)

[joybestourous]

Sorry I went silent on this. I had been hoping to sneak it into 1.60 but then issues with the release cropped up that ended up taking a lot of my time. (I actually didn't see your comment two days ago either...)

We discussed in our API review and the general verdict was "make it a filter like on server-side." Having it just be a hook caused a whole slew of naming/identity/purpose crises that are all solved by mirroring the existing server-side API. Before my time evaporated I was going to actually make the necessary plumbing change...

The listener could receive and return Attributes, just like is done on server-side. In OkHttpClientTransport, BinderTransport, InProcessTransport, CronetClientTransport look to be pretty easy to support (some places attributes would no longer be final; that's okay). NettyClientHandler could pass the Attributes through ClientTransportLifecycleManager.nofiyReady() and update the attributes on return.

But I think that would be inherently racy, in a way not present on server-side. transportReady() is when the client can start sending RPCs on the transport, so we'd need to synchronize the attribute update. On server-side, the server's transport thread can update the attributes before processing any RPCs from the connection.

So instead of adding Attributes to transportReady(), let's add a new method to ManagedClientTransport.Listener just to do this filtering. It can be called just before transportReady(). With a default implementation that just echos back the same attributes, I hope there's not even many tests that have to be updated.

Thanks for getting back to me! I addressed these suggestions, ptal and let me know if i understood you correctly. As for backporting this to 1.61, I think we're fine with waiting for 1.62 if you folks are at all worried about the risk. Thanks again!

[ejona86]

This looks good to me, other than some nits.

[sergiitk]

/tmpfs/src/github/grpc-java/cronet/src/main/java/io/grpc/cronet/CronetClientTransport.java:172: error: cannot assign a value to final variable attrs
        attrs = CronetClientTransport.this.listener.filterTransport(attrs);
        ^

[larry-safran]

Note to build the android related classes (which is what is catching this), use `-PskipAndroid=false`

…

On Tue, Dec 26, 2023 at 3:17 PM Sergii Tkachenko ***@***.***> wrote: /tmpfs/src/github/grpc-java/cronet/src/main/java/io/grpc/cronet/CronetClientTransport.java:172: error: cannot assign a value to final variable attrs attrs = CronetClientTransport.this.listener.filterTransport(attrs); ^ — Reply to this email directly, view it on GitHub <#10646 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AZQMCXWTIFHPVHQX77ROTVLYLNLHPAVCNFSM6AAAAAA64ZPITOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRZHAYTMMZWGM> . You are receiving this because your review was requested.Message ID: ***@***.***>

[larry-safran]

@joybestourous The builds failed because of CronetClientTransportTest. That test uses a mock for clientTransportListener which by default returns null for all of the methods. You will need to put a when clause into setUp() so that clientTransportListener.filterTransport(attr) just returns its argument.

For example:

when(clientTransportListener.filterTransport(any()).thenAnswer(i -> i.getArguments()[0])

[joybestourous]

@joybestourous The builds failed because of CronetClientTransportTest. That test uses a mock for clientTransportListener which by default returns null for all of the methods. You will need to put a when clause into setUp() so that clientTransportListener.filterTransport(attr) just returns its argument.

For example:

when(clientTransportListener.filterTransport(any()).thenAnswer(i -> i.getArguments()[0])

Thanks for bringing this to completion 🙏 sorry I missed these errors my android setup was not working properly.

[larry-safran]

No Problem. Thanks for creating this and taking care of all of the other issues.

…

On Wed, Jan 3, 2024 at 11:20 AM joybestourous ***@***.***> wrote: @joybestourous <https://github.com/joybestourous> The builds failed because of CronetClientTransportTest. That test uses a mock for clientTransportListener which by default returns null for all of the methods. You will need to put a when clause into setUp() so that clientTransportListener.filterTransport(attr) just returns its argument. For example: when(clientTransportListener.filterTransport(any()).thenAnswer(i -> i.getArguments()[0]) Thanks for bringing this to completion 🙏 sorry I missed these errors my android setup was not working properly. — Reply to this email directly, view it on GitHub <#10646 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AZQMCXR3L3PK45TYNWWB66TYMWVQRAVCNFSM6AAAAAA64ZPITOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZVHA2TANRZGU> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[ejona86]

#10805 is suspicious, and shouldn't be necessary. I feel like there's no harm happening at present with making the field volatile, but the fact TSAN is finding issues makes clear something is happening that we don't expect. So we aren't going to backport this into the 1.61 release. We'll probably try to remove the volatile before 1.62, but even if we don't, at that point it will have seen enough testing that we are then more confident it won't cause an issue.

[joybestourous]

#10805 is suspicious, and shouldn't be necessary. I feel like there's no harm happening at present with making the field volatile, but the fact TSAN is finding issues makes clear something is happening that we don't expect. So we aren't going to backport this into the 1.61 release. We'll probably try to remove the volatile before 1.62, but even if we don't, at that point it will have seen enough testing that we are then more confident it won't cause an issue.

Oh hm that's super surprising / suspicious. Makes complete sense to put this in 1.62. Thanks for the heads up!