[Feature]: Port opentelemetry-http to hyper 1.x about opentelemetry-rust HOT 14 OPEN

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.

from opentelemetry-rust.

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.

from opentelemetry-rust.

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).

from opentelemetry-rust.

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 the opentelemtry-http create to use the http 1.0 types. This will require some changes on downsteam create uses those http client defined in opentelemetry-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

from opentelemetry-rust.

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.

from opentelemetry-rust.

Other than the http cteates. Tonic also depends on the http 0.2. We are currently blocked on it hyperium/tonic#1579

from opentelemetry-rust.

cc: @KallDrexx

from opentelemetry-rust.

Would it need to be in the form of a temporary feature or something like that?

from opentelemetry-rust.

I can look into the hyper middle ware or if you want feel free to take this issue @KallDrexx

from opentelemetry-rust.

I should be able to take this up later in the week.

from opentelemetry-rust.

I took an hour or two to look at this, and reqwest is another issue: seanmonstar/reqwest#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.

from opentelemetry-rust.

What do you think about https://github.com/algesten/ureq instead of reqwest?

from opentelemetry-rust.

I totally forgot we discussed removing reqwest and other non-1.x compatible libraries, so ignore my last comment

from opentelemetry-rust.

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.

from opentelemetry-rust.