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

[Raven] Don't log exceptions twice #198

Merged
merged 4 commits into from
May 28, 2013
Merged

Conversation

davidwindell
Copy link
Contributor

As was picked up by in the review for #76, exceptions are being logged twice to Sentry which doesn't make any sense.

@Seldaek
Copy link
Owner

Seldaek commented May 24, 2013

Thanks for confirming this, I wasn't sure how sentry handles this so didn't want to end up ignoring some messages, but if it's duplicating them then that makes no sense. There is a little problem in the fact that the captureException method doesn't take anything else than the exception though, so you can't keep the log record's message and all, you just send the exception and lose the rest. I guess it's better than duplication but it's a bit sad.

I wonder if the if() there shouldn't just be if (isset($record['context']['exception']) && $record['context']['exception'] instanceof \Exception) {? In any case I think we should verify it's a proper exception instance, but also I don't think the log level really matters.

@davidwindell
Copy link
Contributor Author

@Seldaek thanks for the review, OK so I've done a bit of a clean-upr;

  • Removed check for Raven_Client version as v0.1.0 didn't support captureMessage anyway
  • We are now sending the formatted message as an extra key, so it appears in Sentry alongside the exception
  • The error level is also now retained for the exception :)

if you're happy with these changes, I'll update the tests accordingly.

@Seldaek
Copy link
Owner

Seldaek commented May 24, 2013

Looks good yes, thanks.

@davidwindell
Copy link
Contributor Author

Done, I've bumped Raven to 0.5.* too.

Seldaek added a commit that referenced this pull request May 28, 2013
[Raven] Don't log exceptions twice
@Seldaek Seldaek merged commit a3864e0 into Seldaek:master May 28, 2013
@Seldaek
Copy link
Owner

Seldaek commented May 28, 2013

Thanks

@davidwindell davidwindell deleted the patch-1 branch May 28, 2013 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants