Implement support for flushable clients

[ste93cry]

Q	A
Branch?	2.2
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
License	MIT

This PR is another attempt other than #799 to fix #811 and getsentry/sentry-laravel#222. It involves much more changes and do not follow the unified API that expect some methods on the client like flush or close. Instead, it exposes new methods with the Async suffix that return promises and, depending on the implementation, these may be resolved immediatly when they are created (think about a syncronous transport) or at some point in time. The decision to not follow the unified API is because in PHP there is no concept of real async, so having such methods that internally resolves the promises like in the JavaScript SDK would just make them syncronous. There are also some libraries and frameworks like Swoople or ReactPHP that use an event loop (similar to NodeJS) that runs and can periodically tick a queue or advance a timer: this is what we are looking for. cURL can send requests in parallel but is not async at all. To do it in fact, you have to use a loop that will tick the internal queue of requests to send them a little at a time, but obviously this will block the code execution. This is why some HTTP clients like Guzzle have their own queue that can be ticked from the outside and this is what users must do by integrating such libraries with their own event-driven loop.

It follows the specs of the Unified API and implements a flush method on the client that can be called by clients to drain the queue of events being sent on-demand. Since in PHP there is no concept of async out-of-the-box there is no implementation of an async client provided, thus the $timeout parameter is effectively ignored.

[Jean85]

This change seems too overly complicated to me, especially compared to #799. Since PHP doesn't have pure async, this seems an overkill to have not so much advantage. Also, due to the fact that the user still needs to call a specific method, using #799 and wrapping the await/flush call in an async tool of their own seems a better choice to me.

[ste93cry]

I decided to drop the capture*Async methods from the client and just follow the unified API by providing a flush method instead. There is no implementation of a real async transport as it would require a lot of effort by integrating libraries like ReactPHP

[stayallive]

🚢

[Jean85]

I decided to drop the capture*Async methods from the client and just follow the unified API by providing a flush method instead.

Please edit the description of this PR then, I was thrown off-track by that 😅

[Jean85]

I did just a quick scan, but LGTM 👍

[Spriz]

Any updates on when this will be released? :)

[stayallive]

We are currently preparing for the release of 2.2 which will contain this PR. So it'll be soon :)

[Spriz]

@stayallive Is there anything we can do to help? We are missing out on a ton of data because we are using Sentry in production - with this level of support I have actually started looking into alternative solutions to Sentry.

I guess it is really not a rare use case to use Sentry on queue consumers and other kinds of CLI scripts

[stayallive]

@Spriz, 2.2 was released a few days ago.

An issue was fixed when exit was used (usually in CLI scripts) that prevented events from being sent (was actually fixed in 2.1.3) that might have affected you and the new release allows manually flushing the queue if you need to for long running scripts.

$client = \Sentry\SentrySdk::getCurrentHub()->getClient();

if ($client instanceof FlushableClientInterface) {
    $client->flush();
}

The code above will force all events that might be in the queue to be sent immediately and not wait for the script to exit, ofcourse it still sends the events once a script exits.

For an implementation example you can look at the Laravel integration which flushes events in the queue worker after processing an queue job:

https://github.com/getsentry/sentry-laravel/blob/db55dfbb7d45fbda042131ec65ddf5b3eaf2a90e/src/Sentry/Laravel/Integration.php#L89-L102
https://github.com/getsentry/sentry-laravel/blob/db55dfbb7d45fbda042131ec65ddf5b3eaf2a90e/src/Sentry/Laravel/EventHandler.php#L113-L114

Let me know if this solves your issues or we missed a use case!

[Spriz]

Amazing @stayallive - we will implement one of the next few days, and I will get back at ya if we miss anything! 👏 🎉

[helhum]

@stayallive I've been looking at the latest release and the Laravel integration, but I'm still struggling to understand how it is currently meant to work and also don't exactly know where you're heading with a 3.0 release.

As far as I understand the code in HttpTransport all events are now sent directly and synchronously.

Also, with the current implementation $client->flush(); does nothing, as it only flushes queued events, if they are queued, and the used transport implements ClosableTransportInterface, which HttpTransport currently does not implement.

So the code in Laravel is currently a Noop, as the integration does not provide their own HttpTransport implementation.

Speaking of that. I also looked into what I would need to do to implement my own Transport. To do so, it would basically require me to also completely implement (or copy) ClientBuilder, because my alternative HttpTransport would as well need an HttpClient, but creating that is done in a private method (to be precise: the PluginClient is created there, but this adds quite some useful features).

My goals I'm trying to achieve is creating an HttpTransport which:

1. Actually implements ClosableTransportInterface
2. Optionally delays sending events until close is called or script ends
3. Calls a callback that implements any fallback behaviour in case sending an event failed (instead of silently ignoring it)

While I can certainly do that, I don't feel comfortable with the amount of adaptation I need to do and therefore asking for your advice.

Should I just implement my own HttpTransport, or create a pull request that makes the current HttpTransport configurable through options?

I'd happily do the latter, but wonder why delaySendingUntilShutdown is deprecated and what exactly the plans are for ClosableTransportInterface

Thanks a bunch!

