neeckeloo / NewRelic

NewRelic module for Laminas
MIT License
33 stars 13 forks source link

Send original exception object to newrelic log #5

Closed bnkr closed 10 years ago

bnkr commented 10 years ago

I use the 'extra' part of the log event to give the exception to the log writer. This means we can pass the exception object itself which will mean the error type in newrelic is \My\Own\Exception rather than a NoticedError.

I used getMessage for the log message since the backtrace makes the log pretty unreadable when using __string (and is redundant since the backtrace is always included). Unfortunately newrelic seems to drop the point of call for the exception from its backtrace so I added in getLine and getFile too. Not particularly optimal, but I think it's newrelic's behaviour that's at fault here.

Finally I changed the name of the Logger service to something less likely to conflict with application code. It seems to be the convention for applications to name their services with the name of the class so it seems appropriate for a library module to have a name spaced name.

It would probably be better to write a Listener\ErrorListener as has been done for the other events in this module because then you can cover the event with tests. I didn't realise I could do this until after I'd patched but if you'd like that let me know and I'll refactor it.

neeckeloo commented 10 years ago

Thank you for your valuable suggestions. It's a good idea to create an error listener to log exceptions. I let you do.

bnkr commented 10 years ago

I also got the ErrorListener to handle MvcEvent::EVENT_RENDER_ERROR simply because it was easy and it looks like those errors don't get captured at all otherwise.

I wasn't entirely happy that some Listeners attach and then check config to see if they run while the error listener doesn't attach at all if it's turned off. It seems a bit inconsistent to me but I couldn't think which way to do it was better so I left it as-is. I don't intend to refactor this any further.

I also looked a little more into the use of getResult()->exception instead of getParam('exception'). It seems that setParam is called by the DispatchListener in marshallBadControllerEvent whereas the property getResult()->exception is set indirectly by the ExceptionStrategy returning a ViewModel. It seems to me that NewRelic should be accessing the value set by the DispatchListener rather than ExceptionStrategy, which is in fact another AbstractListener. I am not sure of my understanding of these internals of Zend so again I'll leave it to you.

neeckeloo commented 10 years ago

As you pointed out, it seems to be more appropriate to use the method "getParam" to retrieve the exception in both cases. I took the opportunity also to refactor the listener by injecting the logger during instantiation in order to simplify the listener and its associated tests.

bnkr commented 10 years ago

Yes, that's a much better implementation -- especially of the test. Thanks!