-
Notifications
You must be signed in to change notification settings - Fork 20
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
bug: slower response times when events api are enabled #203
Comments
Update: Unless, I'm missing something here, I don't see an issue with our original approach to use a shutdown hook:
There are two cases where the events will be sent before the response is sent to the client:
If the user is consistently seeing significant slower response times when enabling Insights, it is possible that they are queueing a lot of events before responding to the client, hence triggering the 1st case from above. In this case, they can simply increase the threshold. |
@subzero10 so that I'm clear: In cases 1 and 2 above, we're blocking the response while the events are being sent? Or are we sending the events asynchronously, i.e. in a separate thread? We should never be sending events synchronously imo. Am I missing something? |
Yes, that’s correct. PHP applications do not use multi-threading. Each request is handled by a separate process, typically managed by tools like PHP-FPM. To achieve non-blocking processing, PHP applications leverage a worker process (e.g., a job queue service) to handle tasks asynchronously. However, not all PHP applications use such queues, and identifying their presence can be non-trivial. Though, for Laravel apps specifically, detecting a queue setup should be straightforward.
Agreed, in most cases involving HTTP requests, synchronous event sending is suboptimal due to the potential for delayed responses. However, synchronous processing is sometimes acceptable, such as when running PHP outside the context of an HTTP request. In those scenarios (e.g., CLI processes or worker services), response time is less critical, as each job runs independently and on its own process. Our current approach provides a general-purpose solution for PHP and Laravel applications:
Trade-off: Potential Improvements:
If you agree with the above, I can create issues to start working on them. Let me know if there’s anything else you’d like clarified or adjusted. |
The big change here from how it behaves currently (and the solution to this users problem) would be to use the queue system, right? It already queues the events in memeory and sends them in batches after the response is sent (unless there the batch fills up)? |
Correct. Basically, I'm thinking that we would implement more than one mechanism to send events and choose the best one based on configuration. I don't know yet if this can be done entirely automatically without any further configuration (current research shows that we can't) because even though Laravel's configuration may point to a queue, the queue process may not be running (i.e. this is a local/dev/staging/testing environment). This may cause confusion. We will have to make sure this is well documented.
We don't know for sure that this user's problem was the fact that they were generating too many events in a short amount of time. If indeed this was the problem, they could configure the batch threshold to be higher (the default is 50) and their problem would go away even with the current implementation.
Yes, correct. This is the job of the BulkEventDispatcher.php:
|
I'm fine with bumping the default threshold to 100 and recommending that the impacted customer try adjusting that setting. Instead of trying to auto-detect (and use) a queueing system, how about we make it an option in the config to enable a queueing system (from a list of supported systems) for sending payloads? Then the customer can choose to use/enable the plugin. This would be analogous to a Ruby user opting to use Sidekiq jobs for sending events. We could also point customers to using Vector as a proxy: configure the PHP app to deliver payloads to a locally-running Vector instance that is configured to send events on to our API. This would avoid potential network delays, which might have contributed to the reported slowdown. |
👍 OK I will submit a PR for this.
After some more thinking, I'm leaning towards this approach as well. Auto-detection will not always work - in that case we will have to fall back to the default implementation. But this will introduce another debugging step to find out which implementation was used. It will probably create more problems along the way, such as queuing Honeybadger events to the wrong queue. We could create a new implementation and allow people to enable it instead of the default approach and allow configuring it as well (i.e. queue connection and queue name). @joshuap, if you are OK with this, we can create an issue and start working on it.
Correct, that's also an option. I'm thinking that it would be a solution for people that have already witnessed the value of Honeybadger Insights (or are familiar with concepts such as wide events) and would go the extra effort of setting up Vector in their infrastructure. |
I'm good with this. I agree that it should be user-configured instead of automatically detected. |
A user reported they noticed 300ms slower response times when they enabled Insights.
The user suggested to send the notice after the response is returned.
The text was updated successfully, but these errors were encountered: