Fix Guzzle tracing middleware logic

getsentry · May 16, 2024 · b501bef · b501bef
1 parent dfec37e
commit b501bef
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 42 deletions.
diff --git 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
@@ -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))