Make an educated guess if the exception actually was handled (#617)

Author: stayallive

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## Unreleased
 
+- Fix exceptions sent via the `report()` helper being marked as unhandled (#617)
+
 ## 3.1.1
 
 - Fix missing scope information on unhandled exceptions (#611)

composer.json

@@ -23,7 +23,7 @@
     "require": {
         "php": "^7.2 | ^8.0",
         "illuminate/support": "^6.0 | ^7.0 | ^8.0 | ^9.0",
-        "sentry/sentry": "^3.10",
+        "sentry/sentry": "^3.11",
         "sentry/sdk": "^3.3",
         "symfony/psr-http-message-bridge": "^1.0 | ^2.0",
         "nyholm/psr7": "^1.0"

src/Sentry/Laravel/Integration.php

@@ -10,7 +10,6 @@
 use Sentry\Tracing\TransactionSource;
 use Throwable;
 use function Sentry\addBreadcrumb;
-use function Sentry\captureEvent;
 use function Sentry\configureScope;
 use Sentry\Breadcrumb;
 use Sentry\Event;
@@ -177,10 +176,63 @@ public static function sentryBaggageMeta(): string
      */
     public static function captureUnhandledException(Throwable $throwable): ?EventId
     {
+        // We instruct users to call `captureUnhandledException` in their exception handler, however this does not mean
+        // the exception was actually unhandled. Laravel has the `report` helper function that is used to report to a log
+        // file or Sentry, but that means they are handled otherwise they wouldn't have been routed through `report`. So to
+        // prevent marking those as "unhandled" we try and make an educated guess if the call to `captureUnhandledException`
+        // came from the `report` helper and shouldn't be marked as "unhandled" even though the come to us here to be reported
+        $handled = self::makeAnEducatedGuessIfTheExceptionMaybeWasHandled();
+
         $hint = EventHint::fromArray([
-            'mechanism' => new ExceptionMechanism(ExceptionMechanism::TYPE_GENERIC, false),
+            'mechanism' => new ExceptionMechanism(ExceptionMechanism::TYPE_GENERIC, $handled),
         ]);
 
         return SentrySdk::getCurrentHub()->captureException($throwable, $hint);
     }
+
+    /**
+     * Try to make an educated guess if the call came from the Laravel `report` helper.
+     *
+     * @see https://github.com/laravel/framework/blob/008a4dd49c3a13343137d2bc43297e62006c7f29/src/Illuminate/Foundation/helpers.php#L667-L682
+     *
+     * @return bool
+     */
+    private static function makeAnEducatedGuessIfTheExceptionMaybeWasHandled(): bool
+    {
+        // We limit the amount of backtrace frames since it is very unlikely to be any deeper
+        $trace = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 10);
+
+        // We are looking for `$handler->report()` to be called from the `report()` function
+        foreach ($trace as $frameIndex => $frame) {
+            // We need a frame with a class and function defined, we can skip frames missing either
+            if (!isset($frame['class'], $frame['function'])) {
+                continue;
+            }
+
+            // Check if the frame was indeed `$handler->report()`
+            if ($frame['type'] !== '->' || $frame['function'] !== 'report') {
+                continue;
+            }
+
+            // Make sure we have a next frame, we could have reached the end of the trace
+            if (!isset($trace[$frameIndex + 1])) {
+                continue;
+            }
+
+            // The next frame should contain the call to the `report()` helper function
+            $nextFrame = $trace[$frameIndex + 1];
+
+            // If a class was set or the function name is not `report` we can skip this frame
+            if (isset($nextFrame['class']) || !isset($nextFrame['function']) || $nextFrame['function'] !== 'report') {
+                continue;
+            }
+
+            // If we reached this point we can be pretty sure the `report` function was called
+            // and we can can come to the educated conclusion the exception was indeed handled
+            return true;
+        }
+
+        // If we reached this point we can be pretty sure the `report` function was not called
+        return false;
+    }
 }

test/Sentry/IntegrationTest.php

@@ -6,7 +6,9 @@
 use Illuminate\Routing\Events\RouteMatched;
 use Illuminate\Routing\Route;
 use Mockery;
+use RuntimeException;
 use Sentry\Event;
+use Sentry\ExceptionMechanism;
 use Sentry\Laravel\Integration;
 use Sentry\State\Scope;
 use Sentry\Tracing\TransactionSource;
@@ -111,6 +113,36 @@ public function testExtractingNameForRouteWithIncompleteGroupName(): void
         $this->assetRouteNameAndSource($route, $url, TransactionSource::route());
     }
 
+    public function testExceptionReportedUsingReportHelperIsNotMarkedAsUnhandled(): void
+    {
+        $testException = new RuntimeException('This was handled');
+
+        report($testException);
+
+        $this->assertCount(1, $this->lastSentryEvents);
+
+        [, $hint] = $this->lastSentryEvents[0];
+
+        $this->assertEquals($testException, $hint->exception);
+        $this->assertNotNull($hint->mechanism);
+        $this->assertTrue($hint->mechanism->isHandled());
+    }
+
+    public function testExceptionIsNotMarkedAsUnhandled(): void
+    {
+        $testException = new RuntimeException('This was not handled');
+
+        Integration::captureUnhandledException($testException);
+
+        $this->assertCount(1, $this->lastSentryEvents);
+
+        [, $hint] = $this->lastSentryEvents[0];
+
+        $this->assertEquals($testException, $hint->exception);
+        $this->assertNotNull($hint->mechanism);
+        $this->assertFalse($hint->mechanism->isHandled());
+    }
+
     private function assetRouteNameAndSource(Route $route, string $expectedName, TransactionSource $expectedSource): void
     {
         [$actualName, $actualSource] = Integration::extractNameAndSourceForRoute($route);

test/Sentry/TestCase.php

@@ -2,15 +2,20 @@
 
 namespace Sentry\Laravel\Tests;
 
+use Illuminate\Contracts\Debug\ExceptionHandler;
 use ReflectionMethod;
 use Sentry\Breadcrumb;
 use Sentry\ClientInterface;
+use Sentry\Event;
+use Sentry\EventHint;
+use Sentry\Laravel\Integration;
 use Sentry\State\Scope;
 use ReflectionProperty;
 use Sentry\Laravel\Tracing;
 use Sentry\State\HubInterface;
 use Sentry\Laravel\ServiceProvider;
 use Orchestra\Testbench\TestCase as LaravelTestCase;
+use Throwable;
 
 abstract class TestCase extends LaravelTestCase
 {
@@ -19,13 +24,28 @@ abstract class TestCase extends LaravelTestCase
         // or use the `$this->resetApplicationWithConfig([ /* config */ ]);` helper method
     ];
 
+    /** @var array<int, array{0: Event, 1: EventHint}> */
+    protected $lastSentryEvents = [];
+
     protected function getEnvironmentSetUp($app): void
     {
+        $this->lastSentryEvents = [];
+
+        $app['config']->set('sentry.before_send', function (Event $event, EventHint $hint) {
+            $this->lastSentryEvents[] = [$event, $hint];
+
+            return null;
+        });
+
         $app['config']->set('sentry.dsn', 'http://publickey:[email protected]/123');
 
         foreach ($this->setupConfig as $key => $value) {
             $app['config']->set($key, $value);
         }
+
+        $app->extend(ExceptionHandler::class, function (ExceptionHandler $handler) {
+            return new TestCaseExceptionHandler($handler);
+        });
     }
 
     protected function getPackageProviders($app): array

test/Sentry/TestCaseExceptionHandler.php

@@ -0,0 +1,48 @@
+<?php
+
+namespace Sentry\Laravel\Tests;
+
+use Illuminate\Contracts\Debug\ExceptionHandler;
+use Sentry\Laravel\Integration;
+
+/**
+ * This is a proxy class so we can inject the Sentry bits while running tests and handle exceptions like "normal".
+ *
+ * All type hints are remove from this class to prevent issues when running lower PHP versions where Throwable is not yet a thing.
+ */
+class TestCaseExceptionHandler implements ExceptionHandler
+{
+    private $handler;
+
+    public function __construct(ExceptionHandler $handler)
+    {
+        $this->handler = $handler;
+    }
+
+    public function report($e)
+    {
+        Integration::captureUnhandledException($e);
+
+        $this->handler->report($e);
+    }
+
+    public function shouldReport($e)
+    {
+        return $this->handler->shouldReport($e);
+    }
+
+    public function render($request, $e)
+    {
+        return $this->handler->render($request, $e);
+    }
+
+    public function renderForConsole($output, $e)
+    {
+        $this->handler->render($output, $e);
+    }
+
+    public function __call($name, $arguments)
+    {
+        return call_user_func_array([$this->handler, $name], $arguments);
+    }
+}