fix(tornado): ensure reading future.result() won't throw an exception in client.py _finish_tracing_callback

CHANGELOG.md

@@ -80,6 +80,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `opentelemetry-instrumentation-requests` Fix wrong time unit for duration histogram
   ([#2553](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2553))
 - `opentelemetry-util-http` Preserve brackets around literal IPv6 hosts ([#2552](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2552))
+- `opentelemetry-instrumentation-tornado` Fix not handling a http client exception
+  ([#2563](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2563))
 - `opentelemetry-util-redis` Fix net peer attribute for unix socket connection ([#2493](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2493))
 
 ## Version 1.24.0/0.45b0 (2024-03-28)

instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/client.py

@@ -21,7 +21,7 @@
 from opentelemetry.instrumentation.utils import http_status_to_status_code
 from opentelemetry.propagate import inject
 from opentelemetry.semconv.trace import SpanAttributes
-from opentelemetry.trace.status import Status
+from opentelemetry.trace.status import Status, StatusCode
 from opentelemetry.util.http import remove_url_credentials
 
 
@@ -105,37 +105,52 @@ def _finish_tracing_callback(
     request_size_histogram,
     response_size_histogram,
 ):
+    response = None
     status_code = None
+    status = None
     description = None
-    exc = future.exception()
-
-    response = future.result()
 
-    if span.is_recording() and exc:
-        if isinstance(exc, HTTPError):
-            status_code = exc.code
+    exc = future.exception()
+    if exc:
         description = f"{type(exc).__name__}: {exc}"
+        if isinstance(exc, HTTPError):
+            response = exc.response
+            status_code = response.code
+            status = Status(
+                status_code=http_status_to_status_code(status_code),
+                description=description,
+            )
+        else:
+            status = Status(
+                status_code=StatusCode.ERROR,
+                description=description,
+            )
     else:
+        response = future.result()
         status_code = response.code
+        status = Status(
+            status_code=http_status_to_status_code(status_code),
+            description=description,
+        )
 
     if status_code is not None:
         span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code)
-        span.set_status(
-            Status(
-                status_code=http_status_to_status_code(status_code),
-                description=description,
-            )
-        )
+    span.set_status(status)
 
-    metric_attributes = _create_metric_attributes(response)
-    request_size = int(response.request.headers.get("Content-Length", 0))
-    response_size = int(response.headers.get("Content-Length", 0))
+    if response is not None:
+        metric_attributes = _create_metric_attributes(response)
+        request_size = int(response.request.headers.get("Content-Length", 0))
+        response_size = int(response.headers.get("Content-Length", 0))
 
-    duration_histogram.record(
-        response.request_time, attributes=metric_attributes
-    )
-    request_size_histogram.record(request_size, attributes=metric_attributes)
-    response_size_histogram.record(response_size, attributes=metric_attributes)
+        duration_histogram.record(
+            response.request_time, attributes=metric_attributes
+        )
+        request_size_histogram.record(
+            request_size, attributes=metric_attributes
+        )
+        response_size_histogram.record(
+            response_size, attributes=metric_attributes
+        )
 
     if response_hook:
         response_hook(span, future)

instrumentation/opentelemetry-instrumentation-tornado/tests/test_instrumentation.py

@@ -16,6 +16,7 @@
 from unittest.mock import Mock, patch
 
 from http_server_mock import HttpServerMock
+from tornado.httpclient import HTTPClientError
 from tornado.testing import AsyncHTTPTestCase
 
 from opentelemetry import trace
@@ -32,7 +33,7 @@
 from opentelemetry.semconv.trace import SpanAttributes
 from opentelemetry.test.test_base import TestBase
 from opentelemetry.test.wsgitestutil import WsgiTestBase
-from opentelemetry.trace import SpanKind
+from opentelemetry.trace import SpanKind, StatusCode
 from opentelemetry.util.http import (
     OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST,
     OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE,
@@ -493,7 +494,6 @@ def test_response_headers(self):
         self.assertEqual(len(spans), 3)
         self.assertTraceResponseHeaderMatchesSpan(response.headers, spans[1])
 
-        self.memory_exporter.clear()
         set_global_response_propagator(orig)
 
     def test_credential_removal(self):
@@ -602,6 +602,49 @@ def client_response_hook(span, response):
         self.memory_exporter.clear()
 
 
+class TestTornadoHTTPClientInstrumentation(TornadoTest, WsgiTestBase):
+    def test_http_client_success_response(self):
+        response = self.fetch("/")
+        self.assertEqual(response.code, 201)
+
+        spans = self.memory_exporter.get_finished_spans()
+        self.assertEqual(len(spans), 3)
+        manual, server, client = self.sorted_spans(spans)
+        self.assertEqual(manual.name, "manual")
+        self.assertEqual(server.name, "GET /")
+        self.assertEqual(client.name, "GET")
+        self.assertEqual(client.status.status_code, StatusCode.UNSET)
+        self.memory_exporter.clear()
+
+    def test_http_client_failed_response(self):
+        # when an exception isn't thrown
+        response = self.fetch("/some-404")
+        self.assertEqual(response.code, 404)
+
+        spans = self.memory_exporter.get_finished_spans()
+        self.assertEqual(len(spans), 2)
+        server, client = self.sorted_spans(spans)
+        self.assertEqual(server.name, "GET /some-404")
+        self.assertEqual(client.name, "GET")
+        self.assertEqual(client.status.status_code, StatusCode.ERROR)
+        self.memory_exporter.clear()
+
+        # when an exception is thrown
+        try:
+            response = self.fetch("/some-404", raise_error=True)
+            self.assertEqual(response.code, 404)
+        except HTTPClientError:
+            pass  # expected exception - continue
+
+        spans = self.memory_exporter.get_finished_spans()
+        self.assertEqual(len(spans), 2)
+        server, client = self.sorted_spans(spans)
+        self.assertEqual(server.name, "GET /some-404")
+        self.assertEqual(client.name, "GET")
+        self.assertEqual(client.status.status_code, StatusCode.ERROR)
+        self.memory_exporter.clear()
+
+
 class TestTornadoUninstrument(TornadoTest):
     def test_uninstrument(self):
         response = self.fetch("/")