Allow Log integration with Monolog

[brendo]

Our Symphony Log is limited to file rotation logging and doesn't follow any particular standard. Integration with the ever popular and extensible Monolog will allow greater freedom and flexibility to log information and errors.

By default we could use the StreamHandler or RotatingFileHandler to mirror what we get at the moment.

[nitriques]

+1 Building a UI for display in the backend would be nice too (would render this obsolete: https://github.com/DeuxHuitHuit/system_log_link)

[brendo]

So this is something I've been wanting to do for a long time. The core Logger has been ripped out and replaced by Monolog. Monolog is a PSR-3 compatible logging system and out of the box offers integration with many third party logging systems.

One of the more interesting things about this change, is that the Handler/Formatter used is totally derived from the config file. This Log configuration also supports placeholders within the context of the entire configuration file, or within Symphony:: initialiseLog. It's a little rough around the edges, but I wanted to prove the concept can work and thought a first effort would allow more detailed discussion to take place.

This change is likely to not be backwards compatible as the API of the Log component has changed.

@nitriques @pointybeard

[nitriques]

@brendo this is nice. But:

1. Did we lose features (I.e log rotation?)
2. Can't we create a compatibility layer to make it backward compat ? Or it is impossible ?
3. Could it be possible to split the commit ? github has a hard time showing the diff...

Thanks.

[brendo]

Did we lose features (I.e log rotation?)

In it's exact replica, yes, we no longer have log rotation after 'x' size. We do have access to Monolog's RotatingFileHandler however:

RotatingFileHandler: Logs records to a file and creates one logfile per day. It will also delete files older than $maxFiles. You should use logrotate for high profile setups though, this is just meant as a quick and dirty solution.

Can't we create a compatibility layer to make it backward compat ? Or it is impossible ?

Seems unnecessary as several of the removed methods no longer apply. They mean different thing depending on what Handler is used (or not used). I have maintained the pushToLog and pushExceptionToLog methods as these are the most commonly used methods. All others were only used internally to configure the Logger.

Could it be possible to split the commit ? github has a hard time showing the diff...

Not really, there's files added because we don't yet have an installer capable of pulling in composer dependencies. Fortunately you only need to inspect the first couple of changes, everything in vendor is just the Monolog library :)

[nitriques]

Ok thanks a lot Brendan. So we do are backward compatible then?

[jonmifsud]

Seems not the function signatures have changed, so I think it will have to be for 3.0 right?

Pretty keen on trying this out as I want to try out something like logentries or logstash, to hopefully make more sense out of error logs.

[brendo]

Seems not the function signatures have changed, so I think it will have to be for 3.0 right?

Correct. However you are welcome to try this on an existing install as the methods that have removed are not typically used by extensions at all:

• Log->setArchive
• Log->setMaxSize
• Log->setDateTimeFormat
• Log->setLogPath / Log->getLogPath
• Log->popFromLog
• Log->writeToLog
• Log->open
• Log->close
• Log->initialise

Of those, the one that is most likely to be used by your extension is Log->writeToLog, which is easily replaced with Log->pushToLog. Log->pushToLog signature has changed, but I don't honestly believe anyone would be relying on the functionality of those extra parameters.

The Log class will expose any of the internal log methods with the magic __call method, so if there is anything you wanted to access on the Logentries or Logstash handlers you could do so.

Please try it out, I'll be happy to work with you to get it going as I believe this is an awesome change for Symphony and gives a lot of freedom to developers!

It also introduces a cool new method, General:: replacePlaceholdersWithContext which offers a dead easy way to do placeholder replacement in strings.

[jonmifsud]

I agree that it's an awesome change. I'll try setup a local ELK server, and might re-add writeToLog if I have issues (just for backward compatibility) as there's plenty of extensions I'd need to check on the site I'd like to use try it out on.

[jonmifsud]

@brendo did you have any other changes live in addition to the commits on that pr? The configuration class within the core is unable to handle arrays when re-saving any configuration. I used to get around it by storing it in json format for extensions which I had made but saving nested arrays properly in the config would be neat.

We might also need to 'rethink' how the update functionality will work, as we want the config changes to happen first, so that logging would work. It would also need some migration info in case someone rolls back that the log configuration is changing, and needs to be edited manually. (remember re-saving the config with nested array with current versions will crash)

Otherwise once I placed the right config in it seems to work creating the exact log format as before.

I've only found one instance of writeToLog() - in XML Importer from the extensions I'm using on this website. Which is pretty good. I'll be trying to use another log handler to hook it up with a local ELK setup. Will keep you posted on how that goes.

[brendo]

The PR is based off integration from memory which does handle nested arrays from memory On 14 Nov 2015 6:57 PM, "Jonathan Mifsud" [email protected] wrote:

