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

Queued Auditing #846

Merged
merged 14 commits into from
Dec 14, 2023
Merged

Conversation

Orrison
Copy link
Contributor

@Orrison Orrison commented Aug 22, 2023

Per discussions such as #562, this sets up auditing to be dispatched on the Queue.

By default, the listener will go onto the sync connection. So if no configuration is done, the package will work as it did before, auditing immediately in the current request.

Once you configure the audit.queue.connect to be something other than sync, the audits will then be dispatched to be processed asynchronously.

Customization of the queue it is put on, as well as the delay, is also possible.

It is set up similarly to the Auditing/Audited flow. An Event (DispatchingAudit) is fired where you can prevent the auditing dispatch from happening by having a listener return false.

Signed-off-by: Kevin Ullyott <[email protected]>
Signed-off-by: Kevin Ullyott <[email protected]>
Signed-off-by: Kevin Ullyott <[email protected]>
Signed-off-by: Kevin Ullyott <[email protected]>
Signed-off-by: Kevin Ullyott <[email protected]>
Signed-off-by: Kevin Ullyott <[email protected]>
Signed-off-by: Kevin Ullyott <[email protected]>
@Orrison
Copy link
Contributor Author

Orrison commented Aug 24, 2023

Pushed a fix that should resolve one of the test errors. Unsure about the PHP 7 failures though. Having trouble getting this package to work in PHP 7

Signed-off-by: Kevin Ullyott <[email protected]>
@Orrison
Copy link
Contributor Author

Orrison commented Aug 24, 2023

Copied what y'all were doing in the GitHub Workflow setup locally and fixed the PHP 7.4 errors. I believe all tests should pass now if you want to run the test suite again.

@Orrison
Copy link
Contributor Author

Orrison commented Sep 8, 2023

@MortenDHansen just wanted to check in to see if there is anything I can do to move this queued auditing concept along in the package.

Even if it requires me to change how I implement it here, I am completely open and available to do so.

@mohamedgaber-intake40
Copy link

may i know , why this pull request is not merged ? i think this feature is needed to be exists in the package

@Orrison
Copy link
Contributor Author

Orrison commented Dec 7, 2023

@MortenDHansen, I just want to ping one last time to see whether or not this PR could go anywhere here.

Queue auditing would be an amazing improvement. Could you let me know if there is any feedback on my implementation, I am happy to put time into going a different direction if you feel that it's required.

Thank you!

@MortenDHansen
Copy link
Contributor

It seems like it would work. I was worried that it would solve too narrow a case by only augmenting the actual data saving. I will get to try it out and then we could probably merge it. I don't see anything that should break, but i have to try it :)

@MortenDHansen MortenDHansen merged commit cac184d into owen-it:master Dec 14, 2023
13 checks passed
@parallels999
Copy link

This version gave me problems, I will try to locate them and give my feedback

@MortenDHansen
Copy link
Contributor

This version gave me problems, I will try to locate them and give my feedback

Please do! I didn't have any issues with it on my applications, but if we have issues that are hard to track down, we should roll an update without the queue system asap. If it breaks for users, then we need to make it opt-in

@parallels999
Copy link

I found my problem
image
I have an Audited event listener

'OwenIt\Auditing\Events\Audited' => [
    'App\Listeners\AuditedListener',
]

But seems that DispatchAudit is triggered first, and Audited no longer does what it did before,
Works again, if i revert

-$this->dispatchAudit($model->setAuditEvent('created'));
+Auditor::execute($model->setAuditEvent('created'));

I don't know if the problem is that I'm handling the event listeners wrong, but they had worked for me since several major previous versions without modifying anything.

@Orrison
Copy link
Contributor Author

Orrison commented Dec 15, 2023

I found my problem image I have an Audited event listener

'OwenIt\Auditing\Events\Audited' => [
    'App\Listeners\AuditedListener',
]

But seems that DispatchAudit is triggered first, and Audited no longer does what it did before, Works again, if i revert

-$this->dispatchAudit($model->setAuditEvent('created'));
+Auditor::execute($model->setAuditEvent('created'));

I don't know if the problem is that I'm handling the event listeners wrong, but they had worked for me since several major previous versions without modifying anything.

Where are you having these issues? In testing or in staging/production?

The dispatchAudit call just triggers a queued listener that calls Auditor::execute, which works the same as before, performing the audit and then dispatching the Audited event.

