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

Add support for Raven, the protocol used by Sentry (http://github.com/dcramer/sentry) #76

Merged
merged 17 commits into from
Jan 7, 2013
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
acbf574
Add RavenHandler and RavenFormatter for working with Raven which is the
msabramo Apr 25, 2012
8aeb75a
composer.json: Tighten requirement "raven/raven": ">=0.2.0" to
msabramo Apr 26, 2012
ac161a0
Remove leading \ in use statement, as suggested by @Seldaek in
msabramo Apr 26, 2012
0e579f1
Map CRITICAL and ALERT to ERROR because raven doesn't have more than
msabramo Apr 26, 2012
f158009
Minor reformatting suggested by @Seldaek in
msabramo Apr 26, 2012
6d15811
Put braces on same line for if statements, as requested by @Seldaek in
msabramo Apr 26, 2012
f6e75e4
Put args one per line, as suggested by @Seldaek in
msabramo Apr 26, 2012
3238a74
Rework how exceptions are stored and accessed using suggestions from
msabramo Apr 26, 2012
1c59696
Change check of record level to >= Logger::ERROR instead of >
msabramo Apr 26, 2012
7a81acd
Eliminate local variables used as pseudo-named-arguments, as suggested
msabramo Apr 26, 2012
6b4b2a6
Change key from 'context' to 'exception' in
msabramo Apr 26, 2012
44d2441
Pull MockRavenClient (which depends on Raven_Client) out of
msabramo Apr 26, 2012
4830b92
Curly braces on same line for try/catch, as suggested by @stof in
msabramo Apr 27, 2012
5d17212
Address a few of the concerns from @stof in #76.
msabramo Oct 25, 2012
368b1f0
Remove RavenFormatter; this is not needed since Raven is a very simple
msabramo Oct 25, 2012
c2850f1
Remove `close` method which was setting `$this->ravenClient = null`
msabramo Oct 25, 2012
10fcd61
Add note about RavenHandler to README.mdown
msabramo Oct 30, 2012
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,11 @@
"php": ">=5.3.0"
},
"require-dev": {
"raven/raven": "0.2.*",
"mlehner/gelf-php": "1.0.*"
},
"suggest": {
"raven/raven": "Allow sending log messages to a Sentry server",
"mlehner/gelf-php": "Allow sending log messages to a GrayLog2 server"
},
"autoload": {
Expand Down
48 changes: 48 additions & 0 deletions src/Monolog/Formatter/RavenFormatter.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
<?php

/*
* This file is part of the Monolog package.
*
* (c) Jordi Boggiano <j.boggiano@seld.be>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Monolog\Formatter;

use Monolog\Logger;
use Raven_Client;

/**
* Serializes a log message for Raven (https://github.com/getsentry/raven-php)
*
* @author Marc Abramowitz <marc@marc-abramowitz.com>
*/
class RavenFormatter extends NormalizerFormatter
{
/**
* Translates Monolog log levels to Raven log levels.
*/
private $logLevels = array(
Logger::DEBUG => Raven_Client::DEBUG,
Logger::INFO => Raven_Client::INFO,
Logger::WARNING => Raven_Client::WARNING,
Logger::ERROR => Raven_Client::ERROR,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should map CRITICAL and ALERT to ERROR as well if raven doesn't have more than ERROR, because otherwise those messages will fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See 0e579f1

Logger::CRITICAL => Raven_Client::ERROR,
Logger::ALERT => Raven_Client::ERROR,
);

/**
* {@inheritdoc}
*/
public function format(array $record)
{
$record = parent::format($record);

$record['level'] = $this->logLevels[$record['level']];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of changing the level here, I would move the level map to the handler to be consistent with other handlers

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and the formatter should return only the formatted message instead of returning a full record with a different message.

$record['message'] = $record['channel'] . ': ' . $record['message'];

return $record;
}
}
76 changes: 76 additions & 0 deletions src/Monolog/Handler/RavenHandler.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
<?php

/*
* This file is part of the Monolog package.
*
* (c) Jordi Boggiano <j.boggiano@seld.be>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Monolog\Handler;

use Monolog\Logger;
use Monolog\Handler\AbstractProcessingHandler;
use Monolog\Formatter\RavenFormatter;
use \Raven_Client;

/**
* Handler to send messages to a Sentry (https://github.com/dcramer/sentry) server
* using raven-php (https://github.com/getsentry/raven-php)
*
* @author Marc Abramowitz <marc@marc-abramowitz.com>
*/
class RavenHandler extends AbstractProcessingHandler
{
/**
* @var Raven_Client the client object that sends the message to the server
*/
protected $ravenClient;

/**
* @param Raven_Client $ravenClient
* @param integer $level The minimum logging level at which this handler will be triggered
* @param Boolean $bubble Whether the messages that are handled can bubble up the stack or not
*/
public function __construct(Raven_Client $ravenClient, $level = Logger::DEBUG, $bubble = true)
{
parent::__construct($level, $bubble);

$this->ravenClient = $ravenClient;
}

/**
* {@inheritdoc}
*/
public function close()
{
$this->ravenClient = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the dependency seems weird to me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which dependency? I meant to remove RavenFormatter. RavenHandler still depends on RavenClient (in line 16). Or maybe you wanted me to keep a defintion of getDefaultFormatter?

Let me know what you meant and I'll try to turn it around quick.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are removing the dependency of the handler (the raven client) on close, which seems weird to me. This means that the handler is in a totally broken state once close (calling a method on it would trigger a fatal error). And removing the reference is not needed. PHP is able to garbage collect objects.

}

/**
* {@inheritdoc}
*/
protected function write(array $record)
{
if ($record['level'] >= Logger::ERROR && isset($record['context']['exception'])) {
$this->ravenClient->captureException($record['context']['exception']);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While having a convention about the way to provide exception in the context to allow a special handling is fine, I'm concerned with this implementation: it will log only the exception and drop the log message (which could be better to understand the issue as the dev could have explained why he catched the exception in this case and logged it).

} else {
$this->ravenClient->captureMessage(
$record['formatted']['message'],
$record, // $params
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will duplicate many things (as $record['formatted'] contains the normalized record). It should probably be limited to $record['formatted'].

Thus, looking at the raven client, it seems wrong. the params array is meant to contain params to fill placeholders of the message through sprintf. This is not what the record is about. With your current implementation, the message sent to Raven will not contain any data of the context or extra array (and it will store its own timestamp instead of the timestamp provided by Monolog)

$record['formatted']['level'], // $level
true // $stack
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think forcing to use the stacktrace is a good idea. It should be configurable in the constructor

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that for messages it's probably not needed in most cases, for exceptions though it's great. Maybe just capture if level is above ERROR?

);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this capture exceptions twice? Once as a regular message and once as an exception.

}

/**
* {@inheritDoc}
*/
protected function getDefaultFormatter()
{
return new RavenFormatter();
}
}
26 changes: 26 additions & 0 deletions tests/Monolog/Handler/MockRavenClient.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?php

/*
* This file is part of the Monolog package.
*
* (c) Jordi Boggiano <j.boggiano@seld.be>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Monolog\Handler;

use Raven_Client;

class MockRavenClient extends Raven_Client
{
public function capture($data, $stack)
{
$this->lastData = $data;
$this->lastStack = $stack;
}

public $lastData;
public $lastStack;
}
93 changes: 93 additions & 0 deletions tests/Monolog/Handler/RavenHandlerTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
<?php

/*
* This file is part of the Monolog package.
*
* (c) Jordi Boggiano <j.boggiano@seld.be>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Monolog\Handler;

use Monolog\TestCase;
use Monolog\Logger;
use Monolog\Handler\RavenHandler;

class RavenHandlerTest extends TestCase
{
public function setUp()
{
if (!class_exists("Raven_Client")) {
$this->markTestSkipped("raven/raven not installed");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an issue here: this will never be reached as a fatal error will already be thrown by the mock class definition. You would need to move MockRavenClient to a separate file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this seems to be a problem. Note that I copied this idiom from https://github.com/Seldaek/monolog/blob/master/tests/Monolog/Handler/GelfHandlerTest.php so it probably has the same problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I have a fix for this in 44d2441:

Here's how I tested with and without raven-php available...

(sentry)[last: 0] marca@SCML-MarcA:~/Sites/sentry-test/vendor/monolog/monolog$ ls -ld vendor/raven 
drwxr-xr-x 3 marca CHEGG\domain users 102 Apr 25 08:10 vendor/raven/
(sentry)[last: 0] marca@SCML-MarcA:~/Sites/sentry-test/vendor/monolog/monolog$ phpunit
PHPUnit 3.6.10 by Sebastian Bergmann.

Configuration read from /Users/marca/Sites/sentry-test/vendor/monolog/monolog/phpunit.xml.dist

...............................................................  63 / 137 ( 45%)
............................................................... 126 / 137 ( 91%)
...........

Time: 0 seconds, Memory: 13.50Mb

OK (137 tests, 328 assertions)
(sentry)[last: 0] marca@SCML-MarcA:~/Sites/sentry-test/vendor/monolog/monolog$ mv vendor/raven vendor/_raven
(sentry)[last: 0] marca@SCML-MarcA:~/Sites/sentry-test/vendor/monolog/monolog$ ls -ld vendor/raven
gls: cannot access vendor/raven: No such file or directory
(sentry)[last: 0] marca@SCML-MarcA:~/Sites/sentry-test/vendor/monolog/monolog$ phpunit
PHPUnit 3.6.10 by Sebastian Bergmann.

Configuration read from /Users/marca/Sites/sentry-test/vendor/monolog/monolog/phpunit.xml.dist

...............................................................  63 / 137 ( 45%)
.SSSS.......................................................... 126 / 137 ( 91%)
...........

Time: 1 second, Memory: 13.50Mb

OK, but incomplete or skipped tests!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like GelfHandlerTest has the same issue, filed as GH-78:

~/Sites/sentry-test/vendor/monolog/monolog$ mv vendor/mlehner vendor/_mlehner 
~/Sites/sentry-test/vendor/monolog/monolog$ ls -ld vendor/mlehner
ls: cannot access vendor/mlehner: No such file or directory
~/Sites/sentry-test/vendor/monolog/monolog$ phpunit
PHP Fatal error:  Class 'Gelf\MessagePublisher' not found in /Users/marca/Sites/sentry-test/vendor/monolog/monolog/tests/Monolog/Handler/GelfHandlerTest.php on line 21
PHP Stack trace:
PHP   1. {main}() /Users/marca/pear/bin/phpunit:0
PHP   2. PHPUnit_TextUI_Command::main() /Users/marca/pear/bin/phpunit:46
PHP   3. PHPUnit_TextUI_Command->run() /Users/marca/pear/share/pear/PHPUnit/TextUI/Command.php:130
PHP   4. PHPUnit_TextUI_Command->handleArguments() /Users/marca/pear/share/pear/PHPUnit/TextUI/Command.php:139
PHP   5. PHPUnit_Util_Configuration->getTestSuiteConfiguration() /Users/marca/pear/share/pear/PHPUnit/TextUI/Command.php:671
PHP   6. PHPUnit_Util_Configuration->getTestSuite() /Users/marca/pear/share/pear/PHPUnit/Util/Configuration.php:768
PHP   7. PHPUnit_Framework_TestSuite->addTestFiles() /Users/marca/pear/share/pear/PHPUnit/Util/Configuration.php:848
PHP   8. PHPUnit_Framework_TestSuite->addTestFile() /Users/marca/pear/share/pear/PHPUnit/Framework/TestSuite.php:419
PHP   9. PHPUnit_Util_Fileloader::checkAndLoad() /Users/marca/pear/share/pear/PHPUnit/Framework/TestSuite.php:358
PHP  10. PHPUnit_Util_Fileloader::load() /Users/marca/pear/share/pear/PHPUnit/Util/Fileloader.php:79
PHP  11. include_once() /Users/marca/pear/share/pear/PHPUnit/Util/Fileloader.php:95

Fatal error: Class 'Gelf\MessagePublisher' not found in /Users/marca/Sites/sentry-test/vendor/monolog/monolog/tests/Monolog/Handler/GelfHandlerTest.php on line 21

Call Stack:
    0.0003     633432   1. {main}() /Users/marca/pear/bin/phpunit:0
    0.0041    1150600   2. PHPUnit_TextUI_Command::main() /Users/marca/pear/bin/phpunit:46
    0.0042    1151328   3. PHPUnit_TextUI_Command->run() /Users/marca/pear/share/pear/PHPUnit/TextUI/Command.php:130
    0.0042    1151328   4. PHPUnit_TextUI_Command->handleArguments() /Users/marca/pear/share/pear/PHPUnit/TextUI/Command.php:139
    0.0164    3191376   5. PHPUnit_Util_Configuration->getTestSuiteConfiguration() /Users/marca/pear/share/pear/PHPUnit/TextUI/Command.php:671
    0.0164    3192408   6. PHPUnit_Util_Configuration->getTestSuite() /Users/marca/pear/share/pear/PHPUnit/Util/Configuration.php:768
    0.0232    3560352   7. PHPUnit_Framework_TestSuite->addTestFiles() /Users/marca/pear/share/pear/PHPUnit/Util/Configuration.php:848
    0.0596    6247784   8. PHPUnit_Framework_TestSuite->addTestFile() /Users/marca/pear/share/pear/PHPUnit/Framework/TestSuite.php:419
    0.0597    6248384   9. PHPUnit_Util_Fileloader::checkAndLoad() /Users/marca/pear/share/pear/PHPUnit/Framework/TestSuite.php:358
    0.0597    6248544  10. PHPUnit_Util_Fileloader::load() /Users/marca/pear/share/pear/PHPUnit/Util/Fileloader.php:79
    0.0600    6392056  11. include_once('/Users/marca/Sites/sentry-test/vendor/monolog/monolog/tests/Monolog/Handler/GelfHandlerTest.php') /Users/marca/pear/share/pear/PHPUnit/Util/Fileloader.php:95

}

require_once __DIR__ . '/MockRavenClient.php';
}

/**
* @covers Monolog\Handler\RavenHandler::__construct
*/
public function testConstruct()
{
$handler = new RavenHandler($this->getRavenClient());
$this->assertInstanceOf('Monolog\Handler\RavenHandler', $handler);
}

protected function getHandler($ravenClient)
{
$handler = new RavenHandler($ravenClient);
return $handler;
}

protected function getRavenClient()
{
$dsn = 'http://43f6017361224d098402974103bfc53d:a6a0538fc2934ba2bed32e08741b2cd3@marca.python.live.cheggnet.com:9000/1';
return new MockRavenClient($dsn);
}

public function testDebug()
{
$ravenClient = $this->getRavenClient();
$handler = $this->getHandler($ravenClient);

$record = $this->getRecord(Logger::DEBUG, "A test debug message");
$handler->handle($record);

$this->assertEquals($ravenClient::DEBUG, $ravenClient->lastData['level']);
$this->assertContains($record['message'], $ravenClient->lastData['message']);
}

public function testWarning()
{
$ravenClient = $this->getRavenClient();
$handler = $this->getHandler($ravenClient);

$record = $this->getRecord(Logger::WARNING, "A test warning message");
$handler->handle($record);

$this->assertEquals($ravenClient::WARNING, $ravenClient->lastData['level']);
$this->assertContains($record['message'], $ravenClient->lastData['message']);
}

public function testException()
{
$ravenClient = $this->getRavenClient();
$handler = $this->getHandler($ravenClient);

try {
$this->methodThatThrowsAnException();
} catch (\Exception $e) {
$record = $this->getRecord(Logger::ERROR, $e->getMessage(), array('exception' => $e));
$handler->handle($record);
}

$this->assertEquals($record['message'], $ravenClient->lastData['message']);
}

private function methodThatThrowsAnException()
{
throw new \Exception('This is an exception');
}
}