[ste93cry]

You made a bunch of good questions. I will speak for the core SDK as I don't know how the Laravel integration is implemented.

As far as I understand the code in HttpTransport all events are now sent directly and synchronously.

That transport was never meant to be asyncronous nor was meant to delay sending of events until the shutdown of the application, but I didn't know at the time I coded it that requests are not sent until you call wait so what it does now is just a side-effect of a wrong implementation. However as far as I see the behavior should not have been changed and the events should still be sent as it was before. If you try to instantiate the class yourself and rely on this deprecated behavior you will get a deprecation error, but you should not get it if you just use the client builder with the default settings to instantiate the client and/or you just call \Sentry\init(...). If you see a different behavior then we have a bug to fix.

Speaking of that. I also looked into what I would need to do to implement my own Transport. To do so, it would basically require me to also completely implement (or copy) ClientBuilder, because my alternative HttpTransport would as well need an HttpClient, but creating that is done in a private method (to be precise: the PluginClient is created there, but this adds quite some useful features).

You're again right. The issue has been fixed with #855 which is scheduled for release with version 2.3

Should I just implement my own HttpTransport, or create a pull request that makes the current HttpTransport configurable through options?

The HttpTransport should implement the ClosableTransportInterface so that calling the close method will flush the events, however this makes sense only if the HTTP client is "asyncronous" (e.g. Guzzle) but not really asyncronous, e.g. a ReactPHP client)

I'd happily do the latter, but wonder why delaySendingUntilShutdown is deprecated and what exactly the plans are for ClosableTransportInterface

The plan for ClosableTransportInterface is to remove it in 3.0 and merge it with the TransportInterface interface. We cannot do it now because adding a method to an existing interface is a breaking change.

[helhum]

That transport was never meant to be asyncronous nor was meant to delay sending of events until the shutdown of the application, but I didn't know at the time I coded it that requests are not sent until you call wait so what it does now is just a side-effect of a wrong implementation

Ah, OK good to know. So the intention always was to send events immediately and synchronously. And that in 2.2 events were delayed until PHP shutdown was actually a bug (#799, #811), which was fixed here. Correct?

If you see a different behavior then we have a bug to fix.

No, that is how it now works. Thanks for the clarification.

The issue has been fixed with #855 which is scheduled for release with version 2.3

Ah, cool. That would be exactly what I need, when implementing my own Transport.

The plan for ClosableTransportInterface is to remove it in 3.0 and merge it with the TransportInterface interface. We cannot do it now because adding a method to an existing interface is a breaking change.

Also good to know, thanks!

The HttpTransport should implement the ClosableTransportInterface

Do I understand it correctly, that I could do a pull request to do so, or it does not matter much because of:

however this makes sense only if the HTTP client is "asyncronous" (e.g. Guzzle) but not really asyncronous, e.g. a ReactPHP client)

I have a sense what you mean, but just to be sure we're on the same page, I try to rephrase in my own words:

In a synchronous PHP application (not ReactPHP or similar), there is no way to execute code asynchronously, so close would be a noop anyway, as any event would have been sent already anyway.

In a ReactPHP application a Transport implementation of close would have to wait for all pending requests are sent.

If my understanding is correct, then all is fine for me and I can adapt to that with my integration.

Last remaining question though:

