From 9f9f01246c357f76e4b9ceb6d3806b373d4d190b Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Mon, 21 Oct 2024 15:40:33 +0200 Subject: [PATCH 1/5] opentelemetry-instrumentation: add unwrapping from dotted paths strings Make it possible to express object to unwrap as dotted module paths strings. This helps in avoiding side effects or race conditions with other instrumentations if we do importing too early. While at it add tests also for current functionality. --- .../opentelemetry/instrumentation/utils.py | 20 ++++- .../tests/test_utils.py | 80 +++++++++++++++++++ 2 files changed, 99 insertions(+), 1 deletion(-) diff --git a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py index 73c000ee9c..5e31109cf0 100644 --- a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py +++ b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py @@ -14,6 +14,7 @@ import urllib.parse from contextlib import contextmanager +from importlib import import_module from re import escape, sub from typing import Dict, Iterable, Sequence @@ -83,10 +84,27 @@ def http_status_to_status_code( def unwrap(obj, attr: str): """Given a function that was wrapped by wrapt.wrap_function_wrapper, unwrap it + The object containing the function to unwrap may be passed as dotted module path string. + Args: - obj: Object that holds a reference to the wrapped function + obj: Object that holds a reference to the wrapped function or dotted import path as string attr (str): Name of the wrapped function """ + if isinstance(obj, str): + try: + module_path, class_name = obj.rsplit(".", 1) + except ValueError as exc: + raise ImportError( + f"Cannot parse '{obj}' as dotted import path" + ) from exc + module = import_module(module_path) + try: + obj = getattr(module, class_name) + except AttributeError as exc: + raise ImportError( + f"Cannot import '{class_name}' from '{module}'" + ) from exc + func = getattr(obj, attr, None) if func and isinstance(func, ObjectProxy) and hasattr(func, "__wrapped__"): setattr(obj, attr, func.__wrapped__) diff --git a/opentelemetry-instrumentation/tests/test_utils.py b/opentelemetry-instrumentation/tests/test_utils.py index d3807a1bdb..f54a9ed87a 100644 --- a/opentelemetry-instrumentation/tests/test_utils.py +++ b/opentelemetry-instrumentation/tests/test_utils.py @@ -15,6 +15,8 @@ import unittest from http import HTTPStatus +from wrapt import ObjectProxy, wrap_function_wrapper + from opentelemetry.context import ( _SUPPRESS_HTTP_INSTRUMENTATION_KEY, _SUPPRESS_INSTRUMENTATION_KEY, @@ -29,10 +31,19 @@ is_instrumentation_enabled, suppress_http_instrumentation, suppress_instrumentation, + unwrap, ) from opentelemetry.trace import StatusCode +class WrappedClass: + def method(self): + pass + + def wrapper_method(self): + pass + + class TestUtils(unittest.TestCase): # See https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md#status def test_http_status_to_status_code(self): @@ -240,3 +251,72 @@ def test_suppress_http_instrumentation_key(self): self.assertTrue(get_value(_SUPPRESS_HTTP_INSTRUMENTATION_KEY)) self.assertIsNone(get_value(_SUPPRESS_HTTP_INSTRUMENTATION_KEY)) + + @staticmethod + def _wrap_method(): + return wrap_function_wrapper( + WrappedClass, "method", WrappedClass.wrapper_method + ) + + def test_unwrap_can_unwrap_object_attribute(self): + self._wrap_method() + instance = WrappedClass() + self.assertTrue(isinstance(instance.method, ObjectProxy)) + + unwrap(WrappedClass, "method") + self.assertFalse(isinstance(instance.method, ObjectProxy)) + + def test_unwrap_can_unwrap_object_attribute_as_string(self): + self._wrap_method() + instance = WrappedClass() + self.assertTrue(isinstance(instance.method, ObjectProxy)) + + unwrap("tests.test_utils.WrappedClass", "method") + self.assertFalse(isinstance(instance.method, ObjectProxy)) + + def test_unwrap_raises_import_error_if_path_not_well_formed(self): + self._wrap_method() + instance = WrappedClass() + self.assertTrue(isinstance(instance.method, ObjectProxy)) + + with self.assertRaisesRegex( + ImportError, "Cannot parse '' as dotted import path" + ): + unwrap("", "method") + + unwrap(WrappedClass, "method") + self.assertFalse(isinstance(instance.method, ObjectProxy)) + + def test_unwrap_raises_import_error_if_cannot_find_module(self): + self._wrap_method() + instance = WrappedClass() + self.assertTrue(isinstance(instance.method, ObjectProxy)) + + with self.assertRaisesRegex(ImportError, "No module named 'does'"): + unwrap("does.not.exist.WrappedClass", "method") + + unwrap(WrappedClass, "method") + self.assertFalse(isinstance(instance.method, ObjectProxy)) + + def test_unwrap_raises_import_error_if_cannot_find_object(self): + self._wrap_method() + instance = WrappedClass() + self.assertTrue(isinstance(instance.method, ObjectProxy)) + + with self.assertRaisesRegex( + ImportError, "Cannot import 'NotWrappedClass' from" + ): + unwrap("tests.test_utils.NotWrappedClass", "method") + + unwrap(WrappedClass, "method") + self.assertFalse(isinstance(instance.method, ObjectProxy)) + + def test_does_nothing_if_cannot_find_attribute(self): + instance = WrappedClass() + unwrap(instance, "method_not_found") + + def test_unwrap_does_nothing_if_attribute_is_not_from_wrapt(self): + instance = WrappedClass() + self.assertFalse(isinstance(instance.method, ObjectProxy)) + unwrap(WrappedClass, "method") + self.assertFalse(isinstance(instance.method, ObjectProxy)) From eb05bc52f873e6f439b6e22c9f3bd78c62f68fec Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Mon, 21 Oct 2024 15:45:56 +0200 Subject: [PATCH 2/5] Add changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 775043035f..e86b201fa6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#2082](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2082)) - `opentelemetry-instrumentation-redis` Add additional attributes for methods create_index and search, rename those spans ([#2635](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2635)) +- `opentelemetry-instrumentation` Add support for string based dotted module paths in unwrap + ([#2919](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2919)) ### Fixed From f7669da3e969ba77dee3b481dbc1f6d0279f69f2 Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Mon, 21 Oct 2024 15:57:59 +0200 Subject: [PATCH 3/5] Split unwrap tests in their own testcase --- opentelemetry-instrumentation/tests/test_utils.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/opentelemetry-instrumentation/tests/test_utils.py b/opentelemetry-instrumentation/tests/test_utils.py index f54a9ed87a..bd7fdc3c7e 100644 --- a/opentelemetry-instrumentation/tests/test_utils.py +++ b/opentelemetry-instrumentation/tests/test_utils.py @@ -252,13 +252,15 @@ def test_suppress_http_instrumentation_key(self): self.assertIsNone(get_value(_SUPPRESS_HTTP_INSTRUMENTATION_KEY)) + +class UnwrapTestCase: @staticmethod def _wrap_method(): return wrap_function_wrapper( WrappedClass, "method", WrappedClass.wrapper_method ) - def test_unwrap_can_unwrap_object_attribute(self): + def test_can_unwrap_object_attribute(self): self._wrap_method() instance = WrappedClass() self.assertTrue(isinstance(instance.method, ObjectProxy)) @@ -266,7 +268,7 @@ def test_unwrap_can_unwrap_object_attribute(self): unwrap(WrappedClass, "method") self.assertFalse(isinstance(instance.method, ObjectProxy)) - def test_unwrap_can_unwrap_object_attribute_as_string(self): + def test_can_unwrap_object_attribute_as_string(self): self._wrap_method() instance = WrappedClass() self.assertTrue(isinstance(instance.method, ObjectProxy)) @@ -274,7 +276,7 @@ def test_unwrap_can_unwrap_object_attribute_as_string(self): unwrap("tests.test_utils.WrappedClass", "method") self.assertFalse(isinstance(instance.method, ObjectProxy)) - def test_unwrap_raises_import_error_if_path_not_well_formed(self): + def test_raises_import_error_if_path_not_well_formed(self): self._wrap_method() instance = WrappedClass() self.assertTrue(isinstance(instance.method, ObjectProxy)) @@ -287,7 +289,7 @@ def test_unwrap_raises_import_error_if_path_not_well_formed(self): unwrap(WrappedClass, "method") self.assertFalse(isinstance(instance.method, ObjectProxy)) - def test_unwrap_raises_import_error_if_cannot_find_module(self): + def test_raises_import_error_if_cannot_find_module(self): self._wrap_method() instance = WrappedClass() self.assertTrue(isinstance(instance.method, ObjectProxy)) @@ -298,7 +300,7 @@ def test_unwrap_raises_import_error_if_cannot_find_module(self): unwrap(WrappedClass, "method") self.assertFalse(isinstance(instance.method, ObjectProxy)) - def test_unwrap_raises_import_error_if_cannot_find_object(self): + def test_raises_import_error_if_cannot_find_object(self): self._wrap_method() instance = WrappedClass() self.assertTrue(isinstance(instance.method, ObjectProxy)) @@ -311,11 +313,12 @@ def test_unwrap_raises_import_error_if_cannot_find_object(self): unwrap(WrappedClass, "method") self.assertFalse(isinstance(instance.method, ObjectProxy)) + # pylint: disable=no-self-use def test_does_nothing_if_cannot_find_attribute(self): instance = WrappedClass() unwrap(instance, "method_not_found") - def test_unwrap_does_nothing_if_attribute_is_not_from_wrapt(self): + def test_does_nothing_if_attribute_is_not_from_wrapt(self): instance = WrappedClass() self.assertFalse(isinstance(instance.method, ObjectProxy)) unwrap(WrappedClass, "method") From 1e45be199f56e85f0b0702718ae200568b802474 Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Mon, 21 Oct 2024 16:05:21 +0200 Subject: [PATCH 4/5] Oops --- opentelemetry-instrumentation/tests/test_utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentelemetry-instrumentation/tests/test_utils.py b/opentelemetry-instrumentation/tests/test_utils.py index bd7fdc3c7e..5ddd45d692 100644 --- a/opentelemetry-instrumentation/tests/test_utils.py +++ b/opentelemetry-instrumentation/tests/test_utils.py @@ -253,7 +253,7 @@ def test_suppress_http_instrumentation_key(self): self.assertIsNone(get_value(_SUPPRESS_HTTP_INSTRUMENTATION_KEY)) -class UnwrapTestCase: +class UnwrapTestCase(unittest.TestCase): @staticmethod def _wrap_method(): return wrap_function_wrapper( From 50a59a7f0468e023f63702c25cd24c2293cc7e55 Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Tue, 22 Oct 2024 09:43:55 +0200 Subject: [PATCH 5/5] Add typing for first argument --- .../src/opentelemetry/instrumentation/utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py index 5e31109cf0..a0d9ae18f9 100644 --- a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py +++ b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py @@ -16,7 +16,7 @@ from contextlib import contextmanager from importlib import import_module from re import escape, sub -from typing import Dict, Iterable, Sequence +from typing import Dict, Iterable, Sequence, Union from wrapt import ObjectProxy @@ -81,7 +81,7 @@ def http_status_to_status_code( return StatusCode.ERROR -def unwrap(obj, attr: str): +def unwrap(obj: Union[object, str], attr: str): """Given a function that was wrapped by wrapt.wrap_function_wrapper, unwrap it The object containing the function to unwrap may be passed as dotted module path string.