Add ConnectionObserver.ProxyConnectObserver
#2711
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
daschl
reviewed
Sep 26, 2023
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/ProxyConnectChannelSingle.java
Show resolved
Hide resolved
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/AlpnChannelSingle.java
Outdated
Show resolved
Hide resolved
...-http-netty/src/main/java/io/servicetalk/http/netty/ProxyConnectLBHttpConnectionFactory.java
Outdated
Show resolved
Hide resolved
idelpivnitskiy
force-pushed
the
ProxyConnectObserver
branch
from
17f14ab to
a180235
Compare
daschl
approved these changes
Sep 29, 2023
| @@ -71,6 +72,11 @@ public void onFlush() { | |||
| public void onTransportHandshakeComplete() { | |||
| } | |||
|
|
|||
| @Override | |||
There was a problem hiding this comment.
Maybe for a follow-up pr: but why do we have NoopTransportObservers in transport internal and transport api? Maybe we can make the internal one use the one in API?
There was a problem hiding this comment.
The original motivation was to avoid null-checks in our internal codebase, but overtime we found it not user-friendly to always require to return an observer. Users struggle to understand what they suppose to return when they are not interested in callbacks of sub-observer. Even if we make it public in transport-api, it's still hard to discover and easy to return your own, which will not give expected reduction of the overhead. The long-term plan is to make all callbacks that return another observer @Nullable.
Motivation: After apple#2699 implemented HTTP proxy CONNECT with ALPN, it made observability confusing. It broke the contract that only one `connectionEstablished` method can be invoked, which happens when ALPN switches from HTTP/1.1 to HTTP/2 pipeline. Also, for a proxy tunnel with HTTP/1.1 `ConnectionObserver` events were always confusing: users had to wait for `handshakeComplete` after `connectionEstablished` to report that the connection is "ready". Modifications: - Add `ConnectionObserver.ProxyConnectObserver` that reports events related to `CONNECT` phase that starts right after `onTransportHandshakeComplete`; - Refactor `PipelinedLBHttpConnectionFactory` to defer invocation of `connectionEstablished`/`multiplexedConnectionEstablished` until after the handshake is complete to make it consistent with non-proxied connections and make sure only one of the "established" methods is invoked; - Removed `ProxyConnectLBHttpConnectionFactoryTest` because it targeted code paths that do not exist anymore. Instead, enhanced `HttpsProxyTest` to cover similar use-cases. - Enhanced `ProxyTunnel` to allow behavior customization; Result: Enhanced observability for proxy tunnels, `ConnectionObserver` behavior is consistent between proxied and non-proxied use-cases.
idelpivnitskiy
force-pushed
the
ProxyConnectObserver
branch
from
de4d6b6 to
afe36ec
Compare
idelpivnitskiy
added a commit
to idelpivnitskiy/servicetalk
that referenced
this pull request
Oct 2, 2023
Motivation: Prior to `ConnectionObserver.ProxyConnectObserver` added in apple#2711, the sequence of `ConnectionObserver` events was different between regular connections and connection to the proxy tunnels. `HttpContextKeys.HTTP_TARGET_ADDRESS_BEHIND_PROXY` (apple#2169) was useful to distinguish when the correct callback inside `ConnectionObserver` before reporting that a new connection is ready to take traffic. After adding `ProxyConnectObserver`, there is no need in this custom key. Modifications: - Deprecate `HttpContextKeys.HTTP_TARGET_ADDRESS_BEHIND_PROXY` and describe what is an appropriate replacement for users; Result: We will be able to remove `HttpContextKeys.HTTP_TARGET_ADDRESS_BEHIND_PROXY` in the future releases.
idelpivnitskiy
added a commit
that referenced
this pull request
Oct 3, 2023
Motivation: Prior to `ConnectionObserver.ProxyConnectObserver` added in #2711, the sequence of `ConnectionObserver` events was different between regular connections and connection to the proxy tunnels. `HttpContextKeys.HTTP_TARGET_ADDRESS_BEHIND_PROXY` (#2169) was useful to distinguish when the correct callback inside `ConnectionObserver` before reporting that a new connection is ready to take traffic. After adding `ProxyConnectObserver`, there is no need in this custom key. Modifications: - Deprecate `HttpContextKeys.HTTP_TARGET_ADDRESS_BEHIND_PROXY` and describe what is an appropriate replacement for users; Result: We will be able to remove `HttpContextKeys.HTTP_TARGET_ADDRESS_BEHIND_PROXY` in the future releases.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
After #2699 implemented HTTP proxy CONNECT with ALPN, it made observability
confusing. It broke the contract that only one
connectionEstablishedmethod canbe invoked, which happens when ALPN switches from HTTP/1.1 to HTTP/2 pipeline.
Also, for a proxy tunnel with HTTP/1.1
ConnectionObserverevents were always confusing:users had to wait for
handshakeCompleteafterconnectionEstablishedto report thatthe connection is "ready".
Modifications:
ConnectionObserver.ProxyConnectObserverthat reports events related toCONNECTphase that starts right afteronTransportHandshakeComplete;PipelinedLBHttpConnectionFactoryto defer invocation ofconnectionEstablished/multiplexedConnectionEstablisheduntil after the handshakeis complete to make it consistent with non-proxied connections and make sure only
one of the "established" methods is invoked;
ProxyConnectLBHttpConnectionFactoryTestbecause it targeted code pathsthat do not exist anymore. Instead, enhanced
HttpsProxyTestto cover similar use-cases.ProxyTunnelto allow behavior customization;Result:
Enhanced observability for proxy tunnels,
ConnectionObserverbehavior is consistentbetween proxied and non-proxied use-cases.
Expected sequence of observer callbacks:
Depends on #2699, review only starting from "Add ConnectionObserver.ProxyConnectObserver" commit.