I would still like to have to possibility that my integration is notified for each event that failed to be sent. While I could implement my own HttpTransport for that, it feels a bit like too much for this little addition (even with #855 in place). Would you accept a PR that adds an option eventSendingFailedCallback that is evaluated and called in HttpTransport?

Thanks again!

[ste93cry]

Ah, OK good to know. So the intention always was to send events immediately and synchronously.

So, I thought one thing and I wrote something else so I will amend my previous statement, sorry for this. When I implemented the HttpTransport I did not know that the requests do not fire until wait is called and I made a lot of confusion, so what was happening is that the HTTP client created a promise that did not even start resolving when exiting the scope of the send() method. The wait call in both the destructor and the shutdown of the application was there to ensure that all requests fired before exiting and for no othe reason. This however created the side-effect that in long running processes requests were never sent according to the explanation I gave before on the behaviof or promises.

And that in 2.2 events were delayed until PHP shutdown was actually a bug (#799, #811), which was fixed here. Correct?

As I see it now it's a kind of bug since the name of the transport implies that it's syncronous. However, to not break the behavior of existing applications I cannot change when the events are sent and this would anyway heavily hurt performances. Plus, since PHP has no concept of async and we send one request at time we will always send one at time, no matter what or where we do it.

Do I understand it correctly, that I could do a pull request to do so, or it does not matter much because of:

Actually the problem of requests that are not sent until the shutdown in long-running processes is not fixed with this PR as it's missing the implementation of the close method. It's a bit tricky in my opinion to understand where we want to fix it because it depends on how you use the transport. If you rely on the "deprecated" behavior (the default) of waiting until the shutdown of the application in a long-running application (e.g. a queue) and you are not using any framework integration (sentry-laravel or sentry-symfony) then you will face the problem. Integrations solved it long before this PR got merged with workarounds and hacky code. If however you are working with a classic application that processes something and then exists then you don't need to implement that interface. I would definitely ask you, if you are willing to, to open a PR and implement the ClosableTransportInterface interface (basically replacing the cleanupPendingRequests method) so that we fix the issue no matter what context we are

If my understanding is correct, then all is fine for me and I can adapt to that with my integration.

You understood right. The key is that PHP has no concept of async and Curl can only send requests in parallel, but not asyncronously. Libraries like ReactPHP implements their own event loop, so as long as you integrate with it you can have a "truly" asyncronous application and wait would not be necessary anymore. close would just ensure that all the promises resolve before exiting the application. To clarify, if you were to implement it you would do what in JS can be done with Promise.all() and only when that promise resolves you can be safe in shutting down the application.

I would still like to have to possibility that my integration is notified for each event that failed to be sent. While I could implement my own HttpTransport for that, it feels a bit like too much for this little addition (even with #855 in place). Would you accept a PR that adds an option eventSendingFailedCallback that is evaluated and called in HttpTransport?

This is being discussed in #886. The proposal I did was to log errors/debug messages using a PSR-3 logger (which by default would be noop). For sure we won't let exceptions throw/bubble as it would break the entire application of course making this library useless. I would like to avoid adding more options as we already have many of them. I'm not sure if the Unified API specs says something on this matter, but I strongly suggest anyway to continue the discussion in the referenced PR

[helhum]

So, I thought one thing and I wrote something else so I will amend my previous statement, sorry for this

No worries! Just trying to understand and adapt based on that. Thanks for giving me further details promptly!

The wait call in both the destructor and the shutdown of the application was there to ensure that all requests fired before exiting and for no other reason.

Got it, thanks.

Actually the problem of requests that are not sent until the shutdown in long-running processes is not fixed with this PR as it's missing the implementation of the close method.

Speaking of the default implementation (using ClientBuilder or Sentry::init, thus using HttpTransport), it is fixed (in current 2.2), as now wait is called on the promise directly in the send method
And since HttpTransport is never instantiated with the deprecated $delaySendingUntilShutdown set to true, it works as well for long running applications, because events are sent out immediately.

If you rely on the "deprecated" behavior (the default) of waiting until the shutdown of the application

As I tried to point out above, it isn't very easy to rely on the default behavior of the HttpTransport class, as it isn't straight forward to create your own object and provide it to ClientBuilder (thus: #855).

in a long-running application (e.g. a queue) and you are not using any framework integration (sentry-laravel or sentry-symfony) then you will face the problem

You will only face the problem, when implementing providing your own instance of HttpTransport using the deprecated behavior (neither of these integrations does so btw.).

What I'm trying to say is: default usage of sentry-php

1. Sends out each event immediately and synchronously
2. ClosableTransportInterface is only needed for custom Transport implementations that send out events asynchronously in asynchronous applications ReactPHP style. Implementing ClosableTransportInterface for the existing HttpTransport would be a noop when $delaySendingUntilShutdown is false or iterating over all promises and calling wait (like currently done in cleanupPendingRequests)

Are we now on the same page? :)

[mfb]

Stepping back from the discussion here, I wanted to provide some sample real-world numbers on two different HTTP performance improvements:

• Parallel requests: Send events in parallel when flush is called and at process shutdown. This offers a speed gain if processes trigger multiple events, since any wait time for Sentry server is delayed and then executed in parallel. In PHP, can be implemented with PHP streams or cURL.
• Non-blocking ("fire and forget") requests: Send and disconnect; do not wait for a response from Sentry server. This is doable since the library doesn't inform the app whether or not events were sent successfully. In PHP, typically this style of request is implemented using PHP streams.

Some sample metrics for sending four events to my on-premise Sentry server:

• Serial blocking = 0.70 seconds
• Parallel blocking = 0.50 seconds
• Serial non-blocking = 0.44 seconds
• Parallel non-blocking = 0.40 seconds

[helhum]

Thanks @mfb! Sounds reasonable. Have implemented that for sentry-php? If so, it would be great if you could share how (either as code or just the way you did it). Thanks.

[mfb]

Have implemented that for sentry-php?

Not yet, just doing some benchmarking to see what directions are worth exploring.

[ste93cry]

And since HttpTransport is never instantiated with the deprecated $delaySendingUntilShutdown set to true, it works as well for long running applications, because events are sent out immediately.

This is indeed a regression and change of default behavior (what I was trying to explain in the comments I wrote before) and will be fixed in #905. For long-running processes, since PHP doesn't have true async you should implement your own transport that flush events using the CloseTransportInterface::close method where you think it's appropriate. sentry-laravel has an integration that does it automatically if you use the Queue component of Laravel.

Implementing ClosableTransportInterface for the existing HttpTransport would be a noop when $delaySendingUntilShutdown is false or iterating over all promises and calling wait (like currently done in cleanupPendingRequests)

The HttpTransport should implement the CloseTransportInterface interface, but when it will became fully syncronous in 3.0 that method will be a noop as you said. For the actual versions though, it makes sense to be able to flush events on-demand to fix the issue with long-running processes

[helhum]

Perfect! Thanks a bunch!