requests: better exception handling (#765)

Canonical codes for different types of exceptions

Span attributes extracted from exception

Correct span closing for requests with raised errors (A span wasn't closed due to a RequestException)

Co-authored-by: alrex <[email protected]>

ext/opentelemetry-ext-requests/src/opentelemetry/ext/requests/__init__.py

@@ -45,12 +45,14 @@
 import types
 from urllib.parse import urlparse
 
+from requests import Timeout, URLRequired
+from requests.exceptions import InvalidSchema, InvalidURL, MissingSchema
 from requests.sessions import Session
 
 from opentelemetry import context, propagators, trace
 from opentelemetry.auto_instrumentation.instrumentor import BaseInstrumentor
 from opentelemetry.ext.requests.version import __version__
-from opentelemetry.trace import SpanKind, get_tracer
+from opentelemetry.trace import SpanKind
 from opentelemetry.trace.status import Status, StatusCanonicalCode
 
 
@@ -80,31 +82,49 @@ def instrumented_request(self, method, url, *args, **kwargs):
         # https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/http.md#http-client
         try:
             parsed_url = urlparse(url)
+            span_name = parsed_url.path
         except ValueError as exc:  # Invalid URL
-            path = "<Unparsable URL: {}>".format(exc)
-        else:
-            if parsed_url is None:
-                path = "<URL parses to None>"
-            path = parsed_url.path
+            span_name = "<Unparsable URL: {}>".format(exc)
 
-        with tracer.start_as_current_span(path, kind=SpanKind.CLIENT) as span:
+        exception = None
+
+        with tracer.start_as_current_span(
+            span_name, kind=SpanKind.CLIENT
+        ) as span:
             span.set_attribute("component", "http")
             span.set_attribute("http.method", method.upper())
             span.set_attribute("http.url", url)
 
             headers = kwargs.setdefault("headers", {})
             propagators.inject(type(headers).__setitem__, headers)
-            result = wrapped(self, method, url, *args, **kwargs)  # *** PROCEED
 
-            span.set_attribute("http.status_code", result.status_code)
-            span.set_attribute("http.status_text", result.reason)
-            span.set_status(
-                Status(_http_status_to_canonical_code(result.status_code))
-            )
+            try:
+                result = wrapped(
+                    self, method, url, *args, **kwargs
+                )  # *** PROCEED
+            except Exception as exc:  # pylint: disable=W0703
+                exception = exc
+                result = getattr(exc, "response", None)
+
+            if exception is not None:
+                span.set_status(
+                    Status(_exception_to_canonical_code(exception))
+                )
+
+            if result is not None:
+                span.set_attribute("http.status_code", result.status_code)
+                span.set_attribute("http.status_text", result.reason)
+                span.set_status(
+                    Status(_http_status_to_canonical_code(result.status_code))
+                )
+
             if span_callback is not None:
                 span_callback(span, result)
 
-            return result
+        if exception is not None:
+            raise exception.with_traceback(exception.__traceback__)
+
+        return result
 
     instrumented_request.opentelemetry_ext_requests_applied = True
 
@@ -157,6 +177,17 @@ def _http_status_to_canonical_code(code: int, allow_redirect: bool = True):
     return StatusCanonicalCode.UNKNOWN
 
 
+def _exception_to_canonical_code(exc: Exception) -> StatusCanonicalCode:
+    if isinstance(
+        exc,
+        (InvalidURL, InvalidSchema, MissingSchema, URLRequired, ValueError),
+    ):
+        return StatusCanonicalCode.INVALID_ARGUMENT
+    if isinstance(exc, Timeout):
+        return StatusCanonicalCode.DEADLINE_EXCEEDED
+    return StatusCanonicalCode.UNKNOWN
+
+
 class RequestsInstrumentor(BaseInstrumentor):
     """An instrumentor for requests
     See `BaseInstrumentor`

ext/opentelemetry-ext-requests/tests/test_requests_integration.py

@@ -12,18 +12,18 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import sys
+from unittest import mock
 
 import httpretty
 import requests
-import urllib3
 
 import opentelemetry.ext.requests
 from opentelemetry import context, propagators, trace
 from opentelemetry.ext.requests import RequestsInstrumentor
 from opentelemetry.sdk import resources
 from opentelemetry.test.mock_httptextformat import MockHTTPTextFormat
 from opentelemetry.test.test_base import TestBase
+from opentelemetry.trace.status import StatusCanonicalCode
 
 
 class TestRequestsIntegration(TestBase):
@@ -92,13 +92,8 @@ def test_not_foundbasic(self):
 
     def test_invalid_url(self):
         url = "http://[::1/nope"
-        exception_type = requests.exceptions.InvalidURL
-        if sys.version_info[:2] < (3, 5) and tuple(
-            map(int, urllib3.__version__.split(".")[:2])
-        ) < (1, 25):
-            exception_type = ValueError
 
-        with self.assertRaises(exception_type):
+        with self.assertRaises(ValueError):
             requests.post(url)
 
         span_list = self.memory_exporter.get_finished_spans()
@@ -110,6 +105,9 @@ def test_invalid_url(self):
             span.attributes,
             {"component": "http", "http.method": "POST", "http.url": url},
         )
+        self.assertEqual(
+            span.status.canonical_code, StatusCanonicalCode.INVALID_ARGUMENT
+        )
 
     def test_uninstrument(self):
         RequestsInstrumentor().uninstrument()
@@ -229,3 +227,75 @@ def test_custom_tracer_provider(self):
         span = span_list[0]
 
         self.assertIs(span.resource, resource)
+
+    @mock.patch("requests.Session.send", side_effect=requests.RequestException)
+    def test_requests_exception_without_response(self, *_, **__):
+
+        with self.assertRaises(requests.RequestException):
+            requests.get(self.URL)
+
+        span_list = self.memory_exporter.get_finished_spans()
+        self.assertEqual(len(span_list), 1)
+        span = span_list[0]
+        self.assertEqual(
+            span.attributes,
+            {"component": "http", "http.method": "GET", "http.url": self.URL},
+        )
+        self.assertEqual(
+            span.status.canonical_code, StatusCanonicalCode.UNKNOWN
+        )
+
+    mocked_response = requests.Response()
+    mocked_response.status_code = 500
+    mocked_response.reason = "Internal Server Error"
+
+    @mock.patch(
+        "requests.Session.send",
+        side_effect=requests.RequestException(response=mocked_response),
+    )
+    def test_requests_exception_with_response(self, *_, **__):
+
+        with self.assertRaises(requests.RequestException):
+            requests.get(self.URL)
+
+        span_list = self.memory_exporter.get_finished_spans()
+        self.assertEqual(len(span_list), 1)
+        span = span_list[0]
+        self.assertEqual(
+            span.attributes,
+            {
+                "component": "http",
+                "http.method": "GET",
+                "http.url": self.URL,
+                "http.status_code": 500,
+                "http.status_text": "Internal Server Error",
+            },
+        )
+        self.assertEqual(
+            span.status.canonical_code, StatusCanonicalCode.INTERNAL
+        )
+
+    @mock.patch("requests.Session.send", side_effect=Exception)
+    def test_requests_basic_exception(self, *_, **__):
+
+        with self.assertRaises(Exception):
+            requests.get(self.URL)
+
+        span_list = self.memory_exporter.get_finished_spans()
+        self.assertEqual(len(span_list), 1)
+        self.assertEqual(
+            span_list[0].status.canonical_code, StatusCanonicalCode.UNKNOWN
+        )
+
+    @mock.patch("requests.Session.send", side_effect=requests.Timeout)
+    def test_requests_timeout_exception(self, *_, **__):
+
+        with self.assertRaises(Exception):
+            requests.get(self.URL)
+
+        span_list = self.memory_exporter.get_finished_spans()
+        self.assertEqual(len(span_list), 1)
+        self.assertEqual(
+            span_list[0].status.canonical_code,
+            StatusCanonicalCode.DEADLINE_EXCEEDED,
+        )