opentelemetry-rust
opentelemetry-rust copied to clipboard
open-telemetry
[Feature]: Port opentelemetry-http to hyper 1.x
Related Problems?
https://github.com/hyperium/tonic/issues/1307
Describe the solution you'd like:
Hyper 1.0 has been released and as such it'd be nice to upgrade opentelemetry-http's HttpClient to the new version
Considered Alternatives
No response
Additional Context
No response
With the upgrade I think we should also upgrade the version of http to 1.0. However it's currently blocked because some of the http client implementation is still on pre 1.0 and having both http 1.0 create and http 0.2 create caused some issues.
We can either
- Delete all http client that hasn't support http 1.0 yet(that means every http client except
hyper) and upgrade theopentelemtry-httpcreate to use thehttp 1.0types. This will require some changes on downsteam create uses those http client defined inopentelemetry-http. - Wait until http clients to upgrade to http 1.0. However, some clients hasn't upgrade in a while so I am not sure if they will upgrade to 1.0 soon
As I understand in the community meeting this week, it was agreed to go with the first option above i.e., delete all the HTTP clients not supporting HTTP 1.0, and bring them back once the support is there.
Other than the http cteates. Tonic also depends on the http 0.2. We are currently blocked on it https://github.com/hyperium/tonic/issues/1579
After going through this upgrade for our project, I would definitely advise waiting on this until when/if tonic updates. I had to abort our upgrade for now because of the tonic dependency, and couldn't get them to work side by side easily, at least in the few hours I tried.
I take that back. It took some figuring out but in our product I was able to get hyper middle ware with hyper 1.0 and http 1.0 to work with tonic 0.2. So it looks like it should be possible to support both and not have to wait for tonic's upgrade (which doesn't seem to be progressing very fast).
Would it need to be in the form of a temporary feature or something like that?
I can look into the hyper middle ware or if you want feel free to take this issue @KallDrexx
I took an hour or two to look at this, and reqwest is another issue: https://github.com/seanmonstar/reqwest/issues/2039. It's possible to do, but since HttpClient otel trait requires sending http crate Requests and returning a http crate Response, a bunch of conversions have to occur.
What do you think about https://github.com/algesten/ureq instead of reqwest?
I totally forgot we discussed removing reqwest and other non-1.x compatible libraries, so ignore my last comment
I think I'm going to have to pull my volunteering for this task. There's a lot of context here that I'm missing on why things are set up the way they are, that I find it hard to make any concrete decisions.
The opentelemetry-http crate seems to be just a wrapper for opentelemetry executing http requests on your behalf. This ends up having ramifications that we are not providing a composable abstraction. It's not clear to me why opentelemetry doesn't just provide a mechanism for adding specific headers to an existing request or extracting them from an existing response.
This is a fundamental difference because the http upgrade becomes significantly easier. We don't need a trait that takes in a Request<T> that's specific to http 1.x, but instead we have one version that takes in a reqwest::Request, another that takes in http::Request<T>, another that takes in a isachs::Request<T>. It can even take in a library specific RequestBuilder instead.
This means the user is still responsible for managing the client and building the request they want, and adding opentelemetry's info after the fact. This isn't a huge ordeal, because no matter what users still need to build the request they need anyway. It's also trivial for any user to create a wrapper to always add otel headers as needed.
So much of the code relies on these custom OpenTelemetry http client wrappings that it's not trivial to redo everything to remove reqwest.
And it just feels like the wrong abstraction from my point of view, but like I said, I do not know the original context of why things are set up the way they are.
Any chance this will get prioritized? This effectively blocks consumers of opentelemetry_http crate from moving up to hyper 1.0. In my case, we already moved up and am now blocked from using HeaderExtractor as it causes a conflict between the version of http crate in hyper 1.0 and opentelemetry_http.
That probably unblocks this. I've been pretty slammed lately though so it might take me a bit to circle back around to this.
And it just feels like the wrong abstraction from my point of view, but like I said, I do not know the original context of why things are set up the way they are.
As the person who was responsible for starting the opentelemetry-http crate (#415) please don't overestimate the amount of planning -- I think I was merely extracting a bunch of repetitive/related code into one place that seemed more coherent than what came before. If you are in a position to implement a more modular API, I think that would be great -- there are already traits for injecting/extracting headers so maybe those provide a useful starting point?
I gave this a quick shot but got blocked on tonic, which has not yet merged an upgrade to http@1 (see https://github.com/hyperium/tonic/issues/1579 for details). This is an optional dependency of opentelemetry-otlp.
In theory, this doesn't block opentelemetry-http, but the crates are intertwined enough that separating these seemed counterproductive to me.
I think it would be reasonable to attempt to separate the dependencies of opentelemetry-otlp and opentelemetry-http. They each have their own public API that does not need to share the underlying http implementation, no? Tonic might take a long time to port to hyper 1.0. There are users (including myself) that just want to use the header extractor and injector implementation of the opentelemetry-http crate. The published opentelemetry-otlp can continue to rely on [email protected] and [email protected] in the meantime. Thoughts?
FWIF the roll_dice demo works with [email protected] https://github.com/open-telemetry/opentelemetry.io/pull/4242
I'm not sure if this is sufficient to support the proposal for separation of -otlp and -http, on its face, but it's evidence that at least the opentelemetry-sdk is fine with [email protected]. FWIF, in general, the ecosystem looks healthier if packages are relatively decoupled and are free to upgrade critical dependencies asyncrhonously. If they can't, it smells like poor separation of concerns. Understanding there's complexity etc. caveats - nonetheless.
