Sentry-php: Events are sent only on shutdown: does not work in combination with async or long running processes like queue workers

[amcsi]

(...but Rollbar does.)

The way Sentry PHP currently works is that during a request, whenever an error would be sent by Sentry, then rather than sending it off to the Sentry servers right away, it collects all the errors that it would send all in a big batch at the end of the request here.

...which is nice, but there's a problem: it doesn't actually do these batches at the end of the request, but rather on shutdown.

Unfortunately that is a problem when using a Swoole or ReactPHP style project, because the server never shuts down until the user force-terminates the process; thus the errors never get sent out to Sentry.

Also, you would think that since there's a destructor as well also for sending all the batches to Sentry. But unfortunately it doesn't work, because HttpTransport is never destroyed, because Hub has a static property called currentHub which holds an instance of itself which holds an instance of \Sentry\Client which holds an instance to \Sentry\Transport\HttpTransport.

I suppose a workaround could be that after every request, \Sentry\State\Hub::setCurrent(new \Sentry\State\Hub::setCurrent()); could be called which would result in HttpTransport getting destroyed. But it would be great if there weren't a static reference to HttpTransport in the first place so that its destructor would end up getting called.

[ste93cry]

Thanks for the report, we are aware that this is a big problem due to us incorrectly implementing the asynchronous sending of the events and I’m working on a fix which has a high priority on my to-do list. Since this is going to require a lot of changes in the API I didn’t find the time yet to open the PR, but on my forked repository there is a branch with the WIP changes that should solve the issue and allow the correct integration of the SDK with ReactPHP and similar libraries. I really hope to be able to submit a PR soon for review

[amcsi]

@ste93cry I'm glad to hear you are aware of it.

Are you sure it's that difficult to fix by the way? Like I wrote, according to your code, handling the async events seems to be handled on destruction as well; the only problem seems to be the fact that you're holding a static reference that indirectly holds onto the HttpTransport instance. If you could somehow avoid that, then probably destruction would happen as expected, and async requests would go through as desired.

[Jean85]

The transport and the client have to be hold on to, or otherwise they would have to be constructed each time.

We're aware of this since it lead to a similar issue in Laravel queue workers, see getsentry/sentry-laravel#228. We're trying an approach in #799, and in general a simple flush() should suffice as a way to send events when needed.

[mfb]

Would it make any sense for Sentry to automatically flush if it's adding an event and there are already N events are in the queue or an event older than M age is found in the queue? The reason I ask is that if you're implementing Sentry in a large set of apps, it's difficult to ensure that developers are calling flush when they need to...

[ste93cry]

Would it make any sense for Sentry to automatically flush if it's adding an event and there are already N events are in the queue or an event older than M age is found in the queue

It would make sense, but the limit should be configurable by the user because we cannot impose it, and it would mean just another option (and we have a lot of them already). Also it would mean that we have to support the feature across all the transports and it will be more the time we spend doing it than the user to build its own transport.

The way Sentry PHP currently works is that during a request, whenever an error would be sent by Sentry, then rather than sending it off to the Sentry servers right away, it collects all the errors that it would send all in a big batch at the end of the request here.

After investigating, I realized that that is just a side-effect of what I really wanted to do, which was sending events immediatly (because the HttpTransport was meant to be syncronous). When I coded that part I though that the promises started resolving immediatly (since in PHP the concept of async does not exists) and the code that runs in the shutdown function or in the destructor of the class was there just to ensure that the sending of all requests finished before terminating the application. I never realized that the requests were not starting until one of those functions were called and I'm really sorry for this. The referenced PR attempts to solve the problem one time for all by exposing some new methods that will return the promises from the async transports, this way you can await them when you want and integrate the ticking of the queue of HTTP requests with your event-loop of choice. Since I have no experience using libraries or frameworks like ReactPHP or Swoope, it would be great if people who are having the same problem could try my fix and leave a feedback

[amcsi]

I can try it out for you once the PR is merged to see if it works.

[ste93cry]

Obviously the testing should be done before it's merged, otherwise we risk to merge something that doesn't fix the issue or is broken again. If you are willing to test it, you can try by using the branch of my PR in your composer.json

[amcsi]

@ste93cry so is it all ready, and it just needs testing?

[ste93cry]

It's missing some polishing, tests and documentation on the methods (I like to document everything!), but the idea to get the issue fixed should be implemented. As I said I don't have any experience with ReactPHP or Swoole, so I need help testing and validating that such solution works and permits to fix the issue. I don't recomment though to try it in production since it may break things (it shouldn't...)

[ghost]

Just so by the way, for my project I've written a transport based on reactphp-buzz, which does asynchronous I/O. The events are immediately sent and on shutdown all pending events are ensured to be sent by concurrently sending all events and blocking the process until the event loop is empty (since it's a new event loop on shutdown, once all events are sent, the event loop stops ticking).

It would be actually useful if the sentry transport would send the events inmediately. However from what I remember it's an actual implementation issue of httplug (which is broken and fatally misengineered for async environments).

In my opinion choosing httplug was a bad decision.

[ste93cry]

It would be actually useful if the sentry transport would send the events inmediately.

