Skip to content
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

[1.x] Add new EntryRecorded Event #182

Closed
wants to merge 1 commit into from

Conversation

geisi
Copy link

@geisi geisi commented Dec 7, 2023

Description

This PR introduces the EntryRecorded event to the Pulse package, offering developers an easy way to handle and respond to the creation of Pulse Entries.

Use Cases

  • Real-Time Monitoring: Facilitates tracking of Pulse Entries in real-time.
  • Custom Alerts: Developers can set up alerts for exceeding specific thresholds, like request durations, enhancing application performance monitoring.

Assurance of Non-Breaking Changes

  • The addition is a small, backward-compatible enhancement designed to leave existing functionalities intact.

Note on Testing

  • Specific tests for the EntryRecorded event are not included as it leverages core Laravel functionality for dispatching events, which is already well-tested and reliable. If mandatory I can add tests of course.

@jbrooksuk jbrooksuk requested a review from jessarcher December 7, 2023 19:19
@taylorotwell taylorotwell marked this pull request as draft December 7, 2023 20:00
@timacdonald
Copy link
Member

I don't believe we should dispatch the event here.

We want to do everything we can to psh work outside of the request / response lifecycle.

It would be better if we triggered the event from the storage driver, but then it means that every storage driver would need to trigger the event.

It would mean that the event would be triggered during a request if the application was using the storage ingest.

One thing I can think of is only allowing this functionality if the pulse:work command is active. That means any additional work would always be off the main thread.

One other concern I have here is that any listeners of this event might trigger Pulse recorders, e.g., imagine a HTTP request was sent in response to this listener and it was a slow request. We would then need to capture that as well but it would also send Pulse into an infinite loop.

Will think on a solution to all of this and see if we can come up with something that will have the least impact on the main thread and also no create potential infinite loops.

@jessarcher
Copy link
Member

Agree with @timacdonald. I'm not opposed to supporting those use cases, but we'll need to think it through.

@geisi
Copy link
Author

geisi commented Dec 8, 2023

Thank you for your feedback. I am playing around with how Notifications could be implemented, and the infinite loop problem definitely exists :)

I personally like the way to listen directly for Pulse Events, but it might be getting too confusing for developers.
Another approach that will not need any changes to the Pulse main package would be to query the pulse_entries table via a scheduled job and completely skip internal Pulse logic.

I think this is the most robust way to do that.

@timacdonald timacdonald changed the title Add new EntryRecorded Event [1.x] Add new EntryRecorded Event Dec 11, 2023
@timacdonald
Copy link
Member

timacdonald commented Dec 12, 2023

@geisi this is 100% on our radar and something we will look into. It will likely be a bigger project, so I'm going to close this one for now.

You will have to poll the pulse_* tables for now or alternatively you could create either a "decorator" driver or extend the existing storage driver to fire the event for you.

use App\Events\EntriesRecorded;
use Laravel\Pulse\Contracts\Storage;
use Laravel\Pulse\Storage\DatabaseStorage;

$this->app->bind(Storage::class, new class extends DatabaseStorage {
    public function store(): void
    {
        parent::store();

        EntriesRecorded::dispatch($entries);
    }
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants