Adding interceptors to TransportChannelProvider

[nwbirnie]

Interceptors currently have to be specified on an implementation of TransportChannelProvider, e.g. InstantiatingGrpcChannelProvider.

What do you think about adding methods to TransportChannelProvider to set interceptors? The idea being to write unit tests which replacing InstantiatingGrpcChannelProvider with LocalChannelProvider, and still being able to test interceptors all through the stack? We might need some convenience methods for test asserts in LocalChannelProvider as well.

Here are the current implementations (in GAX):

LocalChannelProvider (com.google.api.gax.grpc.testing) InstantiatingGrpcChannelProvider (com.google.api.gax.grpc) FixedTransportChannelProvider (com.google.api.gax.rpc) InstantiatingHttpJsonChannelProvider (com.google.api.gax.httpjson)

LocalChannelProvider and InstantiatingGrpcChannelProvider can both implement these methods.

FixedTransportChannelProvider should probably return false for needsInterceptors() and throw on withInterceptors().

InstantiatingHttpJsonChannelProvider currently doesn't support interceptors, and I didn't see any support drilling down to HttpRequestRunnable. Maybe should do the same as above for FixedTransportChannelProvider?

[vam-google]

Hi @nwbirnie, This looks doable, since TransportChannelProvider is @InternalExtensionOnly and GrpcInterceptorProvider and LocalChannelProvider are @BetaApi (so we can add extensions without formally breaking backward compatibility).

At the same time, this will also require "shading" io.grpc.ClientInterceptor (wrapping it in some other class, so TransportChannelProvider can stay unaware of grpc), which is going to look sloppy. Or, making some empty TransportInterceptorProvider (without methods) and do explicit casting inside implementations, which is also bad oop design. There are must be other options as well but I can't think of any better ones right now.

So if there is a strong reason to do it we definitely can do it, but otherwise I think it would be better to not do it as it somewhat worsens gax's surface.

[nwbirnie]

So the motivation here is testing. I'm using MockServiceHelper and LocalChannelProvider to test headers being sent in RPCs. I'd like to expand the test scope to include interceptors. Is there another way to test that interceptors are being invoked in unit tests?

In terms of the shading approach, I don't think this is so bad - we're effectively doing the same thing for headers/endpoints etc. except those happen to be native types. Increasing decoupling and adding features to GAX seems like a good option in general.

[meltsufin]

@vam-google Do you have any updated thoughts on this?