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

RavenHandler: check if exception capturing succeeded, and send a normal message if it has not #1208

Open
zerkms opened this issue Oct 25, 2018 · 3 comments
Labels

Comments

@zerkms
Copy link
Contributor

zerkms commented Oct 25, 2018

At the moment the RavenHandler capturing messages is implemented like this:

        if (isset($record['context']['exception']) && ($record['context']['exception'] instanceof \Exception || (PHP_VERSION_ID >= 70000 && $record['context']['exception'] instanceof \Throwable))) {
            $options['extra']['message'] = $record['formatted'];
            $this->ravenClient->captureException($record['context']['exception'], $options);
        } else {
            $this->ravenClient->captureMessage($record['formatted'], array(), $options);
        }

basically, if it's an exception - try to capture it, otherwise capture it as a message.

So far so good.

Sentry API accepts data json-encoded, so sentry client JSON-encodes everything that is to be sent.

A serialised exception contains the complete stack trace including arguments.

The problem is that those arguments sometimes are strings with binary data, which is more often than not is not valid UTF8, hence cannot be json-encoded.

If you open the official sentry client you'd find this line somewhere near Client.php:1047

$message = $this->encode($data);

If $data is exception details, then it would simply return false and set $this->_lasterror as something like malformed utf-8.

Then the data is attempted to be sent with $this->send_remote($this->server, $message, $headers); few lines below it.

There is unfortunately no way to detect if the SentryClient::send_remote completed unsuccessfully due to how they designed their API.

But in the monolog RavenHandler we could at least try our best, and here is how:

If right before $this->ravenClient->captureException($record['context']['exception'], $options); we

$lastSentryError = $this->ravenClient->getLastSentryError();

and then after $this->ravenClient->captureException(...) check

if ($lastSentryError !== $this->ravenClient->getLastSentryError()) { ... }

we then are 100% confident the data for some reason was not successfully sent.

What I propose is that - in case if it was not sent, we would run

$this->ravenClient->captureMessage($record['formatted'], array(), $options);

as a fallback.

Incomplete error (just a message won't contain the stack trace) is still better than no error, since at least you're informed. Additionally we could add something like [extras][monolog] = Could not serialise exception, an incomplete error.

But at least it would be something.

Thoughts?

@Seldaek
Copy link
Owner

Seldaek commented Nov 4, 2018

Sounds good to me. More reliability is definitely good. Missing exceptions entirely sucks.

@Seldaek Seldaek added the Feature label Nov 4, 2018
@zerkms
Copy link
Contributor Author

zerkms commented Nov 4, 2018

Awesome, I will allocate some time to prototype it after I return from a parental leave

@zerkms
Copy link
Contributor Author

zerkms commented Nov 15, 2018

I have found that there might be a better solution, using a concept of raven message processors:

  1. Define a processor that normalises and fixes the broken utf8:
final class Utf8NormalizerProcessor extends \Raven_Processor
{
    /**
     * @var \Raven_Serializer
     */
    private $serializer;

    public function __construct(\Raven_Client $client)
    {
        parent::__construct($client);

        $this->serializer = new \Raven_Serializer($client->mb_detect_order, $client->message_limit);
    }

    public function process(&$data)
    {
        foreach ($data as $key => $value) {
            if (is_string($value)) {
                $data[$key] = $this->serializer->serialize($value);
            } elseif (is_array($value)) {
                $data[$key] = $this->process($value);
            }
        }

        return $data;
    }
}
  1. Enable it together with default (and possible other processors, if any):
$ravenClient = new \Raven_Client($dsn, [
    // ... other options omitted
    'processors' => array_merge(
        \Raven_Client::getDefaultProcessors(),
        [Utf8NormalizerProcessor::class]
    ),
]);

I will send a suggestion to the raven-php project a bit later, but at the moment I think this solution is much more clear than what I initially suggested here above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants