Added a new ResettableInterface and implemented it where possible. #997

lyrixx · 2017-05-31T15:51:03Z

Added a new ResetableInterface and implemented it where possible.

When one use Monolog in a long process like an AMQP worker with a
FingersCrossedHandler or BufferHandler there is a drawback: as soon as there
is an AMQP message that generate a log >= error (for example), all next AMQP
messages will output logs, even if theses messages don't generate log where
level >= error.

In the same context there is a drawback for processor that add an UUID to the
logs. The UUID should change for each AMQP messages.

This patch address this issue with a new interface: ResettableInterface interface.
Side note: reset(), flush(), clear(), are already used in Monolog. So
basically, one can use the reset() on the Logger and on some
Handlers / Processors.

It's especially useful for

the FingersCrossedHandler: it close() the buffer, then it clear() the buffer.
the BufferHandler: it flush() the buf

lyrixx · 2017-06-01T14:30:47Z

I forgot to say that we are using this "feature" for years at SensioLabs. But in a very hacky way (with reflection). IMHO, it's better to expose a clean API. That's why I created this PR.

lyrixx · 2018-04-05T13:15:22Z

This PR needs a rebase, but before finishing it I would like to be sure If it will/could be merged.

More over Symfony get a new component: Messenger. So (I hope so) AMQP (or other queue system) will become more standard. So people will expects a nice logging system.

ping @fabpot @nicolas-grekas @sroze

Next step: wire this restart method after each message processed in the Messenger Component

nicolas-grekas

LGTM, just one comment

nicolas-grekas · 2018-04-05T13:51:50Z

src/Monolog/Processor/UidProcessor.php

@@ -43,4 +45,9 @@ public function getUid(): string
    {
        return $this->uid;
    }
+
+    public function restart(int $length = 7)


I'd suggest to store the length as a property instead of adding in here

agreed. None of the APIs are calling it with a value anyway, and we want it to be called with the original config.

nicolas-grekas · 2018-04-05T13:52:48Z

Next step: wire this restart method after each message processed in the Messenger Component

and on container reset

stof · 2018-04-05T13:57:12Z

Why is it in the 2.0 milestone rather than adding this new feature in 1.x (2.0 is kind of stale, and Symfony will have a hard time moving to it due to the non-continuous migration path for handler authors currently)

stof · 2018-04-05T13:59:17Z

src/Monolog/Logger.php

@@ -325,6 +325,21 @@ public function addRecord(int $level, string $message, array $context = []): boo
        return true;
    }

+    public function restart($a = null)


why having an argument here ?

stof · 2018-04-05T14:00:30Z

src/Monolog/Processor/UidProcessor.php

@@ -43,4 +45,9 @@ public function getUid(): string
    {
        return $this->uid;
    }
+
+    public function restart(int $length = 7)


agreed. None of the APIs are calling it with a value anyway, and we want it to be called with the original config.

stof · 2018-04-05T14:01:37Z

src/Monolog/Processor/UidProcessor.php

