Excluded exceptions are being ignored when monolog error handler is enabled

[lkazus]

I've recently enabled the monolog error handler on a Symfony project and realized that the excluded exceptions I configured are being reported.

After doing some digging, I've found that Sentry\Monolog\Handler relies on the captureEvent method which does not check if the payload contains an exception and if that exception should be ignored. I was wondering if it is an intended beheavior?

If not, I would be happy to submit a PR. The idea would be to use either the captureException or the captureEvent method whether the payload has an exception key set.

// Sentry\Monolog\Handler.php

protected function write(array $record): void
{
    ....

    if (isset($record['context']['exception']) && $record['context']['exception'] instanceof \Throwable) {
        $payload['exception'] = $record['context']['exception'];
    }

    $this->hub->withScope(function (Scope $scope) use ($record, $payload): void {
        ...

        if (array_key_exists('exception', $payload)) {
            $this->hub->captureException($payload['exception']);
        } else {
            $this->hub->captureEvent($payload);
        }
    });
}

[Jean85]

FTR, this comes from getsentry/sentry-symfony#273

[ste93cry]

The idea would be to use either the captureException or the captureEvent method whether the payload has an exception key set.

That would not work as you may want to pass a message along with a custom exception (it's a common use case), so with your suggestion you would lose one of the two information anyway. However I don't see any negative point in applying the excluded_exceptions option to all Client::capture* methods, so a contribution is more than welcome 😄

[Jean85]

As I already said in getsentry/sentry-symfony#273 (comment), I'm 👎 on altering the captureException behavior, since it would be a BC:

[...] that kind of fix could have an unintended consequence of blocking the sending of ignored exceptions when done manually, maybe with the explicit intent of overriding that option.

[ste93cry]

Nobody is going to alter the behavior of that method. The change would just move the if statement that applies the excluded_exceptions option from the captureException to the captureEvent method, and since the first calls the latter there would be no BC. The only BC that I can think of is that captureEvent now considers that option. We may solve the problem by using the $payload argument to decide whether we must filter or not the exception, if any, and drop support for this hack in 3.0

[CShigaki]

+1 on this. @lkazus are you working on a fix? If you're not I can submit a PR.
@ste93cry solution looks good to me.

[VincentLanglet]

A fix would be great, I got all my NotFoundHttpException and AccessDeniedHttpException back since I enabled the monolog error handler.

[ste93cry]

Working on it, I hope to open PR soon! However, I decided to try a more flexible way to handle such cases as it may be legit to use the Monolog handler and expect that all logs are captured regardless of the exception that they have attached, so the fix will likely target version 2.3 because it introduces a new feature and consequently to respect Semver it can't be a patch version

[Jean85]

For anyone willing to test the solution with PR #928 (which is now available in dev-develop, will be released as 2.3), here's the explanation of how it works: #928 (comment)