-
-
Notifications
You must be signed in to change notification settings - Fork 450
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Events are sent only on shutdown: does not work in combination with async or long running processes like queue workers #811
Comments
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 |
@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. |
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 |
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... |
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.
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 |
I can try it out for you once the PR is merged to see if it works. |
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 |
@ste93cry so is it all ready, and it just needs testing? |
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...) |
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. |
The
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.
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. |
When can you fix bugs that you can't use in swoole? set_exception_handler and __destroct can't work |
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 |
@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 |
@ste93cry IMHO, it would be cool if you could open issues on GitHub for those you know but have not been opened publicly. |
I'm having a déjà vu on this discussion 😄 Breaking BC is a no-go, we are striving to follow semver and we will continue doing it.
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:
Also, it's cool if you can keep BC 👍 But if fixing something that does not work in the 1st place required breaking BC please DO :-) |
Or deprecate it by saying it's bugged and invite pple to use the new way. |
I implemented a workaround which might be interesting: I created a HttpClient plugin which waits for each promise and swaps it with a
And, instead of initializig sentry with the usual Sentry::init():
I hope it's useful for somebody! |
@palicao that’s actually way better than my current work around 👍 |
For Laravel, the approach I'm currently testing is to apply within ExceptionHandler:
|
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! |
Issue is still top-priority for |
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 |
Hello can anybody help with a solution for Laravel... Doesn't seem to work yet |
This issue should have been fixed a long time ago, also for Laravel. Please open an issue in |
I can confirm that this works now and out of the box (for Octane/Swoole at least) 👍 |
(...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.The text was updated successfully, but these errors were encountered: