Use instanceof to check if responses are valid Response objects (#273)

Author: ThePumpingLemma

CHANGELOG.md

@@ -37,6 +37,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   ([#227](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/227]))
 - `opentelemetry-instrumentation-botocore` Adds a field to report the number of retries it take to complete an API call
   ([#275](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/275))
+- `opentelemetry-instrumentation-requests` Use instanceof to check if responses are valid Response objects
+  ([#273](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/273))
 
 ### Changed
 - `opentelemetry-instrumentation-asgi`, `opentelemetry-instrumentation-wsgi` Return `None` for `CarrierGetter` if key not found

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

@@ -38,6 +38,7 @@
 
 from requests import Timeout, URLRequired
 from requests.exceptions import InvalidSchema, InvalidURL, MissingSchema
+from requests.models import Response
 from requests.sessions import Session
 from requests.structures import CaseInsensitiveDict
 
@@ -158,7 +159,7 @@ def _instrumented_requests_call(
                 finally:
                     context.detach(token)
 
-                if result is not None:
+                if isinstance(result, Response):
                     if span.is_recording():
                         span.set_attribute(
                             "http.status_code", result.status_code

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

@@ -28,6 +28,12 @@
 from opentelemetry.trace.status import StatusCode
 
 
+class InvalidResponseObjectException(Exception):
+    def __init__(self):
+        super().__init__()
+        self.response = {}
+
+
 class RequestsIntegrationTestBase(abc.ABC):
     # pylint: disable=no-member
 
@@ -307,6 +313,42 @@ def test_requests_exception_without_response(self, *_, **__):
     mocked_response.status_code = 500
     mocked_response.reason = "Internal Server Error"
 
+    @mock.patch(
+        "requests.adapters.HTTPAdapter.send",
+        side_effect=InvalidResponseObjectException,
+    )
+    def test_requests_exception_without_proper_response_type(self, *_, **__):
+        with self.assertRaises(InvalidResponseObjectException):
+            self.perform_request(self.URL)
+
+        span = self.assert_span()
+        self.assertEqual(
+            span.attributes,
+            {"component": "http", "http.method": "GET", "http.url": self.URL},
+        )
+        self.assertEqual(span.status.status_code, StatusCode.ERROR)
+
+        self.assertIsNotNone(RequestsInstrumentor().meter)
+        self.assertEqual(len(RequestsInstrumentor().meter.instruments), 1)
+        recorder = list(RequestsInstrumentor().meter.instruments.values())[0]
+        match_key = get_dict_as_key(
+            {
+                "http.method": "GET",
+                "http.url": "http://httpbin.org/status/200",
+            }
+        )
+        for key in recorder.bound_instruments.keys():
+            self.assertEqual(key, match_key)
+            # pylint: disable=protected-access
+            bound = recorder.bound_instruments.get(key)
+            for view_data in bound.view_datas:
+                self.assertEqual(view_data.labels, key)
+                self.assertEqual(view_data.aggregator.current.count, 1)
+
+    mocked_response = requests.Response()
+    mocked_response.status_code = 500
+    mocked_response.reason = "Internal Server Error"
+
     @mock.patch(
         "requests.adapters.HTTPAdapter.send",
         side_effect=requests.RequestException(response=mocked_response),