Fix | Replace Listener For Terminatable Middleware

[Sammyjo20]

Description

This PR aims to fix #30. The issue I recently discovered is that the request logs were slowing down our requests because they were being inserted into the database at the very end of the request lifecycle. This PR removes the EventServiceProvider, Events and Listeners and moves the logic in LogRequest into the LogRequestMiddleware into the terminate method.

I have tested this locally and it definitely runs after the request is returned - however I wonder if there needs to be any specific tests written for it?

Does this close any currently open issues?

Fixes #30

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Any relevant logs, error output, etc?

N/A

Any other comments?

N/A

Checklist

• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[Sammyjo20]

Hey @bilfeldt This is my initial PR for the fix - would love to see what you think

[Sammyjo20]

Self review

[bilfeldt]

I much appreciate this 👍

I do think however that we cannot move all of the logic to the middleware, but we need to use the terminatableCallbacks instead.

I have some use cases where I do not globally add the middleware. Instead I have some logic and only for certain cases then I enable logging using $request->enableLog();

But we can move the function from src/Listeners/LogRequest.php into a utility class and then use that inside our terminatable callback?

please note that this will be a breaking change so if there is another option that is better, let us consider that anyway.

[Sammyjo20]

Self review

[Sammyjo20]

Hey @bilfeldt I have updated the PR to use terminatable instead. The application will now run the LogHandler while it is being terminated, which will record the request logs. Nice find!

[bilfeldt]

I love it @Sammyjo20 👍

Would it be possible to add a test ensuring that the LogHandler is being correctly called/registered?

[Sammyjo20]

Hey @bilfeldt would you mind if I changed this PR to work for v2? We're having issues upgrading to v3.

Update: I think it might just be worth seeing why v3 is having problems. I'll try to debug.

[Sammyjo20]

Self review

[Sammyjo20]

Hey @bilfeldt I have added a feature test which makes an API call and checks that the terminatable middleware is working. I introduced a new ArrayLogger driver which is bound to the application's service provider. I used a scoped-singleton just in case someone uses it in production and it runs into memory leaks. I would rather have the ArrayLogger in the test codebase rather than production but I will leave that up to you.

I moved some of the methods that were on the model to a Loggable trait for reusability.

[bilfeldt]

Hi @Sammyjo20 this PR is fantastic 🥇 It will definitely be a breaking change though, so don't think it can be made compatible with v2.

[Sammyjo20]

Thanks @bilfeldt - glad you like it! Would it be a breaking change if this were merged into v3? In my opinion from working in open source, we're only modifying the internal working of the library, so it should be okay. That being said, we've removed the event which was previously being fired. I highly doubt someone would be tapping into that event themselves, but that is a legitimate concern.

[bilfeldt]

@Sammyjo20 I agree that there is a low chance that people are relying on the stuff that has changed. But removing classes or functions is a breaking change, so this should be a new major release - which I am okay with.

My main concern is currently if I wanna bundle other stuff at the same time in a major release 🤔 Like for example adding the option to queue the actual logging.

[Sammyjo20]

Awesome, makes total sense! The ability for it to be queued would be really handy - but now that it's in a terminatable middleware I don't know how beneficial it would be. It would be pretty easy to implement - in the LogHandler you could wrap the logic in a dispatch(function () {}) or use a queued job internally and if the queue option is set to false you just dispatchSync / dispatch.

I'm happy to add that to this PR if you like?

[bilfeldt]

I'm happy to add that to this PR if you like?

That would be much appriciated @Sammyjo20 👍 Just remember that the job cannot get the request/response object but should get an array of parameters to log.

I think I will be removing this array logger from this PR, simply because I don't wanna have the burden of maintaining it and it is mostly useful in testing.

I also think it would make sense moving almost all filtering/extracting functions from the model and into the new src/Traits/Loggable.php trait.

[bilfeldt]

Hi @Sammyjo20 👋

Wanted to circle back to this PR to get it out the door as I think it will benefit the package a lot 👍

Did you wanna add a queuing option to this pr? If not, then I think I will make the two earlier-mentioned changes and release this:

• Remove the array logger
• Move all filtering/extracting functions from the model and into the new src/Traits/Loggable.php trait.
• Add upgrade guide in CHANGELOG.md

[Sammyjo20]

Hey @bilfeldt I would quite like to get my hands on this feature without using a fork, so if you are okay making the other changes, I won't build the queuing function for now.

Thank you so much for helping with this, almost there!

[bilfeldt]

No worries @Sammyjo20 - I will try if I can get this polished off then.

Are you already using this in production using a fork? If so, have you encountered any problems or do you in fact see a performance improvement?

[Sammyjo20]

Not yet, but I was thinking of swapping it to a fork today as the team would really benefit from the performance improvement. I'm going to need to swap the CorrelationIdServiceProvider with Composer because the logs are really slowing down our parallel tests and causing thousands of lines written - I think it's pretty typical for the service provider boot method to run on every test. In one of my open source packages, I just write a similar check:

https://github.com/saloonphp/laravel-plugin/blob/v3/src/SaloonServiceProvider.php#L55

[Sammyjo20]

Hey @bilfeldt I hope you are well! I was wondering if it would be possible to go through this PR again? I would love to have this feature in the package.

[bilfeldt]

Sorry about the late reply @Sammyjo20 :(

Let us get this out the door an a new version launched.

I like this PR, but I would really like to avoid having to maintain an ArrayLogger because I think that is only relevant in the context of tests. So can we remove that, and instead let the test either actually hit the database or use a mocked logger and assert on that?

[bilfeldt]

Suggested change

	public function createArrayDriver(): ArrayLogger
	{
	return app()->make(ArrayLogger::class);
	}

I don't really wanna have the burden of maintaining an Array logger when I think that is only relevant in the context of tests.

[bilfeldt]

Instead of using the array logger, can we register a mocked logger instance and make an assertion that the logging is done once?

[Sammyjo20]

Unfortunately I don't think I have time to finish this PR so I will be closing this

[bilfeldt]

@Sammyjo20 I already did the changes for the array driver which is the last missing piece. It is laying here waiting for your approval (as I was unable to push to your branch): PlannrCrm#1

Once that it merged, then I think this PR is ready to go.