Replace usage of reqwest with hyper

[LucioFranco]

Currently, we pull in reqwest for tests in a couple of places but we already pull in hyper and http. So I think it would make sense to remove this additional dependency to reduce compile times and the number of dependencies we require during tests.

[benedikt-roth]

I'd like to give this a shot. Can you assign it to me please?

[LucioFranco]

@rossgardt hi! It's yours, let me know if you need any help :)

[LucioFranco]

@rossgardt any update on this? :)

[benedikt-roth]

Hi. I had some issues in the beginning, but now it looks like it is finally working. Will give you the next update by the end of the week.

[LucioFranco]

@rossgardt Great! Let me know if you have any questions, I am happy to help. :)

[benedikt-roth]

@LucioFranco sorry, I was sick throughout the week. Made some progress today.
However, I am facing an issue and need your support.

let https_client = hyper::Client::builder()
  .build::<_, hyper::Body>(https_conn);
let client = SyncClientBuilder::new()
  .http_client(https_client)
  ...

SyncClientBuilder::http_client requires a reqwest::client::Client. I saw that there is an elastic implementation for hyper (link.) But it seems to be outdated. Any idea how to use elastic with the hyper

[binarylogic]

@rossgardt any update on this? Anything we can help with? Could you let us know if you don't plan on making this change (which is fine 😄 ), and we can get someone on the team to do this.

[MOZGIII]

I noticed reqwest is being used by the dependencies of our dependencies. This means we probably won't have the improvement in compile-times.

[LucioFranco]

@MOZGIII I believe most of them should be possible to be used without reqwest.

[MOZGIII]

I actually have a list of all of the ones that rely on reqwest!

• https://github.com/durch/rust-goauth - doesn't have a feature to use hyper, but it's tiny, and we could add it easily!
• https://github.com/clux/kube-rs - this one's bigger; doesn't have a feature to use hyper either, but it might still be doable to port it if we really want to.

That's it, we only have two, but both are problematic, in a sense that we can't simply enable a feature to use bare hyper.

[LucioFranco]

Yeah, I have an issue open for kube-rs to move to tower but have not had time to work on it. Those also should be decently trivial to vendor code instead of trying to contribute for now.

[binarylogic]

@rossgardt any update on this? Otherwise, we can reassign. Thanks!

[fanatid]

I checked reqwest usage in code. Except tests now it's also used in gcp sink: https://github.com/timberio/vector/blob/d5e988a1bd402a84d736de3b61594c43a9a646c5/src/sinks/gcp/mod.rs#L75-L83

For removing it here sink build function should be async or get_token_implicit should block async code (is it possible in tokio?).

Also I found that some test have not trivial reqwest usage, for example in splunk_hec: https://github.com/timberio/vector/blob/d5e988a1bd402a84d736de3b61594c43a9a646c5/src/sinks/splunk_hec.rs#L698
If reqwest will be removed, we will need add serde_urlencode (maybe more deps). Maybe it's ok left reqwest in dev-dependencies for tests?

[bruceg]

Yes we have some sink build functions that do sync requests and can block the build process. The proper solution is to make build async, but that could end up being quite invasive. The gcp code is adapted from the goauth crate. Given how little of that crate we use, the best path would be to internalize the needed parts into our tree and drop the dependency.

[binarylogic]

@ktff assigning this to you since it appears to be a natural extension of #2117.

[ktff]

Continuation from #2782 (comment)

@fanatid let's move that conversation here.

So I agree with the opening comment that less dependency is better, even in tests, and yes some of it's dependencies would need to be added directly. Although I don't know at this point how many. @fanatid do you have some idea which one would we need to add?

But no matter how much transitive dependencies we add, we would still remove quite the amount of code from the build. Although API surface on which we would depend could increase, for that reason we could pick a few representative tests and try it out.

Regarding goauth using reqwest I agree with

Given how little of that crate we use, the best path would be to internalize the needed parts into our tree and drop the dependency.

with which we would also remove one more dependency.

[fanatid]

I do not have exact names of crates which will need to be added, but when I started removing reqwest -- serde_urlencode was definitely needed, maybe 1-2 more crates will be required.

[lukesteensen]

This is not an issue that we should consider vital. The overall idea to reduce our dependency count is a good one, but if we are legitimately using features of the crate that we'd have to pull in or reimplement then this is likely not worth the effort. There's also a decent chance that reqwest is already somewhere else in our dependency tree.

[binarylogic]

I agree. @ktff if you agree feel free to close this.

[ktff]

After some investigation, it stands:

• Transitioning our use of reqwest to hyper is relatively straightforward. So no new feature would need to be added.
• goauth is still the only one that dependens on reqwest. Although some 100-200 new lines would need to be added from goauth to the codebase to remove our dependency on goauth.
• By removing reqwest we wouldn't remove any other transitive dependency besides those that we would lose by upgrading to newer version of reqwest.

So, yes, as @lukesteensen said, this is not worth the effort, gains are marginal at best.

[binarylogic]

Thanks for the update. That sounds good to me.