From dfec37e7c3ba02fabaac62ae75c56e3e6d9a4dee Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Thu, 16 May 2024 12:38:59 +0200 Subject: [PATCH 1/3] Also test the sampled state of a span before creating a child --- src/functions.php | 8 ++++---- tests/FunctionsTest.php | 1 + 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/functions.php b/src/functions.php index 9165a8066..33bb5bdca 100644 --- a/src/functions.php +++ b/src/functions.php @@ -250,10 +250,10 @@ function trace(callable $trace, SpanContext $context) return SentrySdk::getCurrentHub()->withScope(function (Scope $scope) use ($context, $trace) { $parentSpan = $scope->getSpan(); - // If there's a span set on the scope there is a transaction - // active currently. If that is the case we create a child span - // and set it on the scope. Otherwise we only execute the callable - if ($parentSpan !== null) { + // If there is a span set on the scope and it's sampled there is an active transaction. + // If that is the case we create the child span and set it on the scope. + // Otherwise we only execute the callable without creating a span. + if ($parentSpan !== null && $parentSpan->getSampled()) { $span = $parentSpan->startChild($context); $scope->setSpan($span); diff --git a/tests/FunctionsTest.php b/tests/FunctionsTest.php index 77a5c6ee7..50cd79626 100644 --- a/tests/FunctionsTest.php +++ b/tests/FunctionsTest.php @@ -365,6 +365,7 @@ public function testTraceCorrectlyReplacesAndRestoresCurrentSpan(): void $hub = new Hub(); $transaction = new Transaction(new TransactionContext()); + $transaction->setSampled(true); $hub->setSpan($transaction); From b501bef9bb03598168ca966098c2e4eee5c19acb Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Thu, 16 May 2024 12:55:47 +0200 Subject: [PATCH 2/3] Fix Guzzle tracing middleware logic --- src/Tracing/GuzzleTracingMiddleware.php | 71 +++++++++---------- tests/Tracing/GuzzleTracingMiddlewareTest.php | 8 +-- 2 files changed, 37 insertions(+), 42 deletions(-) diff --git a/src/Tracing/GuzzleTracingMiddleware.php b/src/Tracing/GuzzleTracingMiddleware.php index 08e640359..6dfde7840 100644 --- a/src/Tracing/GuzzleTracingMiddleware.php +++ b/src/Tracing/GuzzleTracingMiddleware.php @@ -28,18 +28,7 @@ public static function trace(?HubInterface $hub = null): \Closure return static function (RequestInterface $request, array $options) use ($hub, $handler) { $hub = $hub ?? SentrySdk::getCurrentHub(); $client = $hub->getClient(); - $span = $hub->getSpan(); - - if ($span === null) { - if (self::shouldAttachTracingHeaders($client, $request)) { - $request = $request - ->withHeader('sentry-trace', getTraceparent()) - ->withHeader('traceparent', getW3CTraceparent()) - ->withHeader('baggage', getBaggage()); - } - - return $handler($request, $options); - } + $parentSpan = $hub->getSpan(); $partialUri = Uri::fromParts([ 'scheme' => $request->getUri()->getScheme(), @@ -60,24 +49,30 @@ public static function trace(?HubInterface $hub = null): \Closure $spanAndBreadcrumbData['http.fragment'] = $request->getUri()->getFragment(); } - $spanContext = new SpanContext(); - $spanContext->setOp('http.client'); - $spanContext->setDescription($request->getMethod() . ' ' . $partialUri); - $spanContext->setData($spanAndBreadcrumbData); + $childSpan = null; + + if ($parentSpan !== null && $parentSpan->getSampled()) { + $spanContext = new SpanContext(); + $spanContext->setOp('http.client'); + $spanContext->setDescription($request->getMethod() . ' ' . $partialUri); + $spanContext->setData($spanAndBreadcrumbData); - $childSpan = $span->startChild($spanContext); + $childSpan = $parentSpan->startChild($spanContext); + } if (self::shouldAttachTracingHeaders($client, $request)) { $request = $request - ->withHeader('sentry-trace', $childSpan->toTraceparent()) - ->withHeader('traceparent', $childSpan->toW3CTraceparent()) - ->withHeader('baggage', $childSpan->toBaggage()); + ->withHeader('sentry-trace', getTraceparent()) + ->withHeader('traceparent', getW3CTraceparent()) + ->withHeader('baggage', getBaggage()); } $handlerPromiseCallback = static function ($responseOrException) use ($hub, $spanAndBreadcrumbData, $childSpan, $partialUri) { - // We finish the span (which means setting the span end timestamp) first to ensure the measured time - // the span spans is as close to only the HTTP request time and do the data collection afterwards - $childSpan->finish(); + if ($childSpan !== null) { + // We finish the span (which means setting the span end timestamp) first to ensure the measured time + // the span spans is as close to only the HTTP request time and do the data collection afterwards + $childSpan->finish(); + } $response = null; @@ -91,11 +86,15 @@ public static function trace(?HubInterface $hub = null): \Closure if ($response !== null) { $spanAndBreadcrumbData['http.response.body.size'] = $response->getBody()->getSize(); $spanAndBreadcrumbData['http.response.status_code'] = $response->getStatusCode(); + } - $childSpan->setStatus(SpanStatus::createFromHttpStatusCode($response->getStatusCode())); - $childSpan->setData($spanAndBreadcrumbData); - } else { - $childSpan->setStatus(SpanStatus::internalError()); + if ($childSpan !== null) { + if ($response !== null) { + $childSpan->setStatus(SpanStatus::createFromHttpStatusCode($response->getStatusCode())); + $childSpan->setData($spanAndBreadcrumbData); + } else { + $childSpan->setStatus(SpanStatus::internalError()); + } } $hub->addBreadcrumb(new Breadcrumb( @@ -122,18 +121,14 @@ public static function trace(?HubInterface $hub = null): \Closure private static function shouldAttachTracingHeaders(?ClientInterface $client, RequestInterface $request): bool { - if ($client !== null) { - $sdkOptions = $client->getOptions(); - - // Check if the request destination is allow listed in the trace_propagation_targets option. - if ( - $sdkOptions->getTracePropagationTargets() === null - || \in_array($request->getUri()->getHost(), $sdkOptions->getTracePropagationTargets()) - ) { - return true; - } + if ($client === null) { + return false; } - return false; + $sdkOptions = $client->getOptions(); + + // Check if the request destination is allow listed in the trace_propagation_targets option. + return $sdkOptions->getTracePropagationTargets() === null + || \in_array($request->getUri()->getHost(), $sdkOptions->getTracePropagationTargets()); } } diff --git a/tests/Tracing/GuzzleTracingMiddlewareTest.php b/tests/Tracing/GuzzleTracingMiddlewareTest.php index ae3c30e30..349adadf0 100644 --- a/tests/Tracing/GuzzleTracingMiddlewareTest.php +++ b/tests/Tracing/GuzzleTracingMiddlewareTest.php @@ -23,10 +23,10 @@ final class GuzzleTracingMiddlewareTest extends TestCase { - public function testTraceDoesNothingIfSpanIsNotSet(): void + public function testTraceCreatesBreadcrumbIfSpanIsNotSet(): void { $client = $this->createMock(ClientInterface::class); - $client->expects($this->once()) + $client->expects($this->atLeast(2)) ->method('getOptions') ->willReturn(new Options()); @@ -55,7 +55,7 @@ public function testTraceDoesNothingIfSpanIsNotSet(): void $scope->applyToEvent($event); - $this->assertCount(0, $event->getBreadcrumbs()); + $this->assertCount(1, $event->getBreadcrumbs()); }); } @@ -95,7 +95,7 @@ public function testTraceHeaders(Request $request, Options $options, bool $heade /** * @dataProvider traceHeadersDataProvider */ - public function testTraceHeadersWithTransacttion(Request $request, Options $options, bool $headersShouldBePresent): void + public function testTraceHeadersWithTransaction(Request $request, Options $options, bool $headersShouldBePresent): void { $client = $this->createMock(ClientInterface::class); $client->expects($this->atLeast(2)) From b79a3cdb006eed3686226b57a2ae0bdbee25e2e4 Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Fri, 24 May 2024 11:30:00 +0200 Subject: [PATCH 3/3] Add more test cases --- tests/FunctionsTest.php | 26 ++++- tests/Tracing/GuzzleTracingMiddlewareTest.php | 100 +++++++++++++++++- 2 files changed, 124 insertions(+), 2 deletions(-) diff --git a/tests/FunctionsTest.php b/tests/FunctionsTest.php index 50cd79626..ce892e853 100644 --- a/tests/FunctionsTest.php +++ b/tests/FunctionsTest.php @@ -364,7 +364,7 @@ public function testTraceCorrectlyReplacesAndRestoresCurrentSpan(): void { $hub = new Hub(); - $transaction = new Transaction(new TransactionContext()); + $transaction = new Transaction(TransactionContext::make()); $transaction->setSampled(true); $hub->setSpan($transaction); @@ -388,6 +388,30 @@ public function testTraceCorrectlyReplacesAndRestoresCurrentSpan(): void } } + public function testTraceDoesntCreateSpanIfTransactionIsNotSampled(): void + { + $scope = $this->createMock(Scope::class); + + $hub = new Hub(null, $scope); + + $transaction = new Transaction(TransactionContext::make()); + $transaction->setSampled(false); + + $scope->expects($this->never()) + ->method('setSpan'); + $scope->expects($this->exactly(3)) + ->method('getSpan') + ->willReturn($transaction); + + SentrySdk::setCurrentHub($hub); + + trace(function () use ($transaction, $hub) { + $this->assertSame($transaction, $hub->getSpan()); + }, SpanContext::make()); + + $this->assertSame($transaction, $hub->getSpan()); + } + public function testTraceparentWithTracingDisabled(): void { $propagationContext = PropagationContext::fromDefaults(); diff --git a/tests/Tracing/GuzzleTracingMiddlewareTest.php b/tests/Tracing/GuzzleTracingMiddlewareTest.php index 349adadf0..46e9a8325 100644 --- a/tests/Tracing/GuzzleTracingMiddlewareTest.php +++ b/tests/Tracing/GuzzleTracingMiddlewareTest.php @@ -28,10 +28,60 @@ public function testTraceCreatesBreadcrumbIfSpanIsNotSet(): void $client = $this->createMock(ClientInterface::class); $client->expects($this->atLeast(2)) ->method('getOptions') - ->willReturn(new Options()); + ->willReturn(new Options([ + 'traces_sample_rate' => 0, + ])); + + $hub = new Hub($client); + + $transaction = $hub->startTransaction(TransactionContext::make()); + + $this->assertFalse($transaction->getSampled()); + + $expectedPromiseResult = new Response(); + + $middleware = GuzzleTracingMiddleware::trace($hub); + $function = $middleware(static function () use ($expectedPromiseResult): PromiseInterface { + return new FulfilledPromise($expectedPromiseResult); + }); + + /** @var PromiseInterface $promise */ + $promise = $function(new Request('GET', 'https://www.example.com'), []); + + try { + $promiseResult = $promise->wait(); + } catch (\Throwable $exception) { + $promiseResult = $exception; + } + + $this->assertSame($expectedPromiseResult, $promiseResult); + + $this->assertNull($transaction->getSpanRecorder()); + + $hub->configureScope(function (Scope $scope): void { + $event = Event::createEvent(); + + $scope->applyToEvent($event); + + $this->assertCount(1, $event->getBreadcrumbs()); + }); + } + + public function testTraceCreatesBreadcrumbIfSpanIsRecorded(): void + { + $client = $this->createMock(ClientInterface::class); + $client->expects($this->atLeast(2)) + ->method('getOptions') + ->willReturn(new Options([ + 'traces_sample_rate' => 1, + ])); $hub = new Hub($client); + $transaction = $hub->startTransaction(TransactionContext::make()); + + $this->assertTrue($transaction->getSampled()); + $expectedPromiseResult = new Response(); $middleware = GuzzleTracingMiddleware::trace($hub); @@ -50,6 +100,9 @@ public function testTraceCreatesBreadcrumbIfSpanIsNotSet(): void $this->assertSame($expectedPromiseResult, $promiseResult); + $this->assertNotNull($transaction->getSpanRecorder()); + $this->assertCount(1, $transaction->getSpanRecorder()->getSpans()); + $hub->configureScope(function (Scope $scope): void { $event = Event::createEvent(); @@ -133,6 +186,15 @@ public function testTraceHeadersWithTransaction(Request $request, Options $optio public static function traceHeadersDataProvider(): iterable { + // Test cases here are duplicated with sampling enabled and disabled because trace headers hould be added regardless of the sample decision + + yield [ + new Request('GET', 'https://www.example.com'), + new Options([ + 'traces_sample_rate' => 0, + ]), + true, + ]; yield [ new Request('GET', 'https://www.example.com'), new Options([ @@ -141,6 +203,14 @@ public static function traceHeadersDataProvider(): iterable true, ]; + yield [ + new Request('GET', 'https://www.example.com'), + new Options([ + 'traces_sample_rate' => 0, + 'trace_propagation_targets' => null, + ]), + true, + ]; yield [ new Request('GET', 'https://www.example.com'), new Options([ @@ -150,6 +220,16 @@ public static function traceHeadersDataProvider(): iterable true, ]; + yield [ + new Request('GET', 'https://www.example.com'), + new Options([ + 'traces_sample_rate' => 0, + 'trace_propagation_targets' => [ + 'www.example.com', + ], + ]), + true, + ]; yield [ new Request('GET', 'https://www.example.com'), new Options([ @@ -161,6 +241,14 @@ public static function traceHeadersDataProvider(): iterable true, ]; + yield [ + new Request('GET', 'https://www.example.com'), + new Options([ + 'traces_sample_rate' => 0, + 'trace_propagation_targets' => [], + ]), + false, + ]; yield [ new Request('GET', 'https://www.example.com'), new Options([ @@ -170,6 +258,16 @@ public static function traceHeadersDataProvider(): iterable false, ]; + yield [ + new Request('GET', 'https://www.example.com'), + new Options([ + 'traces_sample_rate' => 0, + 'trace_propagation_targets' => [ + 'example.com', + ], + ]), + false, + ]; yield [ new Request('GET', 'https://www.example.com'), new Options([