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

BUG Show actual stacktrace for errors / exceptions #20

Merged
merged 2 commits into from
Dec 6, 2018

Conversation

tractorcow
Copy link
Contributor

@phptek Sentry had been only ever rendering stack traces for the error handling classes themselves; These are not so useful when trying to diagnose the actual issue. :)

Tested on both fatal errors, as well as exceptions.

Uses the work I did at Seldaek/monolog#1080 to capture non-exception traces as well. :)

'formatted' => $record['formatted'],
'channel' => $record['channel'],
{
$record = array_merge($record, [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added array_merge here to prevent other $record keys getting lost forever.

if (!empty($record['stack'])) {
$stack = (bool) $record['stack'];
if (!empty($record['stacktrace'])) {
$stack = $record['stacktrace'];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use actual trace array instead of bool to prevent sentry re-generating trace incorrectly.

Copy link
Owner

Choose a reason for hiding this comment

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

I dislike multi-type parameters intensely. If Raven::captureMessage() wants to accept an empty param for its 4th arg, it should allow an empty array....but then it goes on to accept $stack as null in Raven::capture() so what can you do...

I don't recall specifically why I cast to a bool, but it could be related to Raven_Client:909:

...
        if ((!$stack && $this->auto_log_stacks) || $stack === true) {
            $stack = debug_backtrace();
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you cast it as a bool, it forces the stacktrace to be generated internally.

If you pass an array, it uses that value as the actual stacktrace.

The problem is that monolog stores the stacktrace for later error reporting (register_shutdown_function), so it's useless to try to use debug_backtrace in the capture since you are no longer in the stack.

I think false is just no stacktrace.

/**
* @var SentryClientAdaptor
*/
protected $client;

/**
* @param int $level
* @param bool $bubble
Copy link
Owner

Choose a reason for hiding this comment

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

Which linting tool / ruleset are you using? I rather like vertically aligned param descriptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PHPStorm auto-format, sorry. I can undo if you like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in Editor > Code Style > PHP

I've turned this on. :)

image

*
* @param Member $member
*
* @param Member $member
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How is this @phptek ?

@tractorcow
Copy link
Contributor Author

@phptek I've done a linting review, fixed a few type hints, fixed a bug in RavenClient::getLevel(), and fixed the indentation based on your preference. How does it look now?

@phptek phptek merged commit c977aff into phptek:master Dec 6, 2018
@phptek
Copy link
Owner

phptek commented Dec 6, 2018

Merged. Many thanks @tractorcow

@tractorcow
Copy link
Contributor Author

Yay, thanks to you!

Now I feel bad about letting my PRs sit around for months when you have a same day turnaround, haha.

@tractorcow tractorcow deleted the bugfix/exceptions-work branch December 6, 2018 01:51
@phptek
Copy link
Owner

phptek commented Dec 6, 2018

LOL. I have standards to maintain seeing as PRs taking too long was the reason I built phptek/sentry instead of pushing PRs to the incumbent's Sentry module.

@tractorcow
Copy link
Contributor Author

@phptek can we please have a new release to include this fix? thanks.

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.

2 participants