feat: Experimental support for reqwest #839
Conversation
untitaker
commented
Nov 16, 2020
•
untitaker
commented
- test out http proxy support e2e
- run https://github.com/getsentry/ingest-load-tester to see impact (it will be slower) -- it's okay but we should measure in prod to get better results. no crass regressions
untitaker
force-pushed
the
ref/forward-through-upstream
branch
from
421988a to
042ce66
Compare
…elay into ref/forward-through-upstream
untitaker
changed the title
relay-server/src/http.rs
Outdated
| } | ||
|
|
||
| /// Add a new header, not replacing existing ones. | ||
| pub fn header(&mut self, key: &str, value: &[u8]) -> &mut Self { |
There was a problem hiding this comment.
If you make this something like Into<Vec<u8>> or Into<HeaderValue>, you can potentially avoid intermediate allocations. This will now always allocate another time, even if you borrow an owned thing in the caller.
There was a problem hiding this comment.
Yeah I'll fix this. I started out with a trait instead of an enum where this was not feasible initially (although I could've implemented additional wrapper methods on impl Box<dyn RequestBuilder>)
There was a problem hiding this comment.
fixed for header value, too hard for header name
Let's clean up once we get rid of awc
There was a problem hiding this comment.
ugh it doesn't work at all. reverted...
relay-server/src/http.rs
Outdated
| .map_err(From::from), | ||
| ), | ||
| Response::Reqwest(_) => { | ||
| // TODO: is this necessary for reqwest? |
There was a problem hiding this comment.
Please verify this. It would be great if reqwest handles this correctly.
There was a problem hiding this comment.
does not appear to be necessary. how did you test this though?
There was a problem hiding this comment.
appears not to make a difference in tests
There was a problem hiding this comment.
We only have a functional test for socket errors, but nothing that asserts that the connection stays open, unfortunately. You would have to manually test this, or figure out a way to assert metrics in tests.
There was a problem hiding this comment.
I don't think asserting that the connection stays open is sufficient. The way we IIRC encountered this in prod (not in dev) was that connections were timing out or abruptly closed in the next request.
There was a problem hiding this comment.
I'm not sure when connections were closed, but they were effectively left in an invalid state after which the downstream had to close and re-establish. Either way, I think we need to verify that reqwest consumes the body or otherwise do this manually.
There was a problem hiding this comment.
I added request consumption for the reqwest codepath as well since I was not really able to reproduce the problem we had (for either actix or reqwest), so I have little confidence that I am able to test it.
| } | ||
| } | ||
|
|
||
| pub fn clone_headers(&self) -> Vec<(String, Vec<u8>)> { |
There was a problem hiding this comment.
clone_headers returns tuples, get_all_headers returns a flat list. Can you unify this?
There was a problem hiding this comment.
This is a flat list, the inner vec is a bytestring
untitaker
force-pushed
the
ref/forward-through-upstream
branch
from
26b2b9e to
d3b84e6
Compare
|
Need to look into a bunch of things again:
|
relay-server/src/http.rs
Outdated
| @@ -331,3 +348,27 @@ impl Response { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| fn reqwest_body_from_read<R>(mut reader: R) -> reqwest::Body | |||
There was a problem hiding this comment.
nit: Please move this function before its first use and add a doc comment.
There was a problem hiding this comment.
There is a doc comment on it (weirdly I got an email for your comment that had the old diff), but yeah I moved the function (we need a linter for that...)
