[FrameworkBundle][Monolog] Added a new way to follow logs #21080

Open
wants to merge 1 commit into
from
@lyrixx
Member
lyrixx commented Dec 28, 2016 edited
Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

If you want to try this PR, you can use my fork:

git clone https://github.com/lyrixx/symfony-standard -b server-log symfony-se-logs
cd symfony-se-logs
composer install
bin/console server:start
bin/console server:log

and from anywhere curl http://0:8000


Basically, it's a new way to view and filter real time logs, from the CLI.

screenshot13

@lyrixx lyrixx referenced this pull request in symfony/monolog-bundle Dec 28, 2016
Open

Added support for ServerLog command #196

@yceruto
Contributor
yceruto commented Dec 28, 2016 edited

๐Ÿ‘ Thanks you for this awesome feature! --filter <expresion language> ๐Ÿ˜ฒ !!

Just a minor fix in your description to try this PR correctly, because server_log handler is configured into config_dev.yml we need to run curl http://0:8000/app_dev.php/ to see the output ;)

@lyrixx
Member
lyrixx commented Dec 28, 2016

@yceruto By default, bin/console use the dev env. So the server is in dev env by default. That's why I did not add /app_dev.php, because it should not be necessary.

@yceruto
Contributor
yceruto commented Dec 28, 2016 edited

@yceruto By default, bin/console use the dev env. So the server is in dev env by default. That's why I did not add /app_dev.php, because it should not be necessary.

Right, but when you run curl http://127.0.0.1:8000/ we are in prod env right? the handler is not there and the server:log doesn't shows nothing.

I missing something :/ ? at least doesn't works for me :( without /app_dev.php/

+ continue;
+ }
+
+ if ($filter && !$this->el->evaluate($filter, ['record' => $record])) {
@maidmaid
maidmaid Dec 28, 2016 Contributor

What about evaluate directly $record with ->evaluate($filter, $record)? filter option becomes easier to use: from --filter "record['level'] > 200" to --filter "level > 200"

@yceruto
Contributor
yceruto commented Dec 28, 2016

Right, but when you run curl http://127.0.0.1:8000/ we are in prod env right?

Nevermind, I forgot my env var in prod by default, sorry for the noise. This works as it is!

@robfrawley
robfrawley suggested changes Dec 28, 2016 edited View changes

You missed a bunch of short-array syntax instances. ;-) Thanks for this feature, though!

+ {
+ // Retry after 1s
+ if (null === $this->socket) {
+ set_error_handler([$this, 'nullErrorHandler']);
@robfrawley
robfrawley Dec 28, 2016 Contributor

You need to fix your array syntax to long throughout the codebase.

@hhamon
Contributor
hhamon commented Dec 29, 2016

Very nice feature!

@lyrixx
Member
lyrixx commented Dec 29, 2016

You missed a bunch of short-array syntax instances. ;-) Thanks for this feature, though!

Yes, I know ;) I'm just waiting for @symfony/deciders feedbacks before fixing this kind of details.

@nicolas-grekas nicolas-grekas added this to the 3.x milestone Jan 2, 2017
@lyrixx
Member
lyrixx commented Jan 6, 2017

@fabpot Should I move this command to the new server bundle?

+ $this->dumper = new CliDumper();
+ $this->dumper->setOutput($this->output = fopen('php://memory', 'r+b'));
+
+ $this->el = new ExpressionLanguage();
@jakzal
jakzal Jan 10, 2017 Member

This property doesn't exist.

@fabpot
Member
fabpot commented Jan 10, 2017

@lyrixx Indeed, it should be moved to the new web server bundle.

@lyrixx
Member
lyrixx commented Jan 12, 2017 edited

I moved the command to the WebServerBundle.

This PR is no "Ready To Review"

@xabbuh
Member
xabbuh commented Jan 15, 2017

The ServerLogCommand on one side and the VarDumperFormatter and ServerLogHandler on the other side are not wired. Is that intended? Wouldn't it be better to split this in two PRs then?

@lyrixx
Member
lyrixx commented Jan 18, 2017

@xabbuh You are right, they are not wired. You might want to see my fork to see how it is wired.
IMHO, it's should not be split in 2 PRs because all theses classes work together.

@lyrixx lyrixx closed this Jan 18, 2017
@lyrixx lyrixx deleted the lyrixx:server-log branch Jan 18, 2017
@lyrixx lyrixx restored the lyrixx:server-log branch Jan 18, 2017
@xabbuh
Member
xabbuh commented Jan 18, 2017

@lyrixx Why did you close here?

@lyrixx lyrixx reopened this Jan 18, 2017
@lyrixx
Member
lyrixx commented Jan 18, 2017

Oups ... I don't know. Sorry for the noise.

@lyrixx
Member
lyrixx commented Feb 3, 2017

