Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hyper-util: expose connection info on whether it's reused from the pool or newly established #3727

Open
magurotuna opened this issue Aug 7, 2024 · 3 comments · May be fixed by hyperium/hyper-util#145
Labels
A-client Area: client. C-feature Category: feature. This is adding a new feature. E-medium Effort: medium. Some knowledge of how hyper internal works would be useful. K-hyper-util Crate: hyper-util

Comments

@magurotuna
Copy link
Contributor

magurotuna commented Aug 7, 2024

Is your feature request related to a problem? Please describe.

While investigating hard-to-debug connection related issues in cloud infra, we will likely want to obtain any kind of information that might be relevant. Even whether a connection that has just failed is reused from the connection pool or is newly established can be a valuable insight that we could use to troubleshoot.

Describe the solution you'd like

Maybe add a new private field to the hyper_util::client::legacy::connect::HttpInfo, like is_new_connection (or is_reused_connection) and make it publicly accessible via a method?

Describe alternatives you've considered

I didn't come up with other alternatives

Additional context

N/A

@magurotuna magurotuna added the C-feature Category: feature. This is adding a new feature. label Aug 7, 2024
@seanmonstar
Copy link
Member

Since the HttpConnector and the Pool are separate, perhaps it could be a separate extension. That way it still works if using a different initial connector.

@seanmonstar seanmonstar added A-client Area: client. E-medium Effort: medium. Some knowledge of how hyper internal works would be useful. K-hyper-util Crate: hyper-util labels Aug 7, 2024
@magurotuna
Copy link
Contributor Author

That makes total sense.

What we are specifically interested in is to expose the "reused or not" information only on error cases, so I thought that we could add is_reused boolean inside hyper_util::client::legacy::Error. Then it felt to me that the most straightforward way to do so would be to add is_reused field in hyper_util::client::legacy::connect::Connected because it is exposed from the Error type and this provides connection-related information, but on second thought, what this Connected type has is more like protocol-related information (except the Connected::poison method that turns on the conneciton pool related flag), so it might be not a perfect fit to have is_reused flag in Connected since connection pooling is not necessarily related to protocols. Another point that made me think that having is_reused in Connected might not be good is that, Pooled::is_reused method provides a way to determine if the pooled object is reused or not, but at the same time PoolClient, which is wrapped in Pooled, has Connected field meaning that if we had is_reused in Connected, that would end up with duplicated is_reused in Connected and Pooled. This could be a code smell.

Given these things, now I think the interface as follows would be good:

pub struct Error {
    kind: ErrorKind,
    source: Option<Box<dyn StdError + Send + Sync>>,
    #[cfg(any(feature = "http1", feature = "http2"))]
-   connect_info: Option<Connected>,
+   connect_info: Option<ConnectInfo>,
}

+ /// Instead of adding `is_reused` to `Connected`, we define a new struct here
+ /// to hold a raw `Connected`, which has protocol-related information, and
+ /// `is_reused` flag, which is connection pool related information.
+ pub struct ConnectInfo {
+     connected: Connected,
+     is_reused: bool,
+ }
+ 
+ /// Instead of exposing all the methods of `Connected`, we expose getter methods
+ /// that allow callers to get necessary information about the failed connection.
+ impl ConnectInfo {
+     pub fn is_proxied(&self) -> bool {
+         self.connected.is_proxied()
+     }
+     
+     pub fn get_extras(&self, extensions: &mut Extensions) {
+         self.connected.get_extras(extensions);
+     }
+     
+     pub fn is_negotiated_h2(&self) -> bool {
+         self.connected.is_negotiated_h2()
+     }
+     
+     pub fn is_reused(&self) -> bool {
+         self.is_reused
+     }
+}

What do you think about this approach? The concern I have with this is that this will be a breaking change as the return type of Error::connect_info will be changed. Not sure if this is acceptable or not though.

@magurotuna
Copy link
Contributor Author

I implemented based on my proposed approach in hyperium/hyper-util#145. Any thoughts or feedback would be appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-client Area: client. C-feature Category: feature. This is adding a new feature. E-medium Effort: medium. Some knowledge of how hyper internal works would be useful. K-hyper-util Crate: hyper-util
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants