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

CHANGELOG.md

@@ -63,6 +63,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))
 
 ## Version 1.24.0/0.45b0 (2024-03-28)
 

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

@@ -109,13 +109,14 @@ def _finish_tracing_callback(
     description = None
     exc = future.exception()
 
-    response = future.result()
-
     if span.is_recording() and exc:
         if isinstance(exc, HTTPError):
             status_code = exc.code
         description = f"{type(exc).__name__}: {exc}"
-    else:
+
+    response = None
+    if not exc:
+        response = future.result()
         status_code = response.code
 
     if status_code is not None:
@@ -127,15 +128,20 @@ def _finish_tracing_callback(
             )
         )
 
-    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

@@ -14,8 +14,10 @@
 
 
 from unittest.mock import Mock, patch
+from urllib.error import HTTPError
 
 from http_server_mock import HttpServerMock
+from tornado.httpclient import HTTPClientError
 from tornado.testing import AsyncHTTPTestCase
 
 from opentelemetry import trace
@@ -602,6 +604,50 @@ 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.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.memory_exporter.clear()
+        print("server span", server)
+
+        # when an exception is thrown
+        try:
+            response = self.fetch("/some-404", raise_error=True)
+            self.assertEqual(response.code, 404)
+        except HTTPClientError:
+            pass
+        except Exception as e:
+            print("Exception type", type(e))
+            self.fail(f"Unexpected exception: {e}")
+
+        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.memory_exporter.clear()
+
+
 class TestTornadoUninstrument(TornadoTest):
     def test_uninstrument(self):
         response = self.fetch("/")

resource/opentelemetry-resource-detector-azure/src/opentelemetry/resource/detector/azure/app_service.py

@@ -12,23 +12,21 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from typing import Optional
 from os import environ
+from typing import Optional
 
+from opentelemetry.resource.detector.azure._utils import (
+    _get_azure_resource_uri,
+    _is_on_functions,
+)
 from opentelemetry.sdk.resources import Resource, ResourceDetector
 from opentelemetry.semconv.resource import (
     CloudPlatformValues,
     CloudProviderValues,
     ResourceAttributes,
 )
-from opentelemetry.resource.detector.azure._utils import _get_azure_resource_uri
-
-from ._constants import (
-    _APP_SERVICE_ATTRIBUTE_ENV_VARS,
-    _WEBSITE_SITE_NAME,
-)
 
-from opentelemetry.resource.detector.azure._utils import _is_on_functions
+from ._constants import _APP_SERVICE_ATTRIBUTE_ENV_VARS, _WEBSITE_SITE_NAME
 
 
 class AzureAppServiceResourceDetector(ResourceDetector):

resource/opentelemetry-resource-detector-azure/src/opentelemetry/resource/detector/azure/functions.py

@@ -14,6 +14,10 @@
 
 from os import environ, getpid
 
+from opentelemetry.resource.detector.azure._utils import (
+    _get_azure_resource_uri,
+    _is_on_functions,
+)
 from opentelemetry.sdk.resources import Resource, ResourceDetector
 from opentelemetry.semconv.resource import (
     CloudPlatformValues,
@@ -26,10 +30,6 @@
     _REGION_NAME,
     _WEBSITE_SITE_NAME,
 )
-from opentelemetry.resource.detector.azure._utils import (
-    _get_azure_resource_uri,
-    _is_on_functions,
-)
 
 
 class AzureFunctionsResourceDetector(ResourceDetector):
@@ -65,4 +65,3 @@ def detect(self) -> Resource:
                     attributes[key] = value
 
         return Resource(attributes)
-

resource/opentelemetry-resource-detector-azure/tests/test_app_service.py

@@ -68,7 +68,7 @@ def test_on_app_service(self):
         self.assertEqual(
             attributes["azure.app.service.stamp"], TEST_WEBSITE_HOME_STAMPNAME
         )
-    
+
     @patch.dict(
         "os.environ",
         {

util/opentelemetry-util-http/tests/test_remove_credentials.py

@@ -19,9 +19,13 @@ def test_remove_credentials(self):
     def test_remove_credentials_ipv4_literal(self):
         url = "http://someuser:[email protected]:8080/test/path?query=value"
         cleaned_url = remove_url_credentials(url)
-        self.assertEqual(cleaned_url, "http://127.0.0.1:8080/test/path?query=value")
+        self.assertEqual(
+            cleaned_url, "http://127.0.0.1:8080/test/path?query=value"
+        )
 
     def test_remove_credentials_ipv6_literal(self):
         url = "http://someuser:somepass@[::1]:8080/test/path?query=value"
         cleaned_url = remove_url_credentials(url)
-        self.assertEqual(cleaned_url, "http://[::1]:8080/test/path?query=value")
+        self.assertEqual(
+            cleaned_url, "http://[::1]:8080/test/path?query=value"
+        )