feat: Add ureq transport support
#419
Conversation
Codecov Report
@@ Coverage Diff @@
## master #419 +/- ##
==========================================
- Coverage 81.68% 81.10% -0.59%
==========================================
Files 71 72 +1
Lines 8092 8165 +73
==========================================
+ Hits 6610 6622 +12
- Misses 1482 1543 +61 |
sentry/src/lib.rs
Outdated
| //! - `ureq`: Enables the ureq transport. | ||
| //! - `ureq-tls`: Enables `rustls` support for `ureq`. Please note that this has to be enabled for | ||
| //! https support, and cannot be enabled by default. | ||
| //! - `ureq-native-tls`: Enables `native-tls` support for `ureq`. |
There was a problem hiding this comment.
you could recommend here to prefer setting one of ureq-tls or ureq-native-tls and mention they automatically pull in ureq.
Or maybe even go further and rename ureq to ureq_base_ or something and make it internal only.
There was a problem hiding this comment.
That sounds like a better plan, I don't think a Sentry transport without https makes a lot of sense 😅
There was a problem hiding this comment.
@flub I opted to go for ureq and ureq-native-tls. No ureq_base_ though, as there's only httpdate support here after we get rid of tokio.
sentry/Cargo.toml
Outdated
| @@ -44,6 +44,9 @@ surf-h1 = ["surf_/h1-client", "httpdate"] | |||
| surf = ["surf_/curl-client", "httpdate", "tokio"] | |||
| native-tls = ["reqwest_/default-tls"] | |||
| rustls = ["reqwest_/rustls-tls"] | |||
| ureq = ["ureq_", "httpdate", "tokio"] | |||
There was a problem hiding this comment.
do you happen to know why this needs the tokio dependency? I thought ureq was blocking? I have the same question for the surf dependency as well to be fair, but that's clearly out of scope here :)
There was a problem hiding this comment.
Because of the TransportThread every backend seems to use. surf has a clear requestr.await, I think that's async driven by tokio too? Perhaps you meant curl which - just like ureq - only has a single async move { rl } and is blocking otherwise.
Getting rid of tokio is our next stop unless it's simpler/cleaner to implement this directly. Is a Transport - or at least the send_envelope function - supposed to return quickly or is it fine if we perform a blocking request here?
There was a problem hiding this comment.
It would be fair to split the TransportThread into two sync/async variants for transports that are based on one or the other in the future.
There was a problem hiding this comment.
oh, the TrnasportThread builds an internal Tokio runtime... that is indeed a bit needless for sync transports and weird for the surf one. would be nice to fix this sometime indeed.
There was a problem hiding this comment.
I'll pick this up in a followup PR!
sentry/Cargo.toml
Outdated
| @@ -44,6 +44,9 @@ surf-h1 = ["surf_/h1-client", "httpdate"] | |||
| surf = ["surf_/curl-client", "httpdate", "tokio"] | |||
| native-tls = ["reqwest_/default-tls"] | |||
| rustls = ["reqwest_/rustls-tls"] | |||
| ureq = ["ureq_", "httpdate", "tokio"] | |||
There was a problem hiding this comment.
It would be fair to split the TransportThread into two sync/async variants for transports that are based on one or the other in the future.
Ureq is easy to use, smaller than the other http crates and doesn't pull in as many crate dependencies either, which is why [we] are actively switching to it and in need of an `ureq` transport in Sentry too. [we]: https://github.com/Traverse-Research
…ts (#422) These transport backends don't need the overhead of a tokio runtime and all the dependency crates that come with it, when they are already blocking on their own. A simple Rust thread and mpsc channel is enough. The transport thread implementation is duplicated between a tokio and non-tokio module that are used by reqwest/surf, and curl/ureq respectively. See also #419 (comment) where this was briefly discussed.
Ureq is easy to use, smaller than the other http crates and doesn't pull in as many crate dependencies either, which is why we are actively switching to it and in need of an
ureqtransport in Sentry too.