+
+    public function restart(int $length = 7)
+    {
+        $this->__construct($length);


I would rather move the uid generation to a private method and call it, rather than reusing __construct (which will be necessary anyway when applying the suggestions). I don't want to push people to think that calling __construct as a method is a supported usage of the library.

lyrixx · 2018-04-05T14:06:51Z

Why is it in the 2.0 milestone

I would prefer 1.X yes.

stof · 2018-04-05T14:13:47Z

Well, I think the 2.0 milestone is because this was submitted to master.

sroze

I don't see either why it would be 2.0 as there is no harm in having that now.

src/Monolog/RestartableInterface.php

sroze · 2018-04-05T14:13:48Z

src/Monolog/Processor/UidProcessor.php

@@ -43,4 +45,9 @@ public function getUid(): string
    {
        return $this->uid;
    }
+
+    public function restart(int $length = 7)


It's very unlikely that this is going to have its argument populated anyway because the chains don't forward the arguments. Instead, I'd use strlen($this->uid) as __construct argument (or store $length as a property).

getting the length of the existing uid is a nice trick to get it 😄

lyrixx · 2018-04-05T15:13:07Z

Thanks. I have addressed your comments and now this PR targets the 1.X branch.

sroze · 2018-04-05T15:41:19Z

src/Monolog/RestartableInterface.php

@@ -0,0 +1,25 @@
+<?php declare(strict_types=1);


Is that OK even if Monolog's 1.x's target is PHP 5.3 ?

lyrixx · 2018-04-05T16:30:04Z

There is a failure on HHVM, but for another test. So this PR is ready.

fabpot · 2018-04-06T06:13:13Z

I would recommend to drop HHVM support as almost all project have done by now (even on 1.x).

lyrixx · 2018-06-07T16:01:15Z

Rebased and green.

@nicolas-grekas How could we handle your new ResettableInterface from symfony/symfony#27093 ?

Include the deps here
create a bridge/decorator in Symfony ?
???

nicolas-grekas · 2018-06-07T16:10:21Z

s/RestartableInterface::restart()/ResettableInteraface::reset()/?
then later one add the Symfony interface on the classes here (or as a child of the Symfony interface?)

lyrixx · 2018-06-07T16:28:56Z

Hmm, reset is already used in monolog with a different meanings.

IIRC, this is why I choose restart instead of reset

nicolas-grekas · 2018-06-07T16:36:38Z

a bridge/proxy would do it also

Seldaek · 2018-06-18T08:50:31Z

Hey, this is something I wanted to support in 2.0, as well as having a Logger::close() to be able to close all handlers (as 2.0 has close in the HandlerInterface).

I am not against supporting this in 1.x though, but I wonder if we shouldn't call this reset rather than restart. The only name conflict is in the FingersCrossedHandler but really the only reason it has a reset() is to do exactly the same thing as this PR intends, so I don't think it's a problem at all, it just generalizes the concept to the rest of Monolog.

lyrixx · 2018-07-06T08:59:12Z

I updated the PR to rename ResartableInterface to ResetableInterface

nicolas-grekas · 2018-07-06T09:04:01Z

s/ResetableInterface/ResettableInterface

lyrixx · 2018-07-06T09:09:21Z

@nicolas-grekas Thanks ; done

nicolas-grekas · 2018-07-06T09:12:52Z

src/Monolog/ResettableInterface.php

+ * Handler or Processor implementing this interface will be reseted when Logger::reset is called.
+ *
+ * Reseting an Handler or a Processor usually means cleaning all buffers or
+ * reseting in its internal state.


double "t": resetted, resetting

nicolas-grekas · 2018-07-06T09:13:17Z

tests/Monolog/Handler/FirePHPHandlerTest.php

@@ -77,7 +77,7 @@ class TestFirePHPHandler extends FirePHPHandler
 {
    protected $headers = array();

-    public static function reset()
+    public static function resetInternalState()


resetStatic?

lyrixx · 2018-07-06T13:26:44Z

@nicolas-grekas @Seldaek Should be ok ;)

nicolas-grekas · 2018-07-06T13:31:15Z

src/Monolog/ResettableInterface.php

+namespace Monolog;
+
+/**
+ * Handler or Processor implementing this interface will be reseted when Logger::reset is called.


resetted ;)
Logger::reset()

I used reset instead of resetted

When one use Monolog in a long process like an AMQP worker with a `FingersCrossedHandler` or `BufferHandler` there is a drawback: as soon as there is an AMQP message that generate a log >= error (for example), all next AMQP messages will output logs, even if theses messages don't generate log where level >= error. In the same context there is a drawback for processor that add an UUID to the logs. The UUID should change for each AMQP messages. --- This patch address this issue with a new interface: `ResettableInterface` interface. Side note: `reset()`, `flush()`, `clear()`, are already used in Monolog. So basically, one can use the `reset()` on the `Logger` and on some `Handler`s / `Processor`s. It's especially useful for * the `FingersCrossedHandler`: it `close()` the buffer, then it `clear()` the buffer. * the `BufferHandler`: it `flush()` the buffer, then it `clear()` the buffer. * the `UidProcessor`: it renew the `uid`.

nicolas-grekas · 2018-07-09T15:16:45Z

What do you think of symfony/symfony#27093 (comment)?

Seldaek · 2018-07-09T15:25:55Z

We have FormattableHandler/ProcessableHandler interfaces already in master so Resettable is kinda in line with that, but maybe should be ResettableHandlerInterface for real consistency.

Seldaek · 2018-11-04T16:43:17Z

Thanks!

Seldaek · 2018-11-04T19:09:10Z

Hey folks so just one more thing before I release this in 1.x.. The more I look at it especially in the master branch for 2.0 where I made close() part of the HandlerInterface, it seems to me like close() and reset() do essentially the same thing.. Or at least they should. Do you also see it like that or not? If so I am thinking to just remove close() in 2.0 and make HandlerInterface implement ResettableInterface. I think it'd simplify things, and it resolves what I had in mind for #197 to add Logger::close() to close all handlers as Logger::reset() was part of this PR.

nicolas-grekas · 2018-11-04T19:57:48Z

If we want to identify a difference, I could see this one: reset is meant to put a service back in a reusable state. This does not necessarily imply closing connections to remote backends. Actually, when using the same kernel in a loop, having connections already open for next requests should be a significant part of why these apps are faster (ie no PDO::__construct() overhead).
Not sure how it applies here, so that's just for thoughts :)

Seldaek · 2018-11-04T20:09:12Z

That makes sense yes, although I don't know how many handlers would actually benefit from this distinction. But then I need to change the implementation a bit, because right now reset() calls close(), so things are even more confusing.

It seems to me like the main use case for reset() is fingers crossed and buffer handlers to make sure they're cleared in between two requests/jobs in long running apps.

The use case for close() is to do some cleanup at the very end, but also right now that's when we flush buffers and stuff like that, so I guess in the buffer case you'd want to flush on reset instead of close. And then close() would become mostly useless, except sometimes it calls reset() to flush, and in a few rare cases it closes some resource. Makes sense? Mostly thinking out loud here..

nicolas-grekas · 2018-11-04T20:13:00Z

Makes sense to me yes :)

sroze · 2018-11-04T21:06:57Z

Makes sense. Close becomes useless as it’s basically end of the process so `reset` is enough :)

…

On Sun, 4 Nov 2018 at 20:13, Nicolas Grekas ***@***.***> wrote: Makes sense to me yes :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#997 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAxHEYosRAIt9vjSa55BjlkIPUBFs0MKks5ur0pSgaJpZM4Nr1RH> .

Seldaek · 2018-11-04T21:43:53Z

OK see e07c948 for changes/clarifications. Happy to get any feedback on this.

Seldaek · 2018-11-04T21:45:04Z

b803523 is the core of the changes for 1.x but I messed up the commit msg :s

Seldaek added this to the 2.0 milestone Jun 19, 2017

nicolas-grekas approved these changes Apr 5, 2018

View reviewed changes

stof suggested changes Apr 5, 2018

View reviewed changes

sroze reviewed Apr 5, 2018

View reviewed changes

lyrixx force-pushed the resetable branch from 146955d to d8f9fd7 Compare April 5, 2018 15:09

lyrixx changed the base branch from master to 1.x April 5, 2018 15:09

lyrixx force-pushed the resetable branch from d8f9fd7 to 1e11f67 Compare April 5, 2018 15:12

sroze approved these changes Apr 5, 2018

View reviewed changes

sroze reviewed Apr 5, 2018

View reviewed changes

lyrixx force-pushed the resetable branch 2 times, most recently from eb81195 to 535b8c5 Compare April 5, 2018 16:18

lyrixx mentioned this pull request Apr 16, 2018

Drop HHVM in .travis.yml #1142

Merged

lyrixx force-pushed the resetable branch from 535b8c5 to da6d3ef Compare June 7, 2018 15:49

Seldaek modified the milestones: 2.0, 1.x Jun 18, 2018

Seldaek added the Feature label Jun 18, 2018

Seldaek mentioned this pull request Jun 18, 2018

Is this project still being actively maintained? #1149

Closed

lyrixx force-pushed the resetable branch from da6d3ef to 9c80bb4 Compare July 6, 2018 08:56

lyrixx changed the title ~~Added a new RestartableInterface and implemented it where possible.~~ Added a new ResetableInterface and implemented it where possible. Jul 6, 2018

lyrixx force-pushed the resetable branch from 9c80bb4 to 74d6959 Compare July 6, 2018 09:08

lyrixx changed the title ~~Added a new ResetableInterface and implemented it where possible.~~ Added a new ResettableInterface and implemented it where possible. Jul 6, 2018

nicolas-grekas reviewed Jul 6, 2018

View reviewed changes

lyrixx force-pushed the resetable branch from 74d6959 to 8d8a094 Compare July 6, 2018 09:17

nicolas-grekas reviewed Jul 6, 2018

View reviewed changes

nicolas-grekas approved these changes Jul 6, 2018

View reviewed changes

lyrixx force-pushed the resetable branch from 8d8a094 to 0625068 Compare July 6, 2018 14:07

lyrixx mentioned this pull request Sep 21, 2018

Flush loggers on kernel.reset symfony/monolog-bundle#279

Merged

Merge branch '1.x' into resetable

531d05a

Seldaek merged commit a03084c into Seldaek:1.x Nov 4, 2018

Seldaek added a commit that referenced this pull request Nov 4, 2018

Merge branch '1.x', clean up close/reset for 2.0, refs #997

e07c948

lyrixx deleted the resetable branch November 5, 2018 08:57

Added a new ResettableInterface and implemented it where possible. #997

Added a new ResettableInterface and implemented it where possible. #997

Conversation

lyrixx commented May 31, 2017 • edited Loading

lyrixx commented Jun 1, 2017

lyrixx commented Apr 5, 2018

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Apr 5, 2018

stof commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyrixx commented Apr 5, 2018

stof commented Apr 5, 2018

sroze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyrixx commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyrixx commented Apr 5, 2018

fabpot commented Apr 6, 2018

lyrixx commented Jun 7, 2018 • edited Loading

nicolas-grekas commented Jun 7, 2018

lyrixx commented Jun 7, 2018

nicolas-grekas commented Jun 7, 2018

Seldaek commented Jun 18, 2018

lyrixx commented Jul 6, 2018

nicolas-grekas commented Jul 6, 2018

lyrixx commented Jul 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyrixx commented Jul 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Jul 9, 2018

Seldaek commented Jul 9, 2018

Seldaek commented Nov 4, 2018

Seldaek commented Nov 4, 2018 • edited Loading

nicolas-grekas commented Nov 4, 2018

Seldaek commented Nov 4, 2018

nicolas-grekas commented Nov 4, 2018

sroze commented Nov 4, 2018 via email

Seldaek commented Nov 4, 2018

Seldaek commented Nov 4, 2018

lyrixx commented May 31, 2017 •

edited

Loading

lyrixx commented Jun 7, 2018 •

edited

Loading

Seldaek commented Nov 4, 2018 •

edited

Loading