Disable the deprecation about delaying sending of events until shutdown

[ste93cry]

This PR fixes #884 by disabling the deprecation when the transport is initialized by Sentry itself (out-of-the-box setup)

[mfb]

So in version 2.2 it sounds like we now have a flush() method, but it's actually not needed because now events are sent immediately?

[ste93cry]

The flush method is part of the Unified API and call be called by the user any time to force the sending of the events in the queue and of course in a fully syncronous transport this is not needed. The reason the deprecation was in-place is because the HttpTransport will became fully syncronous in 3.0, thus the warning was there to advice users to stop relying on the behavior of events sent at the shutdown of the application.

[mfb]

I did some benchmarking of Sentry 1.x, 2.1 and 2.2 using the out-of-the-box configuration, with an unreachable sentry server (connection attempt just hangs forever or until a timeout is reached). The test script triggers four error events.

• Sentry 1.x: Each error takes 2 seconds, at which point the connection timeout is reached; script consumes minimal CPU during each 2-second wait time.
• Sentry 2.1: Each error takes a millisecond or so; then during shutdown the script hangs, consuming 100% CPU.
• Sentry 2.2: Script hangs when first error is triggered, consuming 100% CPU.

This doesn't seem great to me - the default HTTP client would ideally be more resilient to the sentry server being unavailable, network problems, etc.

[B-Galati]

Could it be because of HTTP plug implementation ? Like this one https://twitter.com/jderusse/status/1115333801419399170 ?

[mfb]

Yes presumably this is the bug re: 100% CPU usage. But the endless waiting is also IMO a regression from Sentry 1.x, since this would cause processes to be delayed indefinitely, as well as hogging memory and request slots. Could be resolved by setting a reasonable default timeout as 1.x had.

[ste93cry]

Could be resolved by setting a reasonable default timeout as 1.x had.

No HTTP client that I know sets this timeout to something other than infinite and I think that the reason may be that any value may be too less or too much depending on a case-by-case basis, which I tend to agree. This does not change the fact that there is a bug in the implementation of the client we suggest to use, and since it has some other drawbacks too found in the past maybe we should decide to change it to something more stable like Guzzle.

[mfb]

Yes the timeout should definitely be configurable case-by-case. I'm actually not sure how to configure it at present though.

[ste93cry]

Since it depends on the HTTP client that you choose to use we cannot provide an option out-of-the-box that works for everything and I don't think that even if we could we should do it as it's an implementation detail of the client. The way to configure it is to set your own HTTP client in the builder and then configure it however you like

[mfb]

Wouldn't it make sense to provide a way to access the default HTTP client provided by sentry/sdk and configure things like timeout? Or is that wishful thinking?

[ste93cry]

I don't think that we should expose the HTTP client outside of the Sentry client as it's just an implementation detail needed to make requests and this library is not exposing APIs, so it makes no sense to access it from the outside. Exposing its options would mean leak somewhere details of this implementation imho...

[mfb]

Yes it's an implementation leak given that folks can provide their own HTTP client (and it's currently a good idea since the default one seems to be buggy :), but at the same time, you want to have good developer experience with the out-of-the-box client. The setHttpProxy() method is an illustrative example.. analogous method would be setHttpTimeout().

[ste93cry]

The setHttpProxy() method is an illustrative example.. analogous method would be setHttpTimeout().

In fact when those methods were added I disagreed, but Unified API had them in the specs and I had to accept it. I disagreed because I wanted to prevent arguments like yours: if you want setHttpTimeout, someone else may want setFoo and I may want setBar and there will be no end. Also, since the HTTP client is configurable (and I think this is a big plus), it's simply impossible to support the same option across all available client implementations out of here because the options and how to configure them is different for each of them. This is another cons point that I strongly wanted to avoid: we now have two methods to set the HTTP proxy that works only with the default HTTP client we choose, and if we change it we have to maintain all the code for the legacy client plus support the new one to not break BC. All of this just to save users to write a few lines of code or spend a bit more time to understand how something that they use really works. This is my opinion of course, but I strongly believe that the line between customization and configuration is really really tiny

[mfb]

Well if the timeout isn't configurable and the default is forever, then I'd say the recommended client is just not usable, because whenever you're sending an event if Sentry server is offline or slow to respond, your app will block indefinitely.

Re: timeout Sentry SDK documentation says "The default is SDK specific but typically around 2 seconds."