Fix exception in Urllib3 when dealing with filelike body. (open-telemetry#1399)

Author: isra17

CHANGELOG.md

@@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## Unreleased
 
+- Fix exception in Urllib3 when dealing with filelike body.
+  ([#1399](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1399))
+
 ### Added
 
 - Add connection attributes to sqlalchemy connect span

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

@@ -64,7 +64,9 @@ def response_hook(span, request, response):
 ---
 """
 
+import collections.abc
 import contextlib
+import io
 import typing
 from timeit import default_timer
 from typing import Collection
@@ -213,18 +215,20 @@ def instrumented_urlopen(wrapped, instance, args, kwargs):
             if callable(response_hook):
                 response_hook(span, instance, response)
 
-            request_size = 0 if body is None else len(body)
+            request_size = _get_body_size(body)
             response_size = int(response.headers.get("Content-Length", 0))
+
             metric_attributes = _create_metric_attributes(
                 instance, response, method
             )
 
             duration_histogram.record(
                 elapsed_time, attributes=metric_attributes
             )
-            request_size_histogram.record(
-                request_size, attributes=metric_attributes
-            )
+            if request_size is not None:
+                request_size_histogram.record(
+                    request_size, attributes=metric_attributes
+                )
             response_size_histogram.record(
                 response_size, attributes=metric_attributes
             )
@@ -268,6 +272,16 @@ def _get_url(
     return url
 
 
+def _get_body_size(body: object) -> typing.Optional[int]:
+    if body is None:
+        return 0
+    if isinstance(body, collections.abc.Sized):
+        return len(body)
+    if isinstance(body, io.BytesIO):
+        return body.getbuffer().nbytes
+    return None
+
+
 def _should_append_port(scheme: str, port: typing.Optional[int]) -> bool:
     if not port:
         return False

instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_metrics.py

@@ -12,8 +12,10 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import io
 from timeit import default_timer
 
+import httpretty
 import urllib3
 import urllib3.exceptions
 from urllib3.request import encode_multipart_formdata
@@ -23,181 +25,238 @@
 from opentelemetry.test.test_base import TestBase
 
 
-class TestUrllib3MetricsInstrumentation(HttpTestBase, TestBase):
+class TestURLLib3InstrumentorMetric(HttpTestBase, TestBase):
+    HTTP_URL = "http://httpbin.org/status/200"
+
     def setUp(self):
         super().setUp()
-        self.assert_ip = self.server.server_address[0]
-        self.assert_port = self.server.server_address[1]
-        self.http_host = ":".join(map(str, self.server.server_address[:2]))
-        self.http_url_base = "http://" + self.http_host
-        self.http_url = self.http_url_base + "/status/200"
-        URLLib3Instrumentor().instrument(meter_provider=self.meter_provider)
+        URLLib3Instrumentor().instrument()
+        httpretty.enable(allow_net_connect=False)
+        httpretty.register_uri(httpretty.GET, self.HTTP_URL, body="Hello!")
+        httpretty.register_uri(httpretty.POST, self.HTTP_URL, body="Hello!")
+        self.pool = urllib3.PoolManager()
 
     def tearDown(self):
         super().tearDown()
+        self.pool.clear()
         URLLib3Instrumentor().uninstrument()
 
-    # Return Sequence with one histogram
-    def create_histogram_data_points(self, sum_data_point, attributes):
-        return [
-            self.create_histogram_data_point(
-                sum_data_point, 1, sum_data_point, sum_data_point, attributes
-            )
-        ]
-
-    def test_basic_metric_check_client_size_get(self):
-        with urllib3.PoolManager() as pool:
-            start_time = default_timer()
-            response = pool.request("GET", self.http_url)
-            client_duration_estimated = (default_timer() - start_time) * 1000
-
-            metrics = self.get_sorted_metrics()
-
-            (
-                client_duration,
-                client_request_size,
-                client_response_size,
-            ) = metrics
-
-            self.assertEqual(client_duration.name, "http.client.duration")
-            self.assert_metric_expected(
-                client_duration,
-                self.create_histogram_data_points(
-                    client_duration_estimated,
+        httpretty.disable()
+        httpretty.reset()
+
+    def test_basic_metrics(self):
+        start_time = default_timer()
+        response = self.pool.request("GET", self.HTTP_URL)
+        client_duration_estimated = (default_timer() - start_time) * 1000
+
+        metrics = self.get_sorted_metrics()
+
+        (
+            client_duration,
+            client_request_size,
+            client_response_size,
+        ) = metrics
+
+        self.assertEqual(client_duration.name, "http.client.duration")
+        self.assert_metric_expected(
+            client_duration,
+            [
+                self.create_histogram_data_point(
+                    count=1,
+                    sum_data_point=client_duration_estimated,
+                    max_data_point=client_duration_estimated,
+                    min_data_point=client_duration_estimated,
                     attributes={
-                        "http.status_code": 200,
-                        "http.host": self.assert_ip,
-                        "http.method": "GET",
                         "http.flavor": "1.1",
+                        "http.host": "httpbin.org",
+                        "http.method": "GET",
                         "http.scheme": "http",
-                        "net.peer.name": self.assert_ip,
-                        "net.peer.port": self.assert_port,
+                        "http.status_code": 200,
+                        "net.peer.name": "httpbin.org",
+                        "net.peer.port": 80,
                     },
-                ),
-                est_value_delta=200,
-            )
-
-            self.assertEqual(
-                client_request_size.name, "http.client.request.size"
-            )
-            self.assert_metric_expected(
-                client_request_size,
-                self.create_histogram_data_points(
-                    0,
+                )
+            ],
+            est_value_delta=200,
+        )
+
+        self.assertEqual(client_request_size.name, "http.client.request.size")
+        self.assert_metric_expected(
+            client_request_size,
+            [
+                self.create_histogram_data_point(
+                    count=1,
+                    sum_data_point=0,
+                    max_data_point=0,
+                    min_data_point=0,
                     attributes={
-                        "http.status_code": 200,
-                        "http.host": self.assert_ip,
-                        "http.method": "GET",
                         "http.flavor": "1.1",
+                        "http.host": "httpbin.org",
+                        "http.method": "GET",
                         "http.scheme": "http",
-                        "net.peer.name": self.assert_ip,
-                        "net.peer.port": self.assert_port,
+                        "http.status_code": 200,
+                        "net.peer.name": "httpbin.org",
+                        "net.peer.port": 80,
                     },
-                ),
-            )
-
-            self.assertEqual(
-                client_response_size.name, "http.client.response.size"
-            )
-            self.assert_metric_expected(
-                client_response_size,
-                self.create_histogram_data_points(
-                    len(response.data),
+                )
+            ],
+        )
+
+        expected_size = len(response.data)
+        self.assertEqual(
+            client_response_size.name, "http.client.response.size"
+        )
+        self.assert_metric_expected(
+            client_response_size,
+            [
+                self.create_histogram_data_point(
+                    count=1,
+                    sum_data_point=expected_size,
+                    max_data_point=expected_size,
+                    min_data_point=expected_size,
                     attributes={
-                        "http.status_code": 200,
-                        "http.host": self.assert_ip,
-                        "http.method": "GET",
                         "http.flavor": "1.1",
+                        "http.host": "httpbin.org",
+                        "http.method": "GET",
                         "http.scheme": "http",
-                        "net.peer.name": self.assert_ip,
-                        "net.peer.port": self.assert_port,
+                        "http.status_code": 200,
+                        "net.peer.name": "httpbin.org",
+                        "net.peer.port": 80,
                     },
-                ),
-            )
-
-    def test_basic_metric_check_client_size_post(self):
-        with urllib3.PoolManager() as pool:
-            start_time = default_timer()
-            data_fields = {"data": "test"}
-            response = pool.request("POST", self.http_url, fields=data_fields)
-            client_duration_estimated = (default_timer() - start_time) * 1000
-            body = encode_multipart_formdata(data_fields)[0]
-
-            metrics = self.get_sorted_metrics()
-
-            (
-                client_duration,
-                client_request_size,
-                client_response_size,
-            ) = metrics
-
-            self.assertEqual(client_duration.name, "http.client.duration")
-            self.assert_metric_expected(
-                client_duration,
-                self.create_histogram_data_points(
-                    client_duration_estimated,
+                )
+            ],
+        )
+
+    def test_str_request_body_size_metrics(self):
+        self.pool.request("POST", self.HTTP_URL, body="foobar")
+
+        metrics = self.get_sorted_metrics()
+        (_, client_request_size, _) = metrics
+
+        self.assertEqual(client_request_size.name, "http.client.request.size")
+        self.assert_metric_expected(
+            client_request_size,
+            [
+                self.create_histogram_data_point(
+                    count=1,
+                    sum_data_point=6,
+                    max_data_point=6,
+                    min_data_point=6,
                     attributes={
-                        "http.status_code": 501,
-                        "http.host": self.assert_ip,
-                        "http.method": "POST",
                         "http.flavor": "1.1",
+                        "http.host": "httpbin.org",
+                        "http.method": "POST",
                         "http.scheme": "http",
-                        "net.peer.name": self.assert_ip,
-                        "net.peer.port": self.assert_port,
+                        "http.status_code": 200,
+                        "net.peer.name": "httpbin.org",
+                        "net.peer.port": 80,
                     },
-                ),
-                est_value_delta=200,
-            )
-
-            self.assertEqual(
-                client_request_size.name, "http.client.request.size"
-            )
-            client_request_size_expected = len(body)
-            self.assert_metric_expected(
-                client_request_size,
-                self.create_histogram_data_points(
-                    client_request_size_expected,
+                )
+            ],
+        )
+
+    def test_bytes_request_body_size_metrics(self):
+        self.pool.request("POST", self.HTTP_URL, body=b"foobar")
+
+        metrics = self.get_sorted_metrics()
+        (_, client_request_size, _) = metrics
+
+        self.assertEqual(client_request_size.name, "http.client.request.size")
+        self.assert_metric_expected(
+            client_request_size,
+            [
+                self.create_histogram_data_point(
+                    count=1,
+                    sum_data_point=6,
+                    max_data_point=6,
+                    min_data_point=6,
                     attributes={
-                        "http.status_code": 501,
-                        "http.host": self.assert_ip,
-                        "http.method": "POST",
                         "http.flavor": "1.1",
+                        "http.host": "httpbin.org",
+                        "http.method": "POST",
                         "http.scheme": "http",
-                        "net.peer.name": self.assert_ip,
-                        "net.peer.port": self.assert_port,
+                        "http.status_code": 200,
+                        "net.peer.name": "httpbin.org",
+                        "net.peer.port": 80,
                     },
-                ),
-            )
-
-            self.assertEqual(
-                client_response_size.name, "http.client.response.size"
-            )
-            client_response_size_expected = len(response.data)
-            self.assert_metric_expected(
-                client_response_size,
-                self.create_histogram_data_points(
-                    client_response_size_expected,
+                )
+            ],
+        )
+
+    def test_fields_request_body_size_metrics(self):
+        self.pool.request("POST", self.HTTP_URL, fields={"foo": "bar"})
+
+        metrics = self.get_sorted_metrics()
+        (_, client_request_size, _) = metrics
+
+        self.assertEqual(client_request_size.name, "http.client.request.size")
+        expected_value = len(encode_multipart_formdata({"foo": "bar"})[0])
+        self.assert_metric_expected(
+            client_request_size,
+            [
+                self.create_histogram_data_point(
+                    count=1,
+                    sum_data_point=expected_value,
+                    max_data_point=expected_value,
+                    min_data_point=expected_value,
                     attributes={
-                        "http.status_code": 501,
-                        "http.host": self.assert_ip,
+                        "http.flavor": "1.1",
+                        "http.host": "httpbin.org",
                         "http.method": "POST",
+                        "http.scheme": "http",
+                        "http.status_code": 200,
+                        "net.peer.name": "httpbin.org",
+                        "net.peer.port": 80,
+                    },
+                )
+            ],
+        )
+
+    def test_bytesio_request_body_size_metrics(self):
+        self.pool.request("POST", self.HTTP_URL, body=io.BytesIO(b"foobar"))
+
+        metrics = self.get_sorted_metrics()
+        (_, client_request_size, _) = metrics
+
+        self.assertEqual(client_request_size.name, "http.client.request.size")
+        self.assert_metric_expected(
+            client_request_size,
+            [
+                self.create_histogram_data_point(
+                    count=1,
+                    sum_data_point=6,
+                    max_data_point=6,
+                    min_data_point=6,
+                    attributes={
                         "http.flavor": "1.1",
+                        "http.host": "httpbin.org",
+                        "http.method": "POST",
                         "http.scheme": "http",
-                        "net.peer.name": self.assert_ip,
-                        "net.peer.port": self.assert_port,
+                        "http.status_code": 200,
+                        "net.peer.name": "httpbin.org",
+                        "net.peer.port": 80,
                     },
-                ),
-            )
+                )
+            ],
+        )
+
+    def test_generator_request_body_size_metrics(self):
+        self.pool.request(
+            "POST", self.HTTP_URL, body=(b for b in (b"foo", b"bar"))
+        )
+
+        metrics = self.get_sorted_metrics()
+        self.assertEqual(len(metrics), 2)
+        self.assertNotIn("http.client.request.size", [m.name for m in metrics])
 
     def test_metric_uninstrument(self):
-        with urllib3.PoolManager() as pool:
-            pool.request("GET", self.http_url)
-            URLLib3Instrumentor().uninstrument()
-            pool.request("GET", self.http_url)
+        self.pool.request("GET", self.HTTP_URL)
+        URLLib3Instrumentor().uninstrument()
+        self.pool.request("GET", self.HTTP_URL)
 
-            metrics = self.get_sorted_metrics()
-            self.assertEqual(len(metrics), 3)
+        metrics = self.get_sorted_metrics()
+        self.assertEqual(len(metrics), 3)
 
-            for metric in metrics:
-                for point in list(metric.data.data_points):
-                    self.assertEqual(point.count, 1)
+        for metric in metrics:
+            for point in list(metric.data.data_points):
+                self.assertEqual(point.count, 1)