Page MenuHomePhabricator

Sensible default parameters for Monolog logging
Closed, ResolvedPublic

Description

There is currently no real default parameters for Monolog logging, and depending on environment it can lead to either no logging at all or log everything with debug level.

  1. When the a logger is set to an empty array [] or NULL, the resulting behaviour behaviour is to log everything to php://stderr with level debug (!). Depending on environement nothing is logged (PHP-FPM with default settings redirects stderr to /dev/null, it can be changed with the parameter catch_workers_output) or everything (Apache’s PHP module). This behaviour is because when a Monolog logger has no handler, it is forced to the stream handler php://stderr with level debug (see Logger::addRecord in Monolog 1.x, it will change with Monolog 2.0 where nothing will happen).
  1. If no @default key is defined, there is a PHP notice Undefined index: @default, it would be better to define one, particularly if Monolog becomes the default logging infrastructure T155552. Consequently of the point 1., in such case, Monolog uses the stream handler php://stderr with level debug.

Propositions:

  1. I propose that when a logger has no handler, the behaviour is to use the handler NullHandler, hence the behaviour will be consistent in all environments, and it is quite expected (I think) that if you don’t define a handler nothing is logged.
  2. When there is no @default key, this logger could be defined at short-term to \Monolog\Handler\StreamHandler( 'php://stderr', \Monolog\Logger::ERROR ) to avoid the level DEBUG in Apache (or any other process where stderr is not /dev/null), and perhaps in the longer-term to a specific handler logging either to $wgDebugLogFile or nowhere (to be discussed).

Event Timeline

I don’t know why my patches are not registered by gerritbot. Here they are:

You need to use Bug: T####, not Issue: ...

You need to use Bug: T####, not Issue: ...

Ah thanks! I guess I mixed with other systems, perhaps Gitlab or GitHub.

Change 439870 had a related patch set uploaded (by Seb35; owner: Seb35):
[mediawiki/core@master] Empty Monolog loggers are now real blackholes

https://gerrit.wikimedia.org/r/439870

Change 439874 had a related patch set uploaded (by Seb35; owner: Seb35):
[mediawiki/core@master] Define a default Monolog logger if inexistant

https://gerrit.wikimedia.org/r/439874

Vvjjkkii renamed this task from Sensible default parameters for Monolog logging to p9aaaaaaaa.Jul 1 2018, 1:04 AM
Vvjjkkii triaged this task as High priority.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed subscribers: gerritbot, Aklapper.
CommunityTechBot renamed this task from p9aaaaaaaa to Sensible default parameters for Monolog logging.Jul 2 2018, 2:53 PM
CommunityTechBot raised the priority of this task from High to Needs Triage.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added subscribers: gerritbot, Aklapper.

Change 439874 merged by jenkins-bot:
[mediawiki/core@master] Define a default Monolog logger if inexistant

https://gerrit.wikimedia.org/r/439874

Change 439870 merged by jenkins-bot:
[mediawiki/core@master] Empty Monolog loggers are now real blackholes and add tests

https://gerrit.wikimedia.org/r/439870

Thanks for the reviews Clara/Bill.

Any thoughts if it's worth backporting this for MW-1.35-release ?

Thanks for the reviews Clara/Bill.

Any thoughts if it's worth backporting this for MW-1.35-release ?

Honestly, I'm not sure. @daniel any thoughts?

@Seb35 OOI, for Monolog 2.0, T242751: Update monolog/monolog to 2.1.1 or later... Should we be mostly reverting this change?

Change 632995 had a related patch set uploaded (by Reedy; owner: Seb35):
[mediawiki/core@REL1_35] Empty Monolog loggers are now real blackholes and add tests

https://gerrit.wikimedia.org/r/632995

@Seb35 OOI, for Monolog 2.0, T242751: Update monolog/monolog to 2.1.1 or later... Should we be mostly reverting this change?

Indeed, this code will no more be useful with Monolog 2.0 and can be reverted there (it is even a micro-optimization).

Change 632995 merged by jenkins-bot:
[mediawiki/core@REL1_35] Empty Monolog loggers are now real blackholes and add tests

https://gerrit.wikimedia.org/r/632995

Change 677040 had a related patch set uploaded (by Seb35; author: Seb35):

[mediawiki/core@master] Do not use a NullLogger when no handler is declared

https://gerrit.wikimedia.org/r/677040

Change 677052 had a related patch set uploaded (by Seb35; author: Seb35):

[mediawiki/core@master] Fix the test MonologSpiTest::testDefaultChannel

https://gerrit.wikimedia.org/r/677052

Change 677052 merged by jenkins-bot:

[mediawiki/core@master] Fix the test MonologSpiTest::testDefaultChannel

https://gerrit.wikimedia.org/r/677052

Change 677405 had a related patch set uploaded (by Reedy; author: Seb35):

[mediawiki/core@REL1_35] Fix the test MonologSpiTest::testDefaultChannel

https://gerrit.wikimedia.org/r/677405

Change 677405 merged by jenkins-bot:

[mediawiki/core@REL1_35] Fix the test MonologSpiTest::testDefaultChannel

https://gerrit.wikimedia.org/r/677405

Change 677040 merged by jenkins-bot:

[mediawiki/core@master] Do not use a NullLogger when no handler is declared

https://gerrit.wikimedia.org/r/677040

Seb35 claimed this task.

Resolved in 1.33.0+ for the part "the @default logger is properly defined as php://stderr level ERROR instead of the previous implicitely defined php://stderr level DEBUG".
Resolved in 1.35.1+ for the part "deactivate a logger with false or null or []".