From 9421a96ebad781ea1475779ac975555206a99000 Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Tue, 16 Jan 2024 08:31:53 +0100 Subject: [PATCH] Queue tracing bugfixes (#820) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 ff3c28d8377a94d9a3d564340ee60410ba2f5731. * 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 --- composer.json | 4 +- src/Sentry/Laravel/Features/Feature.php | 20 +++---- .../Features/HttpClientIntegration.php | 4 +- .../Laravel/Features/QueueIntegration.php | 2 +- src/Sentry/Laravel/Tracing/EventHandler.php | 4 +- .../Features/HttpClientIntegrationTest.php | 3 +- .../QueueIntegrationTest.php} | 52 ++++++++++++++++++- 7 files changed, 70 insertions(+), 19 deletions(-) rename test/Sentry/{EventHandler/QueueEventsTest.php => Features/QueueIntegrationTest.php} (74%) diff --git a/composer.json b/composer.json index a477bc8d..bdadd653 100644 --- 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 index e1e3e255..18ad156a 100644 --- 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 */ - private $isTracingFeatureEnabled; + private $isTracingFeatureEnabled = []; /** * In-memory cache for the breadcumb feature flag. * - * @var bool|null + * @var array */ - private $isBreadcrumbFeatureEnabled; + private $isBreadcrumbFeatureEnabled = []; /** * @param Container $container The Laravel application container. @@ -126,11 +126,11 @@ 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]; } /** @@ -138,11 +138,11 @@ protected function isTracingFeatureEnabled(string $feature, bool $default = true */ 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 index be57e7f5..56e81249 100644 --- 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 index 28560dc4..18f12417 100644 --- 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 index 775c7fdf..0a481a07 100644 --- 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 index edd61698..63907c8d 100644 --- 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 b/test/Sentry/Features/QueueIntegrationTest.php similarity index 74% rename from test/Sentry/EventHandler/QueueEventsTest.php rename to test/Sentry/Features/QueueIntegrationTest.php index 91f1ca77..fc472a8b 100644 --- a/test/Sentry/EventHandler/QueueEventsTest.php +++ b/test/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