@brendo https://github.com/brendo did you have any other changes live in addition to the commits on that pr? The configuration class within the core is unable to handle arrays when re-saving any configuration. I used to get around it by storing it in json format for extensions which I had made but saving nested arrays properly in the config would be neat.

We might also need to 'rethink' how the update functionality will work, as we want the config changes to happen first, so that logging would work. It would also need some migration info in case someone rolls back that the log configuration is changing, and needs to be edited manually. (remember re-saving the config with nested array with current versions will crash)

Otherwise once I placed the right config in it seems to work creating the exact log format as before.

I've only found one instance of writeToLog() - in XML Importer from the extensions I'm using on this website. Which is pretty good. I'll be trying to use another log handler to hook it up with a local ELK setup. Will keep you posted on how that goes.

— Reply to this email directly or view it on GitHub https://github.com/symphonycms/symphony-2/issues/2223#issuecomment-156672313 .

[jonmifsud]

Hmm when I merged in the monolog changes the config still wouldn't save, checked the code on monolog and integration, they're consistent. Reading the config is fine it's saving which is a problem. Maybe you've worked on it somewhere else? I'll be happy to take care of this otherwise.

I've also got this setup with ELK, the logstash formatter is really neat gets in data the way it can be easily analysed. However it made me notice one thing, all exceptions thrown at this point are identical. If I switch off my database connection, I get two entries in the log, the first from the Generic Error handler, which does not contain any exception information, and the second throws an exception of SymphonyErrorPage which is not the original exception source. I'd rather we see DatabaseException somewhere within the logs, as all exceptions (at least the ones I could generate easily) always throw a SymphonyErrorPageexception, I assume that is because we catch pretty much everything else to show the 500 error page.

I've poked around a bit and noticed that we pass an 'additional' array (converted to object) which contains the original error details thus I'd like to suggest that we should also change the exception pushed to logs to

self::$_Log->pushExceptionToLog($e->getAdditional()->error ? $e->getAdditional()->error : $e, true);

this should make sure that we pass the original error codes to the logs rather than the generic exception. If one is available.

Edit: I noticed that not all throwCustomError functions pass a related exception, in particular XSLT errors do not have a specific exception, we should consider adding the key ones for logging/exception consistency. Attached screenshot of ELK error counts by exception class to give an idea of what I'm trying to achieve.

[brendo]

I've pushed a couple of commits which fixes those bugs you've found. The last one is a bit trickier, as the XsltProcess class doesn't raise any exceptions.

And yep, saving the config will be a problem, I never tested that in the end! I noticed @nitriques has opened #2519 recently which can address this.

[brendo]

Also, I made a mistake before, I thought I had this work based off integration, but it's actually based off master. I think I was hoping it could be dropped in before integration is a thing. Technically possible by the looks, the updater will have to convert from the old configuration to the new configuration.

That said, we should really just release integration when we can. The changes on integration at the moment are pretty neat and will make things easier for people.

[jonmifsud]

@brendo thanks for getting onto those bugs quickly. I'll see where @nitriques is on that, and see if I can help out. Thanks a bunch for this it's pretty neat, I've had it merged with no issues really, and the site's prod configs are read-only it seems very much production ready. I'll be trying to get client to approve an ELK stack on a separate server, where I can try it out on staging/demo servers prior to moving it to production, so I can give further feedback if necessary.

[nitriques]

@brendo @jonmifsud I'll concentrate to release 2.6.4 before working on anything integration related.

@brendo would you be open to rebase your work when 2.6.4 is shipped ?

[brendo]

Of course

On Fri, Nov 20, 2015 at 11:16 AM, Nicolas Brassard <[email protected]

wrote:

@brendo https://github.com/brendo @jonmifsud https://github.com/jonmifsud I'll concentrate to release 2.6.4 before working on anything integration related.

@brendo https://github.com/brendo would you be open to rebase your work when 2.6.4 is shipped ?

— Reply to this email directly or view it on GitHub https://github.com/symphonycms/symphony-2/issues/2223#issuecomment-158249343 .

[brendo]

Do you want me to pick this up again and rebase against integration? integration has 3.0.0 work right?

Probably worth it's own ticket, but should 3.0 enforce developers to use composer install? It means we can remove the /vendor directory from the project?

[nitriques]

integration has 3.0.0 work right?

Yes! I've rebased integration on top of 2.6.7, a.k.a master at the moment.

Probably worth it's own ticket, but should 3.0 enforce developers to use composer install? It means we can remove the /vendor directory from the project?

Yes. I'd like to create 2.7.0 before as a LTS version. And it also means that symphony itself should be installable with composer. And extensions... Or maybe this will constitute version 4.

Spoiler: It will be called Symphony CMS Beethoven Edition :P