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

msabramo · 2012-04-25T22:40:49Z

Raven is the protocol used by Sentry. Uses raven-php, which is maintained by the Sentry team.

protocol used by Sentry (http://github.com/dcramer/sentry). Uses raven-php (https://github.com/getsentry/raven-php).

Seldaek · 2012-04-26T07:11:39Z

composer.json

@@ -16,9 +16,11 @@
    "php": ">=5.3.0"
  },
  "require-dev": {
+    "raven/raven": ">=0.2.0",


Please use 0.2.* or >=0.2.0,<1.0.0 or something more restrictive because this is not good since they could break BC in a future release. We shouldn't assume they won't.

See 8aeb75a

@Seldaek

"raven/raven": "0.2.*" as suggested by @Seldaek in https://github.com/Seldaek/monolog/pull/76/files#r737288

https://github.com/Seldaek/monolog/pull/76/files#r737290

@Seldaek

ERROR, because otherwise those messages will fail. As suggested by @Seldaek in https://github.com/Seldaek/monolog/pull/76/files#r737292

https://github.com/Seldaek/monolog/pull/76/files#r737297

https://github.com/Seldaek/monolog/pull/76/files#r737303

https://github.com/Seldaek/monolog/pull/76/files#r737313

@Seldaek

@Seldaek in https://github.com/Seldaek/monolog/pull/76/files#r737312

@Seldaek

Logger::ERROR, as suggested by @Seldaek in Seldaek#76 (comment)

@stof

by @stof in Seldaek#76 (comment)

RavenHandlerTest::testException

@stof

tests/Monolog/Handler/RavenHandlerTest.php into a separate file so that the test can pass when raven-php is installed and can be skipped when raven-php is not installed. Addresses @stof's comment in Seldaek#76 (comment)

stof · 2012-04-27T07:32:49Z

tests/Monolog/Handler/RavenHandlerTest.php

+            $this->methodThatThrowsAnException();
+        }
+        catch (\Exception $e)
+        {


curly braces should be on the same line than the control structure (same than if, for, foreach and while)

See 4830b92

Seldaek#76 (comment)

msabramo · 2012-05-02T00:14:21Z

Any more changes needed on this PR?

Seldaek · 2012-05-02T07:02:28Z

Nope it looks good but I haven't had time to look more in depth.

msabramo · 2012-05-06T19:23:56Z

@stof, Any more feedback?

stof · 2012-05-06T20:16:23Z

src/Monolog/Formatter/RavenFormatter.php

+    {
+        $record = parent::format($record);
+
+        $record['level'] = $this->logLevels[$record['level']];


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

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

yguedidi · 2012-10-24T08:50:19Z

Some news about this PR ?

stof · 2012-10-24T09:38:09Z

Well, there is still some feedback which has not been taken into account

msabramo · 2012-10-25T06:58:14Z

Sorry to take so long. I'm not really working with Sentry or monolog these days, so this is all very rusty in my head now I'm afraid.

I just pushed commit 5d17212 which addresses some but not all of @stof's comments. Maybe it's too late at night or perhaps it's simply because I've been away from monolog and raven for a long time, but I had trouble grokking some of it, particularly the comment about the $params passed to the Raven_Client, so I didn't handle all of the concerns.

If there are other folks who are interested in this PR, then it may be most expedient for them to tackle what's remaining. I don't have quite as much time or motivation as I did when I first submitted this many months ago.

I wonder also if the tests could use some improving. It sounds like some stuff with params doesn't work, so maybe I am missing some test cases here.

msabramo · 2012-10-25T15:48:16Z

Looking at this a little bit more this morning, I guess I'm a little confused about what should be done in formatters vs. handlers.

Some of the existing stuff in Monolog follows what you said -- where the translation of log levels happens in the handler and often these have no formatter (which makes sense because the formatters as they're explained in the docs mostly do things that are completely independent of the handler -- e.g.: adding timestamps). An example of this type of handler is SyslogHandler.

Others are splitting processing between the handler and the formatter (and I probably copied the structure of one of these; in fact, I probably copied GelfHandler since Graylog seemed most similar to Sentry). Examples are:

FirePHPHandler and WildfireFormatter.
ChromePHPHandler and ChromePHPFormatter
GelfHandler and GelfMessageFormatter

I think I followed the model of the latter group (specifically GelfHandler), but I wonder if I should be following the model of the former. It seems to me that I probably could get by with just RavenHandler and completely get rid of RavenFormatter. This would let me take advantage of other formatters. Am I right about this?

stof · 2012-10-25T15:59:01Z

In this case, indeed. Your formatted message is not a special format (like for ChromePHP or FirePHP) but a string. So you should simply use a LineFormatter here (btw, your RavenFormatter is simply a Lineformatter with a particular format which is not the default one)

msabramo · 2012-10-25T16:00:26Z

Can I just remove getDefaultFormatter entirely and let it fall back to the default formatter defined by AbstractHandler?

msabramo · 2012-10-25T16:02:09Z

I'm also leaning towards passing $params = array() to Raven_Client::captureMessage because I don't see anything useful at the moment that I can put there (I think what I had before was silly and not doing anything). This seems to be a special feature of Raven which monolog doesn't really need (since it has its own formatters)...?

text-oriented interface and doesn't need special processing.

msabramo · 2012-10-25T16:07:11Z

Hopefully, with 368b1f0, we are almost there.

stof · 2012-10-25T16:13:43Z

src/Monolog/Handler/RavenHandler.php

+     */
+    public function close()
+    {
+        $this->ravenClient = null;


Removing the dependency seems weird to me

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.

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.

msabramo · 2012-10-25T16:31:18Z

OK, I removed the close method. As an aside, GelfHandler does this as well and that is almost certainly where it came from for me.

Personally, I expect things to be unusable after I call close on them so that didn't bother me. But I don't think it matters much either way as garbage collection will take care of like you said, so I'm fine with removing the close method.

Maybe close in monolog is more of a flush in that for RotatingFileHandler it does log rotation. I don't think I realized that until now and thought of close more in the sense of what you do on a raw file or a database connection.

Although just now I noticed that StreamHandler also closes and sets the underlying stream to null. So I don't know what close is supposed to do; I guess it's implementation-dependent.

I don't really see any place where Logger calls close (either implicitly or exposing a method that lets the user close the logger), so this might be a moot point.

msabramo · 2012-10-29T19:53:09Z

Let me know if you'd like me to make more changes.

stof · 2012-10-29T19:55:48Z

This looks good to me. Could you update the readme to add the handler in the list ?

msabramo · 2012-10-30T15:05:30Z

Okey doke. Done in 10fcd61.

msabramo · 2012-11-01T18:13:39Z

Bump (only because my last day at my PHP-focused job is tomorrow and I'd like to get closure on this before I get busy with other stuff).

Seldaek · 2012-11-01T22:13:26Z

@msabramo thanks, unfortunately I don't have time right now but I'll try to look at all the monolog PRs in the nearish future. I'm sure this is fine by now though, don't let this keep you up at night :)

msabramo · 2012-11-01T23:40:29Z

@Seldaek Thanks! No problem. If minor tweaks are needed later on I might be able to make them, but I hesitate to commit. Like you said, it's been through quite a few rounds of discussion, so it's probably very close.

vytautasgimbutas · 2012-11-07T10:47:34Z

I think you shouldn't use captureMessage as you can't pass additional data parameters.

luxifer · 2012-11-22T14:44:20Z

+1 for this pull request

skrivanos · 2012-12-07T08:01:02Z

src/Monolog/Handler/RavenHandler.php

+        );
+        if ($record['level'] >= Logger::ERROR && isset($record['context']['exception'])) {
+            $this->ravenClient->captureException($record['context']['exception']);
+        }


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

Seldaek · 2013-01-07T20:10:15Z

Merged, thanks. I mapped the new monolog log levels to sentry equivalents and added FATAL levels since it seems raven 0.3 added that level. If anyone uses this and thinks it can be improved somehow feel free to open an issue. I know there was some unresolved feedback above but from what I can tell it should work as it is, I just don't know sentry/raven enough to assess this in greater detail. Feedback from the trenches would be welcome.

Add RavenHandler and RavenFormatter for working with Raven which is the

acbf574

protocol used by Sentry (http://github.com/dcramer/sentry). Uses raven-php (https://github.com/getsentry/raven-php).

Seldaek reviewed Apr 26, 2012
View reviewed changes

msabramo added 11 commits April 26, 2012 14:30

composer.json: Tighten requirement "raven/raven": ">=0.2.0" to

8aeb75a

"raven/raven": "0.2.*" as suggested by @Seldaek in https://github.com/Seldaek/monolog/pull/76/files#r737288

Remove leading \ in use statement, as suggested by @Seldaek in

ac161a0

https://github.com/Seldaek/monolog/pull/76/files#r737290

Map CRITICAL and ALERT to ERROR because raven doesn't have more than

0e579f1

ERROR, because otherwise those messages will fail. As suggested by @Seldaek in https://github.com/Seldaek/monolog/pull/76/files#r737292

Minor reformatting suggested by @Seldaek in

f158009

https://github.com/Seldaek/monolog/pull/76/files#r737297

Put braces on same line for if statements, as requested by @Seldaek in

6d15811

https://github.com/Seldaek/monolog/pull/76/files#r737303

Put args one per line, as suggested by @Seldaek in

f6e75e4

https://github.com/Seldaek/monolog/pull/76/files#r737313

Rework how exceptions are stored and accessed using suggestions from

3238a74

@Seldaek in https://github.com/Seldaek/monolog/pull/76/files#r737312

Change check of record level to >= Logger::ERROR instead of >

1c59696

Logger::ERROR, as suggested by @Seldaek in Seldaek#76 (comment)

Eliminate local variables used as pseudo-named-arguments, as suggested

7a81acd

by @stof in Seldaek#76 (comment)

Change key from 'context' to 'exception' in

6b4b2a6

RavenHandlerTest::testException

Pull MockRavenClient (which depends on Raven_Client) out of

44d2441

tests/Monolog/Handler/RavenHandlerTest.php into a separate file so that the test can pass when raven-php is installed and can be skipped when raven-php is not installed. Addresses @stof's comment in Seldaek#76 (comment)

stof reviewed Apr 27, 2012
View reviewed changes

Curly braces on same line for try/catch, as suggested by @stof in

4830b92

Seldaek#76 (comment)

stof reviewed May 6, 2012
View reviewed changes

Address a few of the concerns from @stof in Seldaek#76.

5d17212

Remove RavenFormatter; this is not needed since Raven is a very simple

368b1f0

text-oriented interface and doesn't need special processing.

stof reviewed Oct 25, 2012
View reviewed changes

Remove close method which was setting $this->ravenClient = null

c2850f1

Add note about RavenHandler to README.mdown

10fcd61

skrivanos reviewed Dec 7, 2012
View reviewed changes

Seldaek merged commit 10fcd61 into Seldaek:master Jan 7, 2013

davidwindell mentioned this pull request May 24, 2013

[Raven] Don't log exceptions twice #198

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

msabramo commented Apr 25, 2012

Seldaek Apr 26, 2012

msabramo Apr 26, 2012

stof Apr 27, 2012

msabramo Apr 27, 2012

msabramo commented May 2, 2012

Seldaek commented May 2, 2012

msabramo commented May 6, 2012

stof May 6, 2012

stof Jun 19, 2012

yguedidi commented Oct 24, 2012

stof commented Oct 24, 2012

msabramo commented Oct 25, 2012

msabramo commented Oct 25, 2012

stof commented Oct 25, 2012

msabramo commented Oct 25, 2012

msabramo commented Oct 25, 2012

msabramo commented Oct 25, 2012

stof Oct 25, 2012

msabramo Oct 25, 2012

stof Oct 25, 2012

msabramo commented Oct 25, 2012

msabramo commented Oct 29, 2012

stof commented Oct 29, 2012

msabramo commented Oct 30, 2012

msabramo commented Nov 1, 2012

Seldaek commented Nov 1, 2012

msabramo commented Nov 1, 2012

vytautasgimbutas commented Nov 7, 2012

luxifer commented Nov 22, 2012

skrivanos Dec 7, 2012

Seldaek commented Jan 7, 2013

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

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

Conversation

msabramo commented Apr 25, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msabramo commented May 2, 2012

Seldaek commented May 2, 2012

msabramo commented May 6, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yguedidi commented Oct 24, 2012

stof commented Oct 24, 2012

msabramo commented Oct 25, 2012

msabramo commented Oct 25, 2012

stof commented Oct 25, 2012

msabramo commented Oct 25, 2012

msabramo commented Oct 25, 2012

msabramo commented Oct 25, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msabramo commented Oct 25, 2012

msabramo commented Oct 29, 2012

stof commented Oct 29, 2012

msabramo commented Oct 30, 2012

msabramo commented Nov 1, 2012

Seldaek commented Nov 1, 2012

msabramo commented Nov 1, 2012

vytautasgimbutas commented Nov 7, 2012

luxifer commented Nov 22, 2012

Choose a reason for hiding this comment

Seldaek commented Jan 7, 2013