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

Flush loggers on kernel.reset #279

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@dnna
Copy link

dnna commented Sep 9, 2018

Fixes php-pm/php-pm-httpkernel#134 and php-pm/php-pm-httpkernel#62

When using PHP-PM the worker does not die at the end of the request, so the shutdown functions registered by monolog are never triggered (so for example BufferHandler never flushes). This PR partially fixes the problem by flushing the loggers containing a flush function at the end of each request.

I did not use the close function (mentioned in Seldaek/monolog#1053) because some loggers use sockets or other connections that would be closed in this case and not be reusable in the next request.

I'm not sure if this is the best solution to this problem, happy to discuss alternatives if there is a better way.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Sep 9, 2018

Shouldn't we use the kernel.reset tag instead?
See symfony/symfony#24155

@dnna

This comment has been minimized.

Copy link

dnna commented Sep 9, 2018

Actually I tried kernel.reset, but it didn't seem to trigger the flush when an exception occurred.

@dnna

This comment has been minimized.

Copy link

dnna commented Sep 9, 2018

Never mind, I see the issue now. kernel.reset triggers on the next request, not the current one, so I actually needed to perform a second request for the logs of the first request to flush.

Its not ideal, but it still solves the problem adequately and kernel.reset was designed specifically for PHP-PM cases, so I will adapt the code to use that instead.

@dnna dnna changed the title Flush loggers on kernel.terminate Flush loggers on kernel.reset Sep 13, 2018

@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Sep 21, 2018

I would use Seldaek/monolog#997 instead of using flush

@dnna

This comment has been minimized.

Copy link

dnna commented Sep 21, 2018

Yeap, that solution looks better. Hope it gets merged soon, and I'll change this PR to use that one :)

@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Sep 21, 2018

I hope too :)

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Dec 26, 2018

reset is now part of Monolog 1.24.0 and above.. Would be good to update this PR. You can test for ResettableInterface being present.

@Seldaek Seldaek added this to the 3.4 milestone Dec 26, 2018

@Seldaek Seldaek added the Feature label Dec 26, 2018

@dnna

This comment has been minimized.

Copy link

dnna commented Dec 26, 2018

Perfect, will update it in the next few days.

@dnna

This comment has been minimized.

Copy link

dnna commented Jan 1, 2019

@Seldaek @lyrixx @nicolas-grekas I have updated this PR to use the ResettableInterface.

There is a deprecation notice in the PHP 7.1 tests, but I don't think its related with this PR, could you guys have a look?

@lyrixx

lyrixx approved these changes Jan 3, 2019

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Jan 13, 2019

Sorry for the delay. This looks better already but I am afraid attaching kernel.reset to all handlers means that some of them will end up being reset multiple times. For example a GroupHandler or BufferHandler when it gets reset it resets the handlers it contains. This is so you can simply call Logger::reset and then it trickles down to all handlers from there even tho the Logger doesn't know about all of them.

I think it would be preferrable to set the tag on the Logger instances to avoid these multi-resets.

@dnna

This comment has been minimized.

Copy link

dnna commented Jan 14, 2019

Hmm is it a good idea to explicitly set the tag on specific loggers? Won't this be something we'll have to keep track of whenever a new logger is added?

If reset is designed to trickle down to all handlers, maybe it would be better to just trigger reset on the main handler (if it has ResettableInterface) and let it trickle down to the rest?

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Jan 14, 2019

Yes.. this is kinda complicated by the fact that some handlers can be only in some loggers, and not in all. But I guess resetting only the main logger would probably make sense, otherwise we end up with duplicate resets again. Channel-specific handlers might be needing a manual reset from the user in that case.. that's a trade-off we may have to live with unless someone feels like figuring out which handlers are not in the default logger chain and then tagging those for reset too.. I'm not sure what's worse between maybe double resets and maybe no reset at all.

@dnna

This comment has been minimized.

Copy link

dnna commented Jan 14, 2019

I think just resetting the main logger and having it trickle down the chain would resolve most of the use cases requiring a logger reset anyway.

It will certainly improve the situation compared to what it is currently, so maybe we should proceed with that?

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Jan 14, 2019

Yes that sounds good to me :)

@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Jan 14, 2019

We could leverage the autoconfiguration, isn't ?

@stof

This comment has been minimized.

Copy link
Member

stof commented Jan 14, 2019

I think the DI extension already knows which handlers are nested ones. So it could manage the tagging of non-nested ones.

@dnna

This comment has been minimized.

Copy link

dnna commented Jan 15, 2019

OK, so we'll leverage the the DI extension to find all the top-level handlers (like main) and reset them, given that they implement ResettableInterface. I'll try to work on that in the next few days and update the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment