Set the span status when tracing an HTTP client request (#748)

Author: ste93cry

Diff for: src/Tracing/HttpClient/AbstractTraceableResponse.php

@@ -5,6 +5,7 @@
 namespace Sentry\SentryBundle\Tracing\HttpClient;
 
 use Sentry\Tracing\Span;
+use Sentry\Tracing\SpanStatus;
 use Symfony\Contracts\HttpClient\ChunkInterface;
 use Symfony\Contracts\HttpClient\HttpClientInterface;
 use Symfony\Contracts\HttpClient\ResponseInterface;
@@ -123,9 +124,27 @@ public static function stream(HttpClientInterface $client, iterable $responses,
 
     private function finishSpan(): void
     {
-        if (null !== $this->span) {
-            $this->span->finish();
-            $this->span = null;
+        if (null === $this->span) {
+            return;
         }
+
+        // We finish the span (which means setting the span end timestamp) first
+        // to ensure the measured time is as close as possible to the duration of
+        // the HTTP request
+        $this->span->finish();
+
+        /** @var int $statusCode */
+        $statusCode = $this->response->getInfo('http_code');
+
+        // If the returned status code is 0, it means that this info isn't available
+        // yet (e.g. an error happened before the request was sent), hence we cannot
+        // determine what happened.
+        if (0 === $statusCode) {
+            $this->span->setStatus(SpanStatus::unknownError());
+        } else {
+            $this->span->setStatus(SpanStatus::createFromHttpStatusCode($statusCode));
+        }
+
+        $this->span = null;
     }
 }

Diff for: tests/Tracing/HttpClient/TraceableHttpClientTest.php

@@ -19,6 +19,7 @@
 use Sentry\State\Scope;
 use Sentry\Tracing\PropagationContext;
 use Sentry\Tracing\SpanId;
+use Sentry\Tracing\SpanStatus;
 use Sentry\Tracing\TraceId;
 use Sentry\Tracing\Transaction;
 use Sentry\Tracing\TransactionContext;
@@ -107,10 +108,17 @@ public function testRequest(): void
             'http.fragment' => 'baz',
         ];
 
+        // Call gc to invoke destructors at the right time.
+        unset($response);
+
+        gc_mem_caches();
+        gc_collect_cycles();
+
         $this->assertCount(2, $spans);
-        $this->assertNull($spans[1]->getEndTimestamp());
+        $this->assertNotNull($spans[1]->getEndTimestamp());
         $this->assertSame('http.client', $spans[1]->getOp());
         $this->assertSame('GET https://www.example.com/test-page', $spans[1]->getDescription());
+        $this->assertSame(SpanStatus::ok(), $spans[1]->getStatus());
         $this->assertSame($expectedTags, $spans[1]->getTags());
         $this->assertSame($expectedData, $spans[1]->getData());
     }
@@ -194,6 +202,50 @@ public function testRequestDoesContainsTracingHeadersWithoutTransaction(): void
         $this->assertSame(['baggage: sentry-trace_id=566e3688a61d4bc888951642d6f14a19,sentry-public_key=public,sentry-release=1.0.0,sentry-environment=test'], $mockResponse->getRequestOptions()['normalized_headers']['baggage']);
     }
 
+    public function testRequestSetsUnknownErrorAsSpanStatusIfResponseStatusCodeIsUnavailable(): void
+    {
+        $client = $this->createMock(ClientInterface::class);
+        $client->expects($this->once())
+            ->method('getOptions')
+            ->willReturn(new Options(['dsn' => 'http://public:[email protected]/sentry/1']));
+
+        $transaction = new Transaction(new TransactionContext());
+        $transaction->initSpanRecorder();
+
+        $this->hub->expects($this->once())
+            ->method('getSpan')
+            ->willReturn($transaction);
+
+        $this->hub->expects($this->once())
+            ->method('getClient')
+            ->willReturn($client);
+
+        $decoratedHttpClient = new MockHttpClient(new MockResponse());
+        $httpClient = new TraceableHttpClient($decoratedHttpClient, $this->hub);
+
+        // Cancelling the response is the only way that does not override in any
+        // way the status code and leave it set to 0. This is a required precondition
+        // for the span status to be set to the expected value.
+        $response = $httpClient->request('GET', 'https://www.example.com/test-page');
+        $response->cancel();
+
+        $this->assertNotNull($transaction->getSpanRecorder());
+        $this->assertInstanceOf(AbstractTraceableResponse::class, $response);
+
+        // Call gc to invoke destructors at the right time.
+        unset($response);
+
+        gc_mem_caches();
+        gc_collect_cycles();
+
+        $spans = $transaction->getSpanRecorder()->getSpans();
+
+        $this->assertCount(2, $spans);
+        $this->assertNotNull($spans[1]->getEndTimestamp());
+        $this->assertSame('GET https://www.example.com/test-page', $spans[1]->getDescription());
+        $this->assertSame(SpanStatus::unknownError(), $spans[1]->getStatus());
+    }
+
     public function testStream(): void
     {
         $transaction = new Transaction(new TransactionContext());