Skip to content
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

Refactor the logger to a PSR-3 compatible logger #6622

Closed
mnapoli opened this issue Nov 9, 2014 · 7 comments
Closed

Refactor the logger to a PSR-3 compatible logger #6622

mnapoli opened this issue Nov 9, 2014 · 7 comments
Assignees
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.
Milestone

Comments

@mnapoli
Copy link
Contributor

mnapoli commented Nov 9, 2014

Current issues with the logger:

  1. static methods + singleton
  2. global config not overridable by environment (unless I missed how to do that): e.g. in CLI we might want to log to stdout but in HTTP API we don't want to log into the JSON response Change default logger level from WARNING to ERROR #6612
  3. backends are hardcoded and can't be customized (e.g. logging to email, Slack, New Relic, Graylog…)
  4. we can't specify different logging levels for each backend (e.g. log to email on errors only, log to file for anything above INFO)
  5. we don't comply to standards which requires new contributors to learn Piwik logger's specificities, and which means the logger can't be changed because Piwik Core and plugins are coupled to it
  6. we need to maintain our own logger when many other loggers exist, are maintained and have more features

Solutions:

  1. refactor the logger to an object instance (instead of static methods), injectable with dependency injection
  2. configure the logger with DI config, which should have a global config + specific config per environment
  3. use Monolog for example, which comes with so many backend adapters
  4. Monolog supports a huge list of backends
  5. we should use a PSR-3 compatible logger so that we decouple from a specific implementation and its usage is more standard (i.e. simpler for contributors)
  6. this point is covered above (i.e. reuse an existing library like Monolog)

The challenge would be to keep BC, so if we indeed switch to Monolog we would need to implement our own message formatters to support sprintf and also implement our own adapters for the DB logging probably.

I'm not saying we should do it today, but at least the discussion can be opened.

Linked discussions, for reference: #110, #6209, #6612

@mnapoli mnapoli added the Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. label Nov 9, 2014
@mnapoli mnapoli added the c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. label Nov 9, 2014
@tsteur
Copy link
Member

tsteur commented Nov 10, 2014

👍 for using monolog but still keep the API (as mentioned convert to an object instance etc) and not breaking anything

Replacing our logger with monolog is on the second place on my "most wanted" todo list and I am looking forward to seeing it in Piwik

@mattab
Copy link
Member

mattab commented Nov 10, 2014

i'm very excited by possibilities of logging Tracker events to various backends in #6209 which kinda requires this new logger first so big 👍

@tsteur
Copy link
Member

tsteur commented Nov 10, 2014

I can't wait to use the FingersCrossedHandler ;)

@mattab mattab closed this as completed Nov 11, 2014
@mattab mattab reopened this Nov 11, 2014
@mattab
Copy link
Member

mattab commented Nov 11, 2014

I can't wait to use the FingersCrossedHandler ;)

👍 this looks so useful!

@mnapoli mnapoli self-assigned this Nov 28, 2014
mnapoli added a commit that referenced this issue Nov 28, 2014
…screen, database) out of the logger and into the LoggerFactory
mnapoli added a commit that referenced this issue Dec 1, 2014
…sor" objects (to prepare the transition to Monolog)
mnapoli added a commit that referenced this issue Dec 1, 2014
mnapoli added a commit that referenced this issue Dec 1, 2014
mnapoli added a commit that referenced this issue Dec 9, 2014
mnapoli added a commit that referenced this issue Dec 10, 2014
…tainerFactory

This class can be used in tests to create a specific environment (e.g. the prod environment, CLI environment, etc...)
mnapoli added a commit that referenced this issue Dec 10, 2014
…Handler

This is so the FileHandler can be replaced by Monolog's StreamHandler
mnapoli added a commit that referenced this issue Dec 10, 2014
…eamHandler

We can't remove FileHandler completely because it generates a custom exception message in case of error.
mnapoli added a commit that referenced this issue Dec 10, 2014
mnapoli added a commit that referenced this issue Dec 10, 2014
mnapoli added a commit that referenced this issue Dec 10, 2014
mnapoli added a commit that referenced this issue Dec 10, 2014
…a processor + added tests

Monolog handlers support 1 formatter. Using processors instead of nested formatters leads to much simpler code.
mnapoli added a commit that referenced this issue Dec 10, 2014
In the tests, logging is disabled. But not when testing the logger itself…
mnapoli added a commit that referenced this issue Dec 11, 2014
… logging

- the error handler logs warnings and notices, and turns errors into exceptions (`ErrorException`)
- the exception handler catches all uncatched exceptions and display them to the user (HTML or CLI)
- the "screen" logging backend has been removed
- I've normalized exceptions/errors shown to the user in HTML (wether they are catched by the FrontController or not)
mnapoli added a commit that referenced this issue Dec 14, 2014
…L notifications

The "screen" backend (WebNotificationHandler) now logs to HTML notification boxes using the `Notification` api.
I've re-set the default log level to WARN, and logged PHP notices/warnings to "warning" so that on a default install they are shown in the page in a yellow/warning notification box.

For the record, the default log level had previously been changed from WARN to ERROR because screen logging was previously messing the HTML/JSON output (which was breaking archiving). With the logger refactoring this is no longer a problem.
mnapoli added a commit that referenced this issue Dec 14, 2014
Cannot add tests for the screen backend because I would need to mock the session, which is not possible right now because it's all static stuff.
mnapoli added a commit that referenced this issue Dec 17, 2014
- log_only_when_cli
- log_only_when_debug_parameter
mattab pushed a commit that referenced this issue Dec 19, 2014
Refactored the logger to use PSR-3 and Monolog (#6622)
@mnapoli
Copy link
Contributor Author

mnapoli commented Dec 19, 2014

The pull request was merged, what is left for me to do:

@mnapoli mnapoli modified the milestones: Piwik 2.10.0 , Piwik 2.11.0 Dec 19, 2014
@mnapoli
Copy link
Contributor Author

mnapoli commented Dec 21, 2014

\o/

@mnapoli mnapoli closed this as completed Dec 21, 2014
@mattab
Copy link
Member

mattab commented Dec 21, 2014

Beautiful! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.
Projects
None yet
Development

No branches or pull requests

3 participants