@fabpot It looks like you are not totally convinced by this PR. I could add "experimental" (cf http://symfony.com/blog/experimental-features) if you want.

@nicolas-grekas
Member
nicolas-grekas commented Feb 15, 2017 edited

I like this :) Here are some suggestions:
master...nicolas-grekas:server-log

This patch fixes a 1s pause per log line I experienced when no log server was listening.
It uses persistent and asynchronous connections to make things faster.

@lyrixx
Member
lyrixx commented Feb 16, 2017

This patch fixes a 1s pause per log line I experienced when no log server was listening.

I'm unable to reproduce it. Do you have a reproducer ?

+ $logId = null;
+ foreach ($keys as $key) {
+ if (isset($record['extra'][$key])) {
+ $logId = $record['extra'][$key];
@stof
stof Feb 16, 2017 Member

you should break when you find the log id

@lyrixx
lyrixx Feb 16, 2017 Member

Fixed.

@lyrixx
Member
lyrixx commented Feb 16, 2017

@nicolas-grekas I cherry-picked your commit and added a new one.

+use Monolog\Logger;
+use Symfony\Bridge\Monolog\Formatter\VarDumperFormatter;
+
+class ServerLogHandler extends AbstractHandler
@yceruto
yceruto Feb 16, 2017 edited Contributor

missing author

@@ -0,0 +1,102 @@
+<?php
+
+namespace Symfony\Bridge\Monolog\Handler;
@yceruto
yceruto Feb 16, 2017 Contributor

missing license

@lyrixx
lyrixx Feb 16, 2017 Member

Thanks. Fixed (both comments)

@lyrixx lyrixx [FrameworkBundle][Monolog] Added a new way to follow logs
4c313bd
+ ->addOption('date-format', null, InputOption::VALUE_REQUIRED, 'The date format', 'H:i:s')
+ ->addOption('format', null, InputOption::VALUE_REQUIRED, 'The line format', '%s <%s>%-9s</> <options=bold>%-8.8s</> %s')
+ ->addOption('show-context', null, InputOption::VALUE_NONE, 'Display the context')
+ ->addOption('show-extra', null, InputOption::VALUE_NONE, 'Display the extra')
@jderusse
jderusse Feb 17, 2017 Contributor

I think an option for minimal level (or using the verbosity to quickly restrict levels) could be simpler to use than the expression language

@lyrixx
lyrixx Feb 17, 2017 Member

It does not use the expression language for that. I don't understand

+ ->addOption('format', null, InputOption::VALUE_REQUIRED, 'The line format', '%s <%s>%-9s</> <options=bold>%-8.8s</> %s')
+ ->addOption('show-context', null, InputOption::VALUE_NONE, 'Display the context')
+ ->addOption('show-extra', null, InputOption::VALUE_NONE, 'Display the extra')
+ ->addOption('filter', null, InputOption::VALUE_REQUIRED, 'An expression to filter log. Example: record[\'level\'] > 200 or record[\'channel\'] in [\'app\', \'doctrine\']"')
@jderusse
jderusse Feb 17, 2017 Contributor

Exemple don't work bin/console server:log --filter "record['level'] > 200" =>

Variable "record" is not valid around position 1. 

Did I missed something?

@lyrixx
lyrixx Feb 17, 2017 Member

I guess we broke something. Thanks.

@lyrixx
lyrixx Feb 17, 2017 Member

Actually, it's only bin/console server:log --filter "level > 200" ;) It's simpler.

@stof
stof Feb 17, 2017 Member

yeah, a review comment suggested to simplify it, but the description of the option was not updated when implementing the requested change

+ $this
+ ->setName('server:log')
+ ->setDescription('Start a log server that displays logs in real time')
+ ->addOption('host', null, InputOption::VALUE_REQUIRED, 'The server host', '0:9911')
@stof
stof Feb 17, 2017 Member

why this default port ?

@lyrixx
lyrixx Feb 17, 2017 Member

I really don't know.It was easy to type. What do you suggest ?

+ ->addOption('host', null, InputOption::VALUE_REQUIRED, 'The server host', '0:9911')
+ ->addOption('date-format', null, InputOption::VALUE_REQUIRED, 'The date format', 'H:i:s')
+ ->addOption('format', null, InputOption::VALUE_REQUIRED, 'The line format', '%s <%s>%-9s</> <options=bold>%-8.8s</> %s')
+ ->addOption('show-context', null, InputOption::VALUE_NONE, 'Display the context')
@stof
stof Feb 17, 2017 Member

should we really have options for that ? Couldn't it simply be based on the verbosity instead ? This seems like a perfect use case for it: display more details in verbose mode.

@lyrixx
lyrixx Feb 17, 2017 Member

Indeed I will remove it.

Actually I'm working on a much better default log display in command.

+ $this->dumper = new CliDumper();
+ $this->dumper->setOutput($this->output = fopen('php://memory', 'r+b'));
+
+ if ($input->getOption('filter')) {
@stof
stof Feb 17, 2017 Member

this is wrong. initialize is called before the input validation is completed. so you should not start reading it here IMO

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