Queue tracing bugfixes (#820)

* Move queue tests to more logical place * Fix incorrectly memoizing config tests * Add tests for queue job tracing * Add failing test ensuring the queue span op is correctly set * Fix trace context being lost when finishing queue transaction * Revert "Fix trace context being lost when finishing queue transaction" This reverts commit ff3c28d. * Set all data on span before finishing in case its a transaction * Remove invalid —verbose from phpunit script That flag was removed in phpunit 10 and not really needed on older versions. * Bump required Sentry SDK version * Fix test being skipped because incorrect check
getsentry · Jan 16, 2024 · 9421a96 · 9421a96
1 parent debe205
commit 9421a96
Show file tree

Hide file tree

Showing 7 changed files with 70 additions and 19 deletions.
diff --git a/composer.json b/composer.json
@@ -25,7 +25,7 @@
     "require": {
         "php": "^7.2 | ^8.0",
         "illuminate/support": "^6.0 | ^7.0 | ^8.0 | ^9.0 | ^10.0",
-        "sentry/sentry": "^4.0",
+        "sentry/sentry": "^4.3",
         "symfony/psr-http-message-bridge": "^1.0 | ^2.0",
         "nyholm/psr7": "^1.0"
     },
@@ -56,7 +56,7 @@
             "@phpstan",
             "@tests"
         ],
-        "tests": "vendor/bin/phpunit --verbose",
+        "tests": "vendor/bin/phpunit",
         "cs-check": "vendor/bin/php-cs-fixer fix --verbose --diff --dry-run",
         "cs-fix": "vendor/bin/php-cs-fixer fix --verbose --diff",
         "phpstan": "vendor/bin/phpstan analyse"

diff --git a/src/Sentry/Laravel/Features/Feature.php b/src/Sentry/Laravel/Features/Feature.php
@@ -23,16 +23,16 @@ abstract class Feature
     /**
      * In-memory cache for the tracing feature flag.
      *
-     * @var bool|null
+     * @var array<string, bool>
      */
-    private $isTracingFeatureEnabled;
+    private $isTracingFeatureEnabled = [];
 
     /**
      * In-memory cache for the breadcumb feature flag.
      *
-     * @var bool|null
+     * @var array<string, bool>
      */
