fix(Instrumentation/TraceableHttpClient): span finishes too early (#105)

* fix(TraceableHttpClient): span finishes too early Signed-off-by: Plakhotnikov Vladimir <v.plahotnikov@yclients.tech> * fix(TraceableHttpClient): remove unnecessary code from catch block Signed-off-by: Plakhotnikov Vladimir <v.plahotnikov@yclients.tech> --------- Signed-off-by: Plakhotnikov Vladimir <v.plahotnikov@yclients.tech>
FriendsOfOpenTelemetry · Dec 22, 2024 · ea237a0 · ea237a0
1 parent b50c27a
commit ea237a0
Show file tree

Hide file tree

Showing 2 changed files with 20 additions and 31 deletions.
diff --git a/src/Instrumentation/Symfony/HttpClient/TraceableHttpClient.php b/src/Instrumentation/Symfony/HttpClient/TraceableHttpClient.php
@@ -3,16 +3,13 @@
 namespace FriendsOfOpenTelemetry\OpenTelemetryBundle\Instrumentation\Symfony\HttpClient;
 
 use Nyholm\Psr7\Uri;
-use OpenTelemetry\API\Trace\SpanInterface;
 use OpenTelemetry\API\Trace\SpanKind;
-use OpenTelemetry\API\Trace\StatusCode;
 use OpenTelemetry\API\Trace\TracerInterface;
 use OpenTelemetry\Context\Context;
 use OpenTelemetry\SemConv\TraceAttributes;
 use Psr\Log\LoggerAwareInterface;
 use Psr\Log\LoggerInterface;
 use Symfony\Component\HttpClient\Response\ResponseStream;
-use Symfony\Contracts\HttpClient\Exception\ExceptionInterface;
 use Symfony\Contracts\HttpClient\HttpClientInterface;
 use Symfony\Contracts\HttpClient\ResponseInterface;
 use Symfony\Contracts\HttpClient\ResponseStreamInterface;
@@ -38,38 +35,26 @@ public function request(string $method, string $url, array $options = []): Respo
         } else {
             $this->logger?->debug('No active scope');
         }
-        $span = null;
 
-        try {
-            $uri = new Uri($url);
+        $uri = new Uri($url);
 
-            $spanBuilder = $this->tracer
-                ->spanBuilder('http.client')
-                ->setSpanKind(SpanKind::KIND_CLIENT)
-                ->setParent($scope?->context())
-                ->setAttribute(TraceAttributes::URL_FULL, $url)
-                ->setAttribute(TraceAttributes::URL_SCHEME, $uri->getScheme())
-                ->setAttribute(TraceAttributes::URL_PATH, $uri->getPath())
-                ->setAttribute(TraceAttributes::URL_QUERY, $uri->getQuery())
-                ->setAttribute(TraceAttributes::URL_FRAGMENT, $uri->getFragment())
-                ->setAttribute(TraceAttributes::HTTP_REQUEST_METHOD, $method)
-            ;
+        $spanBuilder = $this->tracer
+            ->spanBuilder('http.client')
+            ->setSpanKind(SpanKind::KIND_CLIENT)
+            ->setParent($scope?->context())
+            ->setAttribute(TraceAttributes::URL_FULL, $url)
+            ->setAttribute(TraceAttributes::URL_SCHEME, $uri->getScheme())
+            ->setAttribute(TraceAttributes::URL_PATH, $uri->getPath())
+            ->setAttribute(TraceAttributes::URL_QUERY, $uri->getQuery())
+            ->setAttribute(TraceAttributes::URL_FRAGMENT, $uri->getFragment())
+            ->setAttribute(TraceAttributes::HTTP_REQUEST_METHOD, $method)
+        ;
 
-            $span = $spanBuilder->startSpan();
+        $span = $spanBuilder->startSpan();
 
-            $this->logger?->debug(sprintf('Starting span "%s"', $span->getContext()->getSpanId()));
+        $this->logger?->debug(sprintf('Starting span "%s"', $span->getContext()->getSpanId()));
 
-            return new TraceableResponse($this->client, $this->client->request($method, $url, $options), $span);
-        } catch (ExceptionInterface $exception) {
-            $span->recordException($exception, [TraceAttributes::EXCEPTION_ESCAPED => true]);
-            $span->setStatus(StatusCode::STATUS_ERROR, $exception->getMessage());
-            throw $exception;
-        } finally {
-            if ($span instanceof SpanInterface) {
-                $this->logger?->debug(sprintf('Ending span "%s"', $span->getContext()->getSpanId()));
-                $span->end();
-            }
-        }
+        return new TraceableResponse($this->client, $this->client->request($method, $url, $options), $span);
     }
 
     public function stream(iterable|ResponseInterface $responses, ?float $timeout = null): ResponseStreamInterface

diff --git a/tests/Functional/Instrumentation/HttpClientTracingTest.php b/tests/Functional/Instrumentation/HttpClientTracingTest.php
@@ -51,6 +51,7 @@ public function testOk(): void
             'url.query' => '',
             'url.fragment' => '',
             'http.request.method' => 'GET',
+            'http.response.status_code' => 200,
         ]);
         self::assertSpanEventsCount($mainSpan, 0);
     }
@@ -82,14 +83,15 @@ public function testFailure(): void
 
         $mainSpan = self::getSpans()[0];
         self::assertSpanName($mainSpan, 'http.client');
-        self::assertSpanStatus($mainSpan, StatusData::unset());
+        self::assertSpanStatus($mainSpan, StatusData::error());
         self::assertSpanAttributes($mainSpan, [
             'url.full' => 'http://localhost/failure',
             'url.scheme' => 'http',
             'url.path' => '/failure',
             'url.query' => '',
             'url.fragment' => '',
             'http.request.method' => 'GET',
+            'http.response.status_code' => 500,
         ]);
         self::assertSpanEventsCount($mainSpan, 0);
     }
@@ -124,6 +126,7 @@ public function testException(): void
             'url.query' => '',
             'url.fragment' => '',
             'http.request.method' => 'GET',
+            'http.response.status_code' => 200,
         ]);
         self::assertSpanEventsCount($mainSpan, 0);
     }
@@ -179,6 +182,7 @@ public function testStream(): void
             'url.query' => '',
             'url.fragment' => '',
             'http.request.method' => 'GET',
+            'http.response.status_code' => 200,
         ]);
         self::assertSpanEventsCount($mainSpan, 0);
     }