Skip to content

Feature/hyper instrumentation#188

Draft
Travispersson wants to merge 6 commits into
mainfrom
feature/hyper-instrumentation
Draft

Feature/hyper instrumentation#188
Travispersson wants to merge 6 commits into
mainfrom
feature/hyper-instrumentation

Conversation

@Travispersson

@Travispersson Travispersson commented Apr 17, 2026

Copy link
Copy Markdown
Contributor
  • instrument hyper senders for http1 and http2 in separate features.
  • from_parts now takes an UrlParts struct instead because the url types used in reqwest and hyper differ. Maybe you rather want us to parse from one to the other type instead.. 🤷
  • removed url crate since reqwest already exports its url type

Comment thread src/instrumentations/http/client.rs Outdated
Comment on lines +17 to +24
pub(crate) struct UrlParts {
pub(crate) full_url: Option<String>,
pub(crate) path: String,
pub(crate) host: Option<String>,
pub(crate) scheme: Option<String>,
pub(crate) port: Option<u16>,
pub(crate) query: Option<String>,
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue here is that hyper uses http::Uri, while reqwest relies on url::Url instead.

I suggest we define some trait to represent any Url-like structure and then implement it for both.

The trait should just return all those values in some way (probably as &str).

I can do it if you want.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread src/instrumentations/http/hyper.rs Outdated
Comment on lines +3 to +4
//! This module instruments `hyper::client::conn::http1::SendRequest` and
//! `hyper::client::conn::http2::SendRequest`.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll have to instrument hyper_util::client::legacy::Client as well, ideally behind an additional feature

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why we may want a separate feature is because it's guarded by the "client" feature in hyper

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah sure! I can do it after you merge #189

Comment thread src/instrumentations/http/hyper.rs Outdated
Comment thread src/instrumentations/http/hyper.rs
Comment thread src/instrumentations/http/hyper.rs
Comment thread src/instrumentations/http/mod.rs Outdated
//! HTTP client instrumentation utilities.

#[cfg(feature = "reqwest")]
#[cfg(any(feature = "reqwest", feature = "hyper-http1", feature = "hyper-http2"))]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have the same condition in src/instrumentations/mod.rs.

We should probably only keep one.

@lbeschastny lbeschastny Apr 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example:

Suggested change
#[cfg(any(feature = "reqwest", feature = "hyper-http1", feature = "hyper-http2"))]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants