Problem/Motivation

We have an opportunity in Drupal 8 to much improve our logging system.

Proposed resolution

Monolog is an abstracted logging service which integrates with many different handlers. In this forked issue from #1289536: Switch Watchdog to a PSR-3 logging framework, we will simply add Monolog to Drupal 8's code base.

Remaining tasks

See #1289536: Switch Watchdog to a PSR-3 logging framework

User interface changes

None.

API changes

Monolog's code is added to Drupal 8. See #1289536: Switch Watchdog to a PSR-3 logging framework for more information.

Original report by catch

hook_watchdog() is a good candidate for moving out of the hook system into pluggable (and in this case stackable) classes - we want it to be available as early as possible, however currently it's not available until bootstrap modules are loaded - too late if you want to log in a cache backend or the lock system for any reason.

However hook_watchdog() is a small part of what our existing logging modules do. dblog has settings for expiring log items and an admin page for viewing/filtering log entries. Syslog has an admin screen, mongodb in contrib has an improved version of the dblog UI. So those would likely stay more or less the same - just the actual logging backend called from watchdog() would be different.

If we moved logging to OOP, I'd want to keep the same basic feature of being able to have more than one logger enabled at a time - it's handy to be able to log different messages to different storage etc.

In #1263478: Identify pieces of Drupal that could be replaced by Symfony code https://github.com/Seldaek/monolog was brought up.

I had a very quick glance at the Monolog code mainly to see if it supports more than one handler (which it does). That would be a showstopper if not, have not looked beyond this though yet.

Files: 
CommentFileSizeAuthor
#13 1810664.patch264.66 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1810664_2.patch. Unable to apply patch. See the log in the details link for more information. View
#10 1810664.patch358.1 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1810664_1.patch. Unable to apply patch. See the log in the details link for more information. View
#6 1810664.patch253.23 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1810664_0.patch. Unable to apply patch. See the log in the details link for more information. View
#3 1810664.patch252.49 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1810664.patch. Unable to apply patch. See the log in the details link for more information. View
1289536-monolog.patch231.62 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] 42,170 pass(es), 1 fail(s), and 0 exception(s). View

Comments

Status: Needs review » Needs work

The last submitted patch, 1289536-monolog.patch, failed testing.

RobLoach’s picture

Title: Add Monolog to Drupal » Add Monolog Symfony-Bridge to Drupal

We should bring in Symfony's MonologBridge instead so that we have compatibility with Symfony's LoggerInterface (from HttpKernel).

RobLoach’s picture

Status: Needs work » Needs review
FileSize
252.49 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1810664.patch. Unable to apply patch. See the log in the details link for more information. View
+++ b/core/composer.json
@@ -8,6 +8,7 @@
     "symfony/http-kernel": "2.1.*",
+    "symfony/monolog-bridge": "2.1.1",
     "symfony/routing": "2.1.*",

We have to target MonologBridge 2.1.1 explicitly until we update Symfony itself. In the mean time, this patch should do it.

RobLoach’s picture

#3: 1810664.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1810664.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
253.23 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1810664_0.patch. Unable to apply patch. See the log in the details link for more information. View

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, 1810664.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review

#6: 1810664.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1810664.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
358.1 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1810664_1.patch. Unable to apply patch. See the log in the details link for more information. View
RobLoach’s picture

#10: 1810664.patch queued for re-testing.

RobLoach’s picture

Status: Needs review » Needs work

core/vendor/symfony/serializer/Symfony/Component/Serializer/symfony-Serializer-f75d06c

There was a file conflict when introducing Symfony Serializer.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
264.66 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1810664_2.patch. Unable to apply patch. See the log in the details link for more information. View
Crell’s picture

Status: Needs review » Reviewed & tested by the community

This is just an "Add library so patches work" issue, which we've generally commited with a "we can easily pull it out if we end up not using it" caveat. So, let's do that and unblock #1289536: Switch Watchdog to a PSR-3 logging framework

fgm’s picture

Status: Reviewed & tested by the community » Needs work

I'm really not convinced by this route: watchdog() previously, and Monolog on its own, expose a much richer interface than the Symfony LoggerInterface, which is basically just made of one method set: <level>(<message>, <context>)

In comparison, watchdog() exposes a richer interface: watchdog(<channel>, <message template>, <message arguments>, <level>, <context>. It has just one method, but more cleanly identified parameters, which lend themselves cleanly to event i18n and aggregation in reports (think of the current mongodb_watchdog UI, or tools like kibana: much easier to use if logged content is structured).

Of course, we can serialize the whole set of structured information in $context just to shoehorn it to the Symfony LoggerInterface, then add a Monolog Processor in the Logger-Handler chain, just to restore the structured data to that native rich handlers can store structured information instead of having just unstructured data to log, but that seems a pointless waste of resources. I think it would make better sense to use a native integration between Drupal and the Monolog: its main entry point, Logger::addRecord() already includes the level parameter, and the channel separation can be replicated efficiently by instantiating multiple Loggers with separate configurations: according to Monolog docs, a Logger is just that, a channel.

We could then expose the more limited LoggerInterface on top of the default logger, for the sake of Symfony code requiring that interface.

Resetting to CNW for these reasons.

Regarding the patch itself the ClassLoader::findFile() changes appear to be due to composer code generation, but:

  • they do not fix the fact that passing findFile('') or findFile('\\') will throw an warning instead of just failing to load the class. It's a stupid case, but it should not cause a php notice anyway
  • are inconsistent between code and phpdoc: the phpdoc says the function returns path|null, but it returns path|false if passed '' or '\\' or a non loadable class name. Since the is no case return null, the one to chane is the phpdoc
fgm’s picture

FWIW, the error mentioned above about the classloader is not present in the original code of the Symfony UniversalClassLoader, but a consequence of the loader changes in Composer. I'll pursue this on composer, not here.

RobLoach’s picture

Let's get #1447736: Adopt Guzzle library to replace drupal_http_request() in first. Adding tags.

watchdog() exposes a richer interface: watchdog(, , , , . It has just one method, but more cleanly identified parameters, which lend themselves cleanly to event i18n and aggregation in reports

Any extraneous meta-data is to be passed through to <context>. You mentioned that this seems kind of weird, but in reality, that's how most logging systems work. Context is simply an array of meta-data. It's up to DblogHandler how context is handled, so we have the power deal with $context however we want.

FWIW, the error mentioned above about the classloader is not present in the original code of the Symfony UniversalClassLoader, but a consequence of the loader changes in Composer. I'll pursue this on composer, not here.

Likely an update from Composer. If we get #1447736: Adopt Guzzle library to replace drupal_http_request() in first, then that change will likely go away.

We could then expose the more limited LoggerInterface on top of the default logger, for the sake of Symfony code requiring that interface.

The great thing about Monolog is that this is already written for us. If we were to write our own implementation of it, than that's just adding more code for us to maintain ourselves. Depending on an external library to make those connections means we're working directly with an already proven code set and community.

fgm’s picture

Status: Needs work » Reviewed & tested by the community

OK, I reviewed it in more detail, and I withhold my earlier objections.

The composer issue is being addressed within composer.

FWIW, I've continued my series on other logging solutions on my blog (log4j, kohana...), to explain why we're bringing in Monolog rather than another solution.

catch’s picture

Status: Reviewed & tested by the community » Needs work

No if we're going to do this at all, let's use the Monolog interface which has feature parity with watchdog() currently. The fact it didn't is the reason I originally rejected it a year or more ago.

There are some places in core that Symfony's logger interface might need a bridge so that Symfony code can actually log, but that's no reason to hamstrung everywhere else.

RobLoach’s picture

Status: Needs work » Postponed

Let's postpone this issue and apply the Symfony-Bridge to it after #1289536: Switch Watchdog to a PSR-3 logging framework then.

Ice-D’s picture

Status: Postponed » Needs review
Issue tags: -WSCCI, -Proudly Found Elsewhere

#10: 1810664.patch queued for re-testing.

Chi’s picture

13: 1810664.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 13: 1810664.patch, failed testing.

Chi’s picture

Issue summary: View changes
Status: Needs work » Postponed
Related issues: +#1289536: Switch Watchdog to a PSR-3 logging framework

Postponed in accordance with #20.

mgifford’s picture

Status: Postponed » Needs work

unblocked.

Crell’s picture

Assigned: RobLoach » Unassigned

I think we can won't-fix this now, no? We have a home-grown PSR-3 compatible logging system now. Some parts of it at least could be swapped out with any PSR-3 library. I don't think it's still in scope to rip it back out in favor of Monolog at this point, much as I'd like to have less home-grown code.

fgm’s picture

One thing we should make sure of is that we can actually swap the logger as wished, probably like we swap the database for the mongodb version using service tags.

mgifford’s picture

I was just cleaning up the Postponed tag on D8. Lots of stuff got forgotten there. I'm fine with closing this, but wanted to get it out of limbo.

catch’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue tags: +minor version target

Crell asked me to look at this figure out whether it's won't fix or not. If the PSR-3 logger is implemented properly, then we can add monolog from contrib. If it's not that's a bug but would be a separate bug report to this issue.

Adding the library and refactoring core to use it could be done in a way which doesn't break bc or cause much disruption (although would need to figure out what to do with contrib logging modules in use on sites etc. which starts to get tricky).

If that's correct and we thought it was worth doing (not sure on that bit), then it'd be possible to do this in a minor version. See #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase? for the long version of that answer.

Going to tag minor version target and bump to 8.1.x for now.

ParisLiakos’s picture

Related issues: +#2351073: Port Monolog to D8
Crell’s picture

The services that are using a PSR-3 logging interface would port easily. However, some are using the logger factory directly instead. Those would need to be modified as that's a Drupal-specific interface.

ParisLiakos’s picture

Well, the logger factory should be swapped out anyway, to return Monolog\Logger objects instead of Drupal\Core\LoggerChannel objects, so that shouldnt be a problem at all. Where i see the problem is:
The mess with log levels.

  • PSR3 uses strings
  • Drupal core uses RFC 5424 ints
  • Monolog uses ints but 100-600

#2267545: Standardize to RFC 5424 log levels

Though, Monolog plans to also switch to RFC in 2.0, which will make things easier..I would say, that would be a good point to pull Monolog in since it would require less changes

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dagmar’s picture

Component: watchdog.module » dblog.module

Moved to dblog.module for future triage.