@parallels999
Copy link

parallels999 commented Dec 15, 2023

Where are you having these issues? In testing or in staging/production?

everywhere

dispatching the Audited event

Yes, I don't know what's happening, maybe it's as if the order of events has changed, or as if the changes to the model are not made in the same instance.

I will try to do more tests and find a solution.

@parallels999
Copy link

question, is changing contracts a breaking change?

@danharrin
Copy link
Contributor

lgtm

@parallels999
Copy link

@Orrison I already isolated my problem
Look at the 2737, is laravel recently created model(spl_object_id), using sync
right v13.6.0
left v13.6.0 just i did remove implements ShouldQueue on ProcessDispatchAudit,
image
Without queue, spl_object_id remains, it means that the instance reference is the same in all events
With queue, spl_object_id changes on ProcessDispatchAudit to 2782, means that there is a second clone of my model, so the model on my audit event listeners is not the same as the other laravel events, so the process stop with 500 error

we need to make it opt-in

Maybe

@mattvb91
Copy link

question, is changing contracts a breaking change?

Seems to be. I've just upgraded cause phpstan is failing and now all of my tests where I validate audits were created are failing. Havent had a chance to look closer yet

@cosmastech
Copy link
Contributor

Our pipelines are also failing with this change.

@mattvb91
Copy link

Fixed for me in 13.6.1 thanks everyone

@Tofandel
Copy link

Tofandel commented Dec 19, 2023

Heey hey hey, this is a breaking change (interface signature change) and should have been released in 14.x, I mean there is a reason there is Contracts in the namespace, because you can't change it without breaking stuff

It should be reverted on 13.x (ASAP because it will cause yet another breaking change for people who solved the issue in their project but they will be able to upgrade to 14.x to solve it, but solve a lot of headaches for peolpe who have yet to upgrade) and rereleased on 14.x

Otherwise anyone implementing this interface will get a fatal error in php Declaration of App\Resolvers\UserResolver::resolve() must be compatible with OwenIt\Auditing\Contracts\UserResolver::resolve(OwenIt\Auditable $auditable)

@chrisRedwine
Copy link

I believe this also broke Lumen compatibility, with the introduction of Illuminate\Foundation\Events\Dispatchable

@erikn69
Copy link
Contributor

erikn69 commented Dec 19, 2023

@chrisRedwine #883

@MortenDHansen
Copy link
Contributor

And now v13.6.2 should fix the contracts issue so that it doesn't break. I haven't tried it on Lumen, so if anyone experiences issues there, let us now!

Big thanks to @erikn69 again ❤️

@kenfai
Copy link

kenfai commented Dec 21, 2023

The change has produced a lot of logging when using Supervisor to manage process queues and job logging, and Laravel Excel package to import large amount of data to the database.

[2023-12-21 14:36:32][] Processing: OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:32][] Processed:  OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:32][] Processing: OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:32][] Processed:  OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:32][] Processing: OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:32][] Processed:  OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:32][] Processing: OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:32][] Processed:  OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:32][] Processing: OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:32][] Processed:  OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:32][] Processing: OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:32][] Processed:  OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:32][] Processing: OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:32][] Processed:  OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:32][] Processing: OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:32][] Processed:  OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processing: OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processed:  OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processing: OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processed:  OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processing: OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processed:  OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processing: OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processed:  OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processing: OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processed:  OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processing: OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processed:  OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processing: OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processed:  OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processing: OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processed:  OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processing: OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processed:  OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processing: OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processed:  OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processing: OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processed:  OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processing: OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processed:  OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processing: OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processed:  OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processing: OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processed:  OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processing: OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processed:  OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processing: OwenIt\Auditing\Listeners\ProcessDispatchAudit
[2023-12-21 14:36:33][] Processed:  OwenIt\Auditing\Listeners\ProcessDispatchAudit

I understand this may not be related to Laravel-Auditing library, but is there anyway to disable logging for the above audit listener class by default?

@MortenDHansen
Copy link
Contributor

The change has produced a lot of logging when using Supervisor to manage process queues and job logging, and Laravel Excel package to import large amount of data to the database.

[2023-12-21 14:36:32][] Processing: OwenIt\Auditing\Listeners\ProcessDispatchAudit

I understand this may not be related to Laravel-Auditing library, but is there anyway to disable logging for the above audit listener class by default?

Hi, could you please create an issue for it? 🙂

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.