envoy-mobile
envoy-mobile copied to clipboard
envoyproxy
Okhttp interceptor location?
Thanks to Yuri getting the interceptor working over in https://github.com/square/okhttp/pull/7231 there was a question raised in the weekly meeting if this should live in the E-M repo or android-test over in okhttp. E-M devs were enthusiastically welcoming it in our repo but we were missing any input from okhttp folks. @yschimke can we get your thoughts?
It's just a test in the okhttp repo. If you want it to become a real thing, battle hardened and so on, I suggest hosting it in the EM repo and publishing from there.
SGTM. We'll see if we can figure out how to test it over on our side of things then :-)
Potential limit w/ this approach is: this EM logic has to be via an okhttp app interceptor as network interceptor would not allow short-circuiting. This might be fine for apps that don't have their own network interceptors(i.e., network monitoring involves connection) or they might want to fork okhttp
@woshimaliang I'd strongly advise against forking OkHttp to try to allow network interceptors.
https://square.github.io/okhttp/features/interceptors/#choosing-between-application-and-network-interceptors
- Able to operate on intermediate responses like redirects and retries.
- Not invoked for cached responses that short-circuit the network.
- Observe the data just as it will be transmitted over the network.
- Access to the Connection that carries the request.
We discussed the OkHttp approach to such bridging interceptors and the approach we decided internally, was to assume that once you adopt EnvoyMobile, we would expect you to want to use the network monitoring from EnvoyMobile.
Most useful uses of interceptors for things like auth will be app interceptors.
I was using app interceptor as well :) just wanted to point out that people would need to migrate their own network interceptors or anything relies on those built-in interceptors(retry, cache, etc but they're provided by EM already). Not a new problem for EM though. actually the same for for okhttp+cronet approach.
Another limit with app interceptor is that people often do baseOkHttpClient.newBuild().addInterceptor(their_new interceptor_for_their_okhttpclient).build() so they would need to make sure Evnoy interceptor is the last one, so it has to be registered per okhttp client, but not on baseOkHttpClient
@woshimaliang good point about the ordering... Not sure about a good solution here. It's not fatal issue, just something that will be an unexpected surprise. ordering was always something you need to consider.
One other thing to consider is Duplex Streams https://square.github.io/okhttp/4.x/okhttp/okhttp3/-request-body/is-duplex/#duplex-transmission
Which seem similar to BidirectionalStreams?
Probably important for clients using gRPC like Wire.