From 75a29c24b492735dd99c05ae208329abbe167d76 Mon Sep 17 00:00:00 2001 From: isra17 Date: Sun, 23 Oct 2022 13:27:52 -0400 Subject: [PATCH] Fix exception in Urllib3 when dealing with filelike body. --- CHANGELOG.md | 2 + .../pyproject.toml | 1 + .../instrumentation/urllib3/__init__.py | 22 ++- .../tests/test_urllib3_integration.py | 1 + .../tests/test_urllib3_ip_support.py | 174 +++++++++--------- 5 files changed, 108 insertions(+), 92 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 58c9c75d01..2cc08cf483 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/instrumentation/opentelemetry-instrumentation-urllib3/pyproject.toml b/instrumentation/opentelemetry-instrumentation-urllib3/pyproject.toml index 1ac208d89a..98c06f8fce 100644 --- a/instrumentation/opentelemetry-instrumentation-urllib3/pyproject.toml +++ b/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] diff --git a/instrumentation/opentelemetry-instrumentation-urllib3/src/opentelemetry/instrumentation/urllib3/__init__.py b/instrumentation/opentelemetry-instrumentation-urllib3/src/opentelemetry/instrumentation/urllib3/__init__.py index 59d7cd35bd..f9085606da 100644 --- a/instrumentation/opentelemetry-instrumentation-urllib3/src/opentelemetry/instrumentation/urllib3/__init__.py +++ b/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,8 +214,9 @@ 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 ) @@ -221,9 +224,10 @@ def instrumented_urlopen(wrapped, instance, args, kwargs): 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 diff --git a/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_integration.py b/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_integration.py index 2e70a9d2ab..b4384e1bcd 100644 --- a/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_integration.py +++ b/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 diff --git a/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_ip_support.py b/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_ip_support.py index d47cd8f1ea..a495befd45 100644 --- a/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_ip_support.py +++ b/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)