Fix exception in Urllib3 when dealing with filelike body.

Author: isra17

CHANGELOG.md

@@ -10,6 +10,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   ([#1252](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1252))
 - Fix bug in Falcon instrumentation
   ([#1377](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1377))
+- Fix exception in Urllib3 when dealing with filelike body.
+  ([#1399](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1399))
 
 
 ### Added

instrumentation/opentelemetry-instrumentation-urllib3/pyproject.toml

@@ -39,6 +39,7 @@ test = [
   "opentelemetry-instrumentation-urllib3[instruments]",
   "httpretty ~= 1.0",
   "opentelemetry-test-utils == 0.34b0",
+  "parameterized ~= 0.8",
 ]
 
 [project.entry-points.opentelemetry_instrumentor]

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
@@ -212,18 +214,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
             )
@@ -267,6 +271,16 @@ def _get_url(
     return url
 
 
+def _get_body_size(body: object) -> typing.Optional[int]:
+    if body is None:
+        return 0
+    elif isinstance(body, collections.abc.Sized):
+        return len(body)
+    elif 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_integration.py

@@ -13,6 +13,7 @@
 # limitations under the License.
 import json
 import typing
+from io import BytesIO
 from unittest import mock
 
 import httpretty

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

@@ -12,10 +12,12 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import io
 from timeit import default_timer
 
 import urllib3
 import urllib3.exceptions
+from parameterized import parameterized
 from urllib3.request import encode_multipart_formdata
 
 from opentelemetry import trace
@@ -116,107 +118,103 @@ def test_metric_uninstrument(self):
                         for point in list(metric.data.data_points):
                             self.assertEqual(point.count, 1)
 
+    def assert_metrics(
+        self,
+        *,
+        expected_data,
+        client_duration_estimated,
+        expected_attributes={}
+    ):
+        expected_attributes = {
+            "http.status_code": 200,
+            "http.host": self.assert_ip,
+            "http.method": "GET",
+            "http.flavor": "1.1",
+            "http.scheme": "http",
+            "net.peer.name": self.assert_ip,
+            "net.peer.port": self.assert_port,
+            **expected_attributes,
+        }
+
+        expected_metrics = [
+            "http.client.duration",
+        ] + list(expected_data.keys())
+
+        resource_metrics = (
+            self.memory_metrics_reader.get_metrics_data().resource_metrics
+        )
+        for metrics in resource_metrics:
+            for scope_metrics in metrics.scope_metrics:
+                self.assertEqual(
+                    len(scope_metrics.metrics), len(expected_metrics)
+                )
+                for metric in scope_metrics.metrics:
+                    for data_point in metric.data.data_points:
+                        if metric.name in expected_data:
+                            self.assertEqual(
+                                data_point.sum, expected_data[metric.name]
+                            )
+                        if metric.name == "http.client.duration":
+                            self.assertAlmostEqual(
+                                data_point.sum,
+                                client_duration_estimated,
+                                delta=1000,
+                            )
+                        self.assertIn(metric.name, expected_metrics)
+                        self.assertDictEqual(
+                            expected_attributes,
+                            dict(data_point.attributes),
+                        )
+                        self.assertEqual(data_point.count, 1)
+
     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
 
-            expected_attributes = {
-                "http.status_code": 200,
-                "http.host": self.assert_ip,
-                "http.method": "GET",
-                "http.flavor": "1.1",
-                "http.scheme": "http",
-                "net.peer.name": self.assert_ip,
-                "net.peer.port": self.assert_port,
-            }
-            expected_data = {
-                "http.client.request.size": 0,
-                "http.client.response.size": len(response.data),
-            }
-            expected_metrics = [
-                "http.client.duration",
-                "http.client.request.size",
-                "http.client.response.size",
-            ]
-
-            resource_metrics = (
-                self.memory_metrics_reader.get_metrics_data().resource_metrics
+            self.assert_metrics(
+                expected_data={
+                    "http.client.request.size": 0,
+                    "http.client.response.size": len(response.data),
+                },
+                client_duration_estimated=client_duration_estimated,
             )
-            for metrics in resource_metrics:
-                for scope_metrics in metrics.scope_metrics:
-                    self.assertEqual(len(scope_metrics.metrics), 3)
-                    for metric in scope_metrics.metrics:
-                        for data_point in metric.data.data_points:
-                            if metric.name in expected_data:
-                                self.assertEqual(
-                                    data_point.sum, expected_data[metric.name]
-                                )
-                            if metric.name == "http.client.duration":
-                                self.assertAlmostEqual(
-                                    data_point.sum,
-                                    client_duration_estimated,
-                                    delta=1000,
-                                )
-                            self.assertIn(metric.name, expected_metrics)
-                            self.assertDictEqual(
-                                expected_attributes,
-                                dict(data_point.attributes),
-                            )
-                            self.assertEqual(data_point.count, 1)
 
-    def test_basic_metric_check_client_size_post(self):
+    @parameterized.expand(
+        [
+            ({"body": "foobar"}, {"http.client.request.size": 6}),
+            ({"body": b"foobar"}, {"http.client.request.size": 6}),
+            ({}, {"http.client.request.size": 0}),
+            ({"body": io.BytesIO(b"foobar")}, {"http.client.request.size": 6}),
+            ({"body": io.StringIO("foobar")}, {}),
+            ({"body": (s for s in [b"foo", b"bar"])}, {}),
+            (
+                {"fields": {"data": "test"}},
+                {
+                    "http.client.request.size": len(
+                        encode_multipart_formdata({"data": "test"})[0]
+                    )
+                },
+            ),
+        ]
+    )
+    def test_basic_metric_data(self, kwargs, expected_data):
         with urllib3.PoolManager() as pool:
             start_time = default_timer()
-            data_fields = {"data": "test"}
-            response = pool.request("POST", self.http_url, fields=data_fields)
+            response = pool.request("POST", self.http_url, **kwargs)
             client_duration_estimated = (default_timer() - start_time) * 1000
 
-            expected_attributes = {
-                "http.status_code": 501,
-                "http.host": self.assert_ip,
-                "http.method": "POST",
-                "http.flavor": "1.1",
-                "http.scheme": "http",
-                "net.peer.name": self.assert_ip,
-                "net.peer.port": self.assert_port,
-            }
-
-            body = encode_multipart_formdata(data_fields)[0]
-
             expected_data = {
-                "http.client.request.size": len(body),
                 "http.client.response.size": len(response.data),
+                **expected_data,
             }
-            expected_metrics = [
-                "http.client.duration",
-                "http.client.request.size",
-                "http.client.response.size",
-            ]
-
-            resource_metrics = (
-                self.memory_metrics_reader.get_metrics_data().resource_metrics
+
+            self.assert_metrics(
+                expected_data=expected_data,
+                expected_attributes={
+                    "http.status_code": 501,
+                    "http.method": "POST",
+                },
+                client_duration_estimated=client_duration_estimated,
             )
-            for metrics in resource_metrics:
-                for scope_metrics in metrics.scope_metrics:
-                    self.assertEqual(len(scope_metrics.metrics), 3)
-                    for metric in scope_metrics.metrics:
-                        for data_point in metric.data.data_points:
-                            if metric.name in expected_data:
-                                self.assertEqual(
-                                    data_point.sum, expected_data[metric.name]
-                                )
-                            if metric.name == "http.client.duration":
-                                self.assertAlmostEqual(
-                                    data_point.sum,
-                                    client_duration_estimated,
-                                    delta=1000,
-                                )
-                            self.assertIn(metric.name, expected_metrics)
-
-                            self.assertDictEqual(
-                                expected_attributes,
-                                dict(data_point.attributes),
-                            )
-                            self.assertEqual(data_point.count, 1)