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

[Feature]: Port opentelemetry-http to hyper 1.x #1427

Open
Cogitri opened this issue Dec 5, 2023 · 20 comments · May be fixed by #1674
Open

[Feature]: Port opentelemetry-http to hyper 1.x #1427

Cogitri opened this issue Dec 5, 2023 · 20 comments · May be fixed by #1674
Assignees
Labels
enhancement New feature or request triage:accepted Has been triaged and accepted.

Comments

@Cogitri
Copy link

Cogitri commented Dec 5, 2023

Related Problems?

hyperium/tonic#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

@Cogitri Cogitri added enhancement New feature or request triage:todo Needs to be traiged. labels Dec 5, 2023
@TommyCpp TommyCpp self-assigned this Dec 10, 2023
@TommyCpp TommyCpp added triage:accepted Has been triaged and accepted. and removed triage:todo Needs to be traiged. labels Dec 12, 2023
@TommyCpp
Copy link
Contributor

TommyCpp commented Dec 12, 2023

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

@lalitb
Copy link
Member

lalitb commented Dec 13, 2023

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.

@TommyCpp
Copy link
Contributor

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

@hdost
Copy link
Contributor

hdost commented Jan 9, 2024

cc: @KallDrexx

@KallDrexx
Copy link
Contributor

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.

@KallDrexx
Copy link
Contributor

KallDrexx commented Jan 12, 2024

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

@hdost
Copy link
Contributor

hdost commented Jan 14, 2024

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

@TommyCpp
Copy link
Contributor

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

@KallDrexx
Copy link
Contributor

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

@KallDrexx
Copy link
Contributor

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.

@frederikhors
Copy link

frederikhors commented Jan 18, 2024

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

@KallDrexx
Copy link
Contributor

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

@TommyCpp TommyCpp assigned KallDrexx and unassigned TommyCpp Jan 22, 2024
@KallDrexx
Copy link
Contributor

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.

@ramgdev
Copy link
Contributor

ramgdev commented Feb 12, 2024

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.

@dotansimha
Copy link

It seems like reqwest@0.12 supports http@v1 - this should unblock this issue? @KallDrexx

@KallDrexx
Copy link
Contributor

That probably unblocks this. I've been pretty slammed lately though so it might take me a bit to circle back around to this.

@djc
Copy link
Contributor

djc commented Mar 25, 2024

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?

@SamirTalwar
Copy link

I gave this a quick shot but got blocked on tonic, which has not yet merged an upgrade to http@1 (see hyperium/tonic#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.

@jnicholls
Copy link

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 opentelemetry-http@0.11 and http@0.2 in the meantime. Thoughts?

@ekquasar
Copy link

FWIF the roll_dice demo works with hyper@1.x
open-telemetry/opentelemetry.io#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 hyper@1.x. 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.

@aumetra aumetra linked a pull request Apr 22, 2024 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request triage:accepted Has been triaged and accepted.
Projects
None yet
Development

Successfully merging a pull request may close this issue.