ConnectionObserver: provide ConnectionInfo on transport handshake #2726
Conversation
Motivation: If connection fails with an exception before one of the "established" methods were invoked, users don't have access to meaningful information, like `SslConfig`, SocketOptions, channelId (for correlation with wire logs), etc. Modifications: - Add `ConnectionObserver.onConnectionInitialization(ConnectionInfo)` callback that gives users a ST view of the netty's `Channel`; Results: 1. Users can collect more information about a connection if it's failed before "established". 2. Users can get Channel's ID for reporting `ProxyConnectObserver` events.
| * Finalized {@link ConnectionInfo} will be available via {@link #connectionEstablished(ConnectionInfo)} or | ||
| * {@link #multiplexedConnectionEstablished(ConnectionInfo)} callbacks. | ||
| */ | ||
| default void onConnectionInitialization(ConnectionInfo info) { // FIXME: 0.43 - consider removing default impl |
There was a problem hiding this comment.
Open for better name ideas
There was a problem hiding this comment.
The trick is that at this point channel.localAddress() and channel.remoteAddress() return null. This is not aligned with ConnectionInfo API (non-nullable methods). Another option is to provide onTransportHandshakeComplete(ConnectionInfo) instead of onTransportHandshakeComplete(). When Channel becomes active, addresses are available.
There was a problem hiding this comment.
Looking at the naming for other events, they all use the past tense (failed, complete, established, etc.). So to align with the naming I would suggest onConnectionInitialized
I see that you want to convey that initialization just started, so what about onConnectionInitStarted ? (or onConnectionInitializationStarted but that is a bit long as well)
There was a problem hiding this comment.
As we discussed offline, changing to onTransportHandshakeComplete(ConnectionInfo)
| * Finalized {@link ConnectionInfo} will be available via {@link #connectionEstablished(ConnectionInfo)} or | ||
| * {@link #multiplexedConnectionEstablished(ConnectionInfo)} callbacks. | ||
| */ | ||
| default void onConnectionInitialization(ConnectionInfo info) { // FIXME: 0.43 - consider removing default impl |
There was a problem hiding this comment.
Looking at the naming for other events, they all use the past tense (failed, complete, established, etc.). So to align with the naming I would suggest onConnectionInitialized
I see that you want to convey that initialization just started, so what about onConnectionInitStarted ? (or onConnectionInitializationStarted but that is a bit long as well)
| * Finalized {@link ConnectionInfo} will be available via {@link #connectionEstablished(ConnectionInfo)} or | ||
| * {@link #multiplexedConnectionEstablished(ConnectionInfo)} callbacks. | ||
| */ | ||
| default void onConnectionInitialization(ConnectionInfo info) { // FIXME: 0.43 - consider removing default impl |
There was a problem hiding this comment.
If the ConnectionInfo has too much that we can offer, maybe we should revisit the discussed concept of a ReducedConnectionInfo?
There was a problem hiding this comment.
Context for others from our offline discussion:
I thought about that as well, but because
EarlyConnectionAcceptoralready usesConnectionInfoand it's aFunctionalInterface, didn't want to create inconsistency. Propagation ofExecutionContextmakes it harder indeed. I was less worried, bcz officially it's "internal" API, changes for initializers should not be visible for users in practice.
idelpivnitskiy
changed the title
|
@daschl, resolved all comments based on our discussion. I will merge this bcz I need it as a snapshot. Please review the final changes, I will submit a follow-up PR if you have comments. |
LGTM 👍 |
Motivation:
If connection fails with an exception before one of the "established" methods were invoked, users don't have access to meaningful information, like
SslConfig, SocketOptions, channelId (for correlation with wire logs), local address, etc.Modifications:
ConnectionObserver.onTransportHandshakeComplete(ConnectionInfo)callback that gives users a ST view of the netty'sChannel;ConnectionObserver.onTransportHandshakeComplete();Results:
ProxyConnectObserverevents and security handshake failures.