Add error types option

[d3f3kt]

According to my pull request: getsentry/sentry-php#369
With this error types options its possible to configure which error types should be
reported.

[Addvilz]

Why all of this magic?

Changing the service definition from

        arguments: ['%sentry.dsn%']

to

        arguments: ['%sentry.dsn%', %sentry.options%]

would achieve exactly the same goal, plus give access to all of the RavenClient options, not just this one.

[Addvilz]

Typo: error_types?

[d3f3kt]

To understand this commit, please check this pull request: getsentry/sentry-php#369

[Addvilz]

Line 4 has errro_types, should have error_types. Nothing to do with that change.

[Addvilz]

The test cases seems lacking. For configuration extension, SentrySymfonyClient, no container test...

[dcramer]

yeah they def are

[Addvilz]

Line 4 has errro_types, should have error_types. Nothing to do with that change.

[Addvilz]

Would it not be simpler to just pass array's RavenClient expect instead of transforming strings to constants, back to Sentry levels again?

[d3f3kt]

Oh, you're right - 'errro_types' was a typo. It's corrected now.

Of course it would be easier, but this is the established syntax to define error levels in php.

[Addvilz]

It is a way (legacy one, I might add) on how you define error reporting levels for PHP interpreter - nothing more. This configuration however is meant for Sentry, and Sentry has it's own error levels defined already - so it only makes sense, and is in a way expected behavior to follow Sentry convention.

As far as I know, otherwise, it's inconsistent - Symfony client uses one set of levels, Sentry uses another. What gives?

[d3f3kt]

I don't really know which Sentry error levels do you mean. The Raven_ErrorHandler exactly needs the error levels which i defined it in the configuration.

    /**
     * Register a handler which will intercept standard PHP errors and report them to the
     * associated Sentry client.
     *
     * @param bool $call_existing Call any existing errors handlers after processing
     *                            this instance.
     * @return array
     */
    public function registerErrorHandler($call_existing = true, $error_types = null)
    {
        if ($error_types !== null) {
            $this->error_types = $error_types;
        }
        $this->old_error_handler = set_error_handler(array($this, 'handleError'), E_ALL);
        $this->call_existing_error_handler = $call_existing;
        return $this;
    }

[Addvilz]

Yep, terribly sorry, I was under impression RavenClient uses "error", "warning" kind of strings.

Apart from that - can we avoid at least part of the conversions? For example, instead of having the php.ini-like single variable string E_ALL && ~E_WARNING, perhaps we can have array of levels we want and then just get the values via constant? Without all the preg_replace etc.

[Addvilz]

Well, consider that this code will be run every time there is an issue - so we probably want to keep the logic there to the absolute minimum. There aren't that many error types, and perhaps it should be inverse: e.g. list the types you don't want to receive. That would be consistent with exception filtering.

[d3f3kt]

That code is not rocket science. It's a simply preg_replace to secure the php eval function.

[Addvilz]

Of course. However, consider that this code is used in projects of all sizes, and this code is executed on construct - which is both good (executed just once) and bad thing (executed always). It's not a big issue if you get few hundred thousand executions per day. It becomes an issue when you have this running couple of billion times a day (like for us) - especially if there is a better and much less costly way to do it. Converting a list of "exclude" rules to constants and passing them on to Raven is such a way.

[d3f3kt]

Don't make a mountain out of a mole hill. It's a simply string replace operation.

And furthermore the code gets only executed if you have defined error types in your configuration.

[d3f3kt]

Short update - I've built a small test case for you:

class SentrySymfonyClient extends \Raven_Client
{
    public function __construct($dsn=null, $options=array())
    {
        $stopwatch = new Stopwatch();
        $stopwatch->start('initialize');

        if (true || array_key_exists('error_types', $options)) {
            $stopwatch->start('parser');
            $exParser = new ErrorTypesParser('E_ALL & ~E_DEPRECATED & ~E_NOTICE');
            $options['error_types'] = $exParser->parse();
            $parserEvent = $stopwatch->stop('parser');
        }

        $options['sdk'] = array(
            'name' => 'sentry-symfony',
            'version' => SentryBundle::VERSION,
        );
        parent::__construct($dsn, $options);
        $initEvent = $stopwatch->stop('initialize');

        echo $initEvent."\n";
        echo $parserEvent."\n";
    }
}

With following results. The complete init takes 2ms and the parser takes 0ms. That should be enough to prove a string replace function is not expensive.

[dcramer]

In general I think this is good, and its far easier to configure than other mechanisms.