-    private $isBreadcrumbFeatureEnabled;
+    private $isBreadcrumbFeatureEnabled = [];
 
     /**
      * @param Container $container The Laravel application container.
@@ -126,23 +126,23 @@ protected function shouldSendDefaultPii(): bool
      */
     protected function isTracingFeatureEnabled(string $feature, bool $default = true): bool
     {
-        if ($this->isTracingFeatureEnabled === null) {
-            $this->isTracingFeatureEnabled = $this->isFeatureEnabled('tracing', $feature, $default);
+        if (!array_key_exists($feature, $this->isTracingFeatureEnabled)) {
+            $this->isTracingFeatureEnabled[$feature] = $this->isFeatureEnabled('tracing', $feature, $default);
         }
 
-        return $this->isTracingFeatureEnabled;
+        return $this->isTracingFeatureEnabled[$feature];
     }
 
     /**
      * Indicates if the given feature is enabled for breadcrumbs.
      */
     protected function isBreadcrumbFeatureEnabled(string $feature, bool $default = true): bool
     {
-        if ($this->isBreadcrumbFeatureEnabled === null) {
-            $this->isBreadcrumbFeatureEnabled = $this->isFeatureEnabled('breadcrumbs', $feature, $default);
+        if (!array_key_exists($feature, $this->isBreadcrumbFeatureEnabled)) {
+            $this->isBreadcrumbFeatureEnabled[$feature] = $this->isFeatureEnabled('breadcrumbs', $feature, $default);
         }
 
-        return $this->isBreadcrumbFeatureEnabled;
+        return $this->isBreadcrumbFeatureEnabled[$feature];
     }
 
     /**

diff --git a/src/Sentry/Laravel/Features/HttpClientIntegration.php b/src/Sentry/Laravel/Features/HttpClientIntegration.php
@@ -94,13 +94,13 @@ public function handleResponseReceivedHandlerForTracing(ResponseReceived $event)
         $span = $this->maybePopSpan();
 
         if ($span !== null) {
-            $span->finish();
             $span->setData(array_merge($span->getData(), [
                 // See: https://develop.sentry.dev/sdk/performance/span-data-conventions/#http
                 'http.response.status_code' => $event->response->status(),
                 'http.response.body.size' => $event->response->toPsrResponse()->getBody()->getSize(),
             ]));
             $span->setHttpStatus($event->response->status());
+            $span->finish();
         }
     }
 
@@ -109,8 +109,8 @@ public function handleConnectionFailedHandlerForTracing(ConnectionFailed $event)
         $span = $this->maybePopSpan();
 
         if ($span !== null) {
-            $span->finish();
             $span->setStatus(SpanStatus::internalError());
+            $span->finish();
         }
     }
 

diff --git a/src/Sentry/Laravel/Features/QueueIntegration.php b/src/Sentry/Laravel/Features/QueueIntegration.php
@@ -169,8 +169,8 @@ private function finishJobWithStatus(SpanStatus $status): void
         $span = $this->maybePopSpan();
 
         if ($span !== null) {
-            $span->finish();
             $span->setStatus($status);
+            $span->finish();
         }
     }
 

diff --git a/src/Sentry/Laravel/Tracing/EventHandler.php b/src/Sentry/Laravel/Tracing/EventHandler.php
@@ -266,8 +266,8 @@ protected function transactionCommittedHandler(DatabaseEvents\TransactionCommitt
         $span = $this->popSpan();
 
         if ($span !== null) {
-            $span->finish();
             $span->setStatus(SpanStatus::ok());
+            $span->finish();
         }
     }
 
@@ -276,8 +276,8 @@ protected function transactionRolledBackHandler(DatabaseEvents\TransactionRolled
         $span = $this->popSpan();
 
         if ($span !== null) {
-            $span->finish();
             $span->setStatus(SpanStatus::internalError());
+            $span->finish();
         }
     }
 

diff --git a/test/Sentry/Features/HttpClientIntegrationTest.php b/test/Sentry/Features/HttpClientIntegrationTest.php
@@ -5,6 +5,7 @@
 use GuzzleHttp\Psr7\Request as PsrRequest;
 use GuzzleHttp\Psr7\Response as PsrResponse;
 use Illuminate\Http\Client\Events\ResponseReceived;
+use Illuminate\Http\Client\Factory;
 use Illuminate\Http\Client\Request;
 use Illuminate\Http\Client\Response;
 use Illuminate\Support\Facades\Http;
@@ -128,7 +129,7 @@ public function testHttpClientSpanIsNotRecordedWhenDisabled(): void
 
     public function testHttpClientRequestTracingHeadersAreAttached(): void
     {
-        if (!method_exists(Http::class, 'globalRequestMiddleware')) {
+        if (!method_exists(Factory::class, 'globalRequestMiddleware')) {
             $this->markTestSkipped('The `globalRequestMiddleware` functionality we rely on was introduced in Laravel 10.14');
         }
 

diff --git a/test/Sentry/EventHandler/QueueEventsTest.php → .../Sentry/Features/QueueIntegrationTest.php b/test/Sentry/EventHandler/QueueEventsTest.php → .../Sentry/Features/QueueIntegrationTest.php
@@ -5,11 +5,12 @@
 use Exception;
 use Illuminate\Contracts\Queue\ShouldQueue;
 use Sentry\Breadcrumb;
+use Sentry\EventType;
 use Sentry\Laravel\Tests\TestCase;
 use function Sentry\addBreadcrumb;
 use function Sentry\captureException;
 
-class QueueEventsTest extends TestCase
+class QueueIntegrationTest extends TestCase
 {
     public function testQueueJobPushesAndPopsScopeWithBreadcrumbs(): void
     {
@@ -85,6 +86,55 @@ public function testQueueJobsWithBreadcrumbSetInBetweenKeepsNonJobBreadcrumbsOnC
 
         $this->assertCount(1, $this->getCurrentSentryBreadcrumbs());
     }
+
+    protected function withoutSampling($app): void
+    {
+        $app['config']->set('sentry.traces_sample_rate', 1.0);
+    }
+
+    /**
+     * @define-env withoutSampling
+     */
+    public function testQueueJobDoesntCreateTransactionByDefault(): void
+    {
+        dispatch(new QueueEventsTestJob);
+
+        $transaction = $this->getLastSentryEvent();
+
+        $this->assertNull($transaction);
+    }
+
+    protected function withQueueJobTracingEnabled($app): void
+    {
+        $app['config']->set('sentry.traces_sample_rate', 1.0);
+        $app['config']->set('sentry.tracing.queue_job_transactions', true);
+    }
+
+    /**
+     * @define-env withQueueJobTracingEnabled
+     */
+    public function testQueueJobCreatesTransactionWhenEnabled(): void
+    {
+        dispatch(new QueueEventsTestJob);
+
+        $transaction = $this->getLastSentryEvent();
+
+        $this->assertNotNull($transaction);
+
+        $this->assertEquals(EventType::transaction(), $transaction->getType());
+        $this->assertEquals(QueueEventsTestJob::class, $transaction->getTransaction());
+
+        $traceContext = $transaction->getContexts()['trace'];
+
+        $this->assertEquals('queue.process', $traceContext['op']);
+    }
+}
+
+class QueueEventsTestJob implements ShouldQueue
+{
+    public function handle(): void
+    {
+    }
 }
 
 function queueEventsTestAddTestBreadcrumb($message = null): void