The HttpTransport was never meant to be asyncronous and I totally failed to implement it the way I though it would work in my head :cry: The typehint against the HttpAsyncClient interface is there only because it's easier to make an asyncronous HTTP client to act as a syncronous one and not because you should expect the transport to act async. Another mistake I made is that I never (yes, again) noticed that the request did not start until you call wait. The fact that I coded the cleanupPendingRequests method was only to ensure that in any case at the end of the application all requests were sent and not to delay the sending until that time. Indeed I learned something new from this (bad) experience.

However from what I remember it's an actual implementation issue of httplug (which is broken and fatally misengineered for async environments).

Mmm, what are you referring to? Httplug relies on a set of adapters for other clients, so if there is a bug it's on the implementation of the client itself.

In my opinion choosing httplug was a bad decision.

At that time there was no PSR about the HTTP client, so the only way to ensure that everyone could use whichever HTTP client they wanted we choose to use Httplug, which by the way is used by many other (even important) projects and libraries. I won't say it doesn't have some drawbacks in the implementation or that is perfect because I struggled too with some bugs or issues, but they were nothing that I could not resolve, maybe even contibuting to the project itself. It would be cool to have a standardized PSR for async clients too, but it doesn't exists yet.

[601157475]

When can you fix bugs that you can't use in swoole? set_exception_handler and __destroct can't work

[ste93cry]

We are working to find a solution, and actually there are a few proposal open to discussion, but since this involves unified API this is not as simple as it seems. Also, making something that does not break BC in this case is harder than usual, so there is no ETA at the moment

[B-Galati]

@ste93cry Just had the same issue within a worker.

I think it's ok to break BC if it's broken anyway.

Also want to add that with the current implementation the SpoolTransport does not make a lot of sense.

[B-Galati]

@ste93cry IMHO, it would be cool if you could open issues on GitHub for those you know but have not been opened publicly.

[ste93cry]

I think it's ok to break BC if it's broken anyway.

I'm having a déjà vu on this discussion :smile: Breaking BC is a no-go, we are striving to follow semver and we will continue doing it.

IMHO, it would be cool if you could open issues on GitHub for those you know but have not been opened publicly.

I'm not sure I get what you mean here. The proposals about how to solve this specific issue are two public PRs

[B-Galati]

I'm not sure I get what you mean here. The proposals about how to solve this specific issue are two public PRs

@ste93cry Ok thank you, my bad! I was saying that because you already knew about the issue but no issue was opened:

Thanks for the report, we are aware that this is a big problem due to us incorrectly implementing the asynchronous sending

Also, it's cool if you can keep BC :+1: But if fixing something that does not work in the 1st place required breaking BC please DO :-)

[B-Galati]

Or deprecate it by saying it's bugged and invite pple to use the new way.

[Jean85]

@B-Galati this is the issue, I'll change the title to make it more clear. As for the proposed solutions, these are the pending PRs: #799 & #813

[palicao]

I implemented a workaround which might be interesting: I created a HttpClient plugin which waits for each promise and swaps it with a FulfilledPromise. I am not 100% satisfied because, in a long running process, I am still bloating the memory with useless data, but I found it easier than trying to use my own client or my own transport (every class is final otherwise it would have been way easier).
The code is, more or less:

use Exception;
use Http\Client\Common\Plugin;
use Http\Promise\FulfilledPromise;
use Http\Promise\Promise;
use Psr\Http\Message\RequestInterface;

class SynchronizeHttpClientPlugin implements Plugin
{
    public function handleRequest(RequestInterface $request, callable $next, callable $first): Promise
    {
        /** @var Promise $promise */
        $promise = $next($request);

        try {
            $promise->wait();
        } catch (Exception $e) {
            // do nothing...
        }

        // we return an already fulfilled promise so that the sentry client is happy
        return new FulfilledPromise(null);
    }
}

And, instead of initializig sentry with the usual Sentry::init():

$options = []; // my options here...
$client = ClientBuilder::create($options)
        ->addHttpClientPlugin(new SynchronizeHttpClientPlugin())
        ->getClient();

Hub::setCurrent(new Hub($client));

I hope it's useful for somebody!

[RageZBla]

@palicao that’s actually way better than my current work around :+1:

[eithed]

For Laravel, the approach I'm currently testing is to apply within ExceptionHandler:

public function report(Exception $exception)
    {
        if (app()->bound('sentry') && $this->shouldReport($exception)) {
            app('sentry')->captureException($exception);

            if (app()->runningInConsole()) {
                app('sentry')->getClient()->getIntegration(\Sentry\Laravel\Integration::class)->flushEvents();
            }
        }

        parent::report($exception);
    }

[mfb]

Still need to update the sentry drupal integration - which has a flush() method for long-running processes - from sentry-php 1.x to 2.x, so I was hoping this would be addressed in the library soon? :) but if not, the sentry-laravel workaround looks like it could work, thanks for posting!

[ste93cry]

Issue is still top-priority for 2.2 and in the next days I'm gonna hopefully update my PR with the changes that will fix the issue on time for all, but due to the vacation period I don't know when it will be merged. The release plan in my head is to merge all pending PRs for that version along with this fix and then release the new version

[ste93cry]

Closing this issue as it has been solved in the referenced PR. I'm sorry this has been a so long outstanding issue, we will release the new version asap

[paschaldev]

Hello can anybody help with a solution for Laravel... Doesn't seem to work yet

[ste93cry]

This issue should have been fixed a long time ago, also for Laravel. Please open an issue in getsentry/sentry-laravel if you are still having issues