Idea: allow using custom reqwest client for transport

[vorner]

The reqwest client that is used by default starts something like 5 background threads. That's quite a lot, considering sentry just sits there doing nothing most of the time (at least if there are no errors to report, which should be the case).

However, if the application already has a reqwest client around, it could be shared ‒ cloning the client only creates another handle to the background handle. Would it be possible to create another transport factory that creates the RequestTransport, but with the provided client?

Thank you

[mixalturek]

I have tried to implement the code. There are two pull requests that implement this feature request using two different approaches. The first one injects the Reqwest client through ClientOptions and the second one through TransportFactory. None of them is fully clean and smooth but I have no more ideas how to improve them.

• ClientOptions
  • pros: Very simple and straighforward to use, nothing is hidden for the user.
  • cons: One more field in ClientOptions struct with conditional compilation, which is quite dirty.
  • cons: Only for Reqwest variant, but not for Curl. But Curl client is single thread only so no sharing is possible in practice. There would be very expensive mutex blocking around I/O operations.
• TransportFactory
  • pros: Possibly for any transport implementation. But still not suitable for single thread Curl client.
  • cons: More difficult to use, partially hidden feature for the users.

[vorner]

I believe the Transport thing is conceptually the correct way to do it.

[mitsuhiko]

I think the right way would be to clean up the code behind the http transports to reuse and then expose some of it, so someone can use the already existing transport interface to inject a an already existing reqwest client.

[mitsuhiko]

Since an Arc<Transport> is a valid transport factory it's not very hard to use btw.

[mixalturek]

Yes, this is exactly the PR #131, so I'm closing #132 .

[mixalturek]

Since an Arc is a valid transport factory it's not very hard to use btw.

I know, but this is not possible in practice with ReqwestHttpTransport. The core issue is a cyclic dependency, I hit it during integration of the changes in feature branch with our code base. transport is passed to ClientOptions but you need ClientOptions to build the transport. It can be solved by build of ClientOptions in two phases using struct defaults, but the code would be really ugly. Much better (but still unpleasant) is build the transport lazily using transport factory.

sentry::ClientOptions {
        transport: Box::new(|options: &sentry::ClientOptions| -> Box<sentry::internals::Transport> {
            Box::new(ReqwestHttpTransport::with_client(options, reqwest::Client::clone(&CLIENT.client())))
        }),
        // ...
}