[POC/RFC] Simplify Client logger #1069
Conversation
merk
changed the title
|
Generally +1 on removing logging functionality form Elastica as this is not the responsibility of Elastica and with Psr/Log now a reasonable standard exist. As I don't really have experience with Psr logger two questions:
It is important for me to keep starting a project with Elastica stays as simple as creating a new Elastica\Client with empty params for localhost (if possible). |
|
|
I've modified the ->debug call to provide additional context information. Most LoggerInterface implementations will provide a way to format additional context information in whatever way the user desires. For example, the primary logger implementation used by Symfony provides formatters and processors to allow customisation of the log messages as they're written: https://github.com/Seldaek/monolog/blob/master/doc/02-handlers-formatters-processors.md Alternatively, per my second point we could provide something like what FOSElasticaBundle provides: https://github.com/FriendsOfSymfony/FOSElasticaBundle/blob/master/Logger/ElasticaLogger.php (note that ours does more to provide debugging information in the Symfony profiler.) |
|
My initial attempt at adding more context wouldn't work. I've had another go. We now log a successful request/response, or a request/exception pair. I've also removed the null condition on the $_logger - it is always a LoggerInterface. If one is not provided, it is an instance of NullLogger. |
| @@ -640,6 +647,14 @@ public function request($path, $method = Request::GET, $data = array(), array $q | |||
|
|
|||
| return $this->request($path, $method, $data, $query); | |||
| } | |||
|
|
|||
| $this->_logger->debug('Elastica Request', [ | |||
There was a problem hiding this comment.
Just a thought: I'm not sure how the implementation of Elastica\Log will handle the context array if we have any unserializable object as part of the Request (from what it looks like, the Response would die before we get here if it had anything unserializable).
There was a problem hiding this comment.
In which cases would that happen? Not sure I can follow.
There was a problem hiding this comment.
If a document had a reference to something that couldn't be serialized it could become a problem - except I suspect Elastica itself wouldn't work in that circumstance anyway.
There was a problem hiding this comment.
Yeah, that would probably break quite a few things.
|
The build failure looks like its unrelated to the PR. |
|
I'm ready for this to be merged if the change is agreeable. I believe I've covered most use cases and avoided any BC breaks. |
|
LGTM. Can you rebase this on top of master again? |
| * | ||
| * @throws Exception\RuntimeException | ||
| */ | ||
| protected function _log($context) |
There was a problem hiding this comment.
I'm worried here - this method could be overwritten by people extending \Elastica\Client. Its a BC break.
We should probably still do the logging in this method but deprecate it.
There was a problem hiding this comment.
I've added _log back with similar behaviour above. It's still not perfect: without adding a second parameter (which will cause php errors if someone has overwritten _log) we cant get the Response for logging details about the response.
I think its an okay trade-off, its simple to fix by adding the second parameter to the overwritten _log method.
There was a problem hiding this comment.
Never mind, we store it in Client::_lastResponse.
Current coverage is 84.42%@@ master #1069 diff @@
=====================================
Files 179 179
Lines 4756 4750 -6
Methods 1188 1186 -2
Branches 0 0
=====================================
+ Hits 4006 4010 +4
+ Misses 750 740 -10
Partials 0 0
|
merk
force-pushed
the
remove-logger
branch
2 times, most recently
from
16c8bf4
to
9f6ce9a
Compare
|
Rebased |
|
Merged. Thanks. |
Not for merging (yet).
While working elsewhere, I've run into a few things that could be cleaned up and simplified. I've chosen 2 things I've noticed to send initial POC PRs to start a discussion about potential directions.
In this case, there is no point in having complicated configurations for logging in Elastica - lets move the heavy lifting of logging to the PSR logger and just pass one in at Client construction.
This could be implemented differently again: if we end up with a ClientInterface, we could wrap the Client with a LoggingClient decorator instead. I realise this may lead to more boilerplate for simple projects but there are solutions to talk about for that if the direction is a good one for this project.