From 00362cf947dc9d0bf63a1ef35cb9c7fcb90b603b Mon Sep 17 00:00:00 2001 From: rjduffner Date: Tue, 21 Jan 2025 14:02:29 -0800 Subject: [PATCH] Make auto instrumentation use the same dependency resolver as manual instrumentation does --- CHANGELOG.md | 3 + .../tests/test_fastapi_instrumentation.py | 93 ++++++++++-------- .../auto_instrumentation/_load.py | 55 +++-------- .../instrumentation/dependencies.py | 8 ++ .../instrumentation/instrumentor.py | 6 ++ .../tests/auto_instrumentation/test_load.py | 94 +++++++------------ .../tests/test_instrumentor.py | 19 ++++ 7 files changed, 138 insertions(+), 140 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f78a8d50f..0e7249da9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +- Make auto instrumentation use the same dependency resolver as manual instrumentation does + ([#3202](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3202)) + ### Added - `opentelemetry-instrumentation-confluent-kafka` Add support for confluent-kafka <=2.7.0 diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index fdbad4effb..439cebf427 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -16,7 +16,7 @@ import unittest from timeit import default_timer -from unittest.mock import Mock, patch +from unittest.mock import Mock, call, patch import fastapi from fastapi.middleware.httpsredirect import HTTPSRedirectMiddleware @@ -37,6 +37,10 @@ from opentelemetry.instrumentation.auto_instrumentation._load import ( _load_instrumentors, ) +from opentelemetry.instrumentation.dependencies import ( + DependencyConflict, + DependencyConflictError, +) from opentelemetry.sdk.metrics.export import ( HistogramDataPoint, NumberDataPoint, @@ -54,10 +58,7 @@ from opentelemetry.semconv.trace import SpanAttributes from opentelemetry.test.globals_test import reset_trace_globals from opentelemetry.test.test_base import TestBase -from opentelemetry.util._importlib_metadata import ( - PackageNotFoundError, - entry_points, -) +from opentelemetry.util._importlib_metadata import entry_points from opentelemetry.util.http import ( OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SANITIZE_FIELDS, OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST, @@ -1031,26 +1032,6 @@ def client_response_hook(send_span, scope, message): ) -def mock_version_with_fastapi(*args, **kwargs): - req_name = args[0] - if req_name == "fastapi": - # TODO: Value now matters - return "0.58" - raise PackageNotFoundError() - - -def mock_version_with_old_fastapi(*args, **kwargs): - req_name = args[0] - if req_name == "fastapi": - # TODO: Value now matters - return "0.57" - raise PackageNotFoundError() - - -def mock_version_without_fastapi(*args, **kwargs): - raise PackageNotFoundError() - - class TestAutoInstrumentation(TestBaseAutoFastAPI): """Test the auto-instrumented variant @@ -1062,31 +1043,65 @@ def test_entry_point_exists(self): (ep,) = entry_points(group="opentelemetry_instrumentor") self.assertEqual(ep.name, "fastapi") - @patch("opentelemetry.instrumentation.dependencies.version") - def test_instruments_with_fastapi_installed(self, mock_version): - mock_version.side_effect = mock_version_with_fastapi + @staticmethod + def _instrumentation_loaded_successfully_call(): + return call("Instrumented %s", "fastapi") + + @staticmethod + def _instrumentation_failed_to_load_call(dependency_conflict): + return call( + "Skipping instrumentation %s: %s", "fastapi", dependency_conflict + ) + + @patch("opentelemetry.instrumentation.auto_instrumentation._load._logger") + def test_instruments_with_fastapi_installed(self, mock_logger): mock_distro = Mock() + mock_distro.load_instrumentor.return_value = None _load_instrumentors(mock_distro) - mock_version.assert_called_once_with("fastapi") self.assertEqual(len(mock_distro.load_instrumentor.call_args_list), 1) (ep,) = mock_distro.load_instrumentor.call_args.args self.assertEqual(ep.name, "fastapi") + mock_logger.debug.assert_has_calls( + [self._instrumentation_loaded_successfully_call()] + ) - @patch("opentelemetry.instrumentation.dependencies.version") - def test_instruments_with_old_fastapi_installed(self, mock_version): # pylint: disable=no-self-use - mock_version.side_effect = mock_version_with_old_fastapi + @patch("opentelemetry.instrumentation.auto_instrumentation._load._logger") + def test_instruments_with_old_fastapi_installed(self, mock_logger): # pylint: disable=no-self-use + dependency_conflict = DependencyConflict("0.58", "0.57") mock_distro = Mock() + mock_distro.load_instrumentor.side_effect = DependencyConflictError( + dependency_conflict + ) _load_instrumentors(mock_distro) - mock_version.assert_called_once_with("fastapi") - mock_distro.load_instrumentor.assert_not_called() + self.assertEqual(len(mock_distro.load_instrumentor.call_args_list), 1) + (ep,) = mock_distro.load_instrumentor.call_args.args + self.assertEqual(ep.name, "fastapi") + assert ( + self._instrumentation_loaded_successfully_call() + not in mock_logger.debug.call_args_list + ) + mock_logger.debug.assert_has_calls( + [self._instrumentation_failed_to_load_call(dependency_conflict)] + ) - @patch("opentelemetry.instrumentation.dependencies.version") - def test_instruments_without_fastapi_installed(self, mock_version): # pylint: disable=no-self-use - mock_version.side_effect = mock_version_without_fastapi + @patch("opentelemetry.instrumentation.auto_instrumentation._load._logger") + def test_instruments_without_fastapi_installed(self, mock_logger): # pylint: disable=no-self-use + dependency_conflict = DependencyConflict("0.58", None) mock_distro = Mock() + mock_distro.load_instrumentor.side_effect = DependencyConflictError( + dependency_conflict + ) _load_instrumentors(mock_distro) - mock_version.assert_called_once_with("fastapi") - mock_distro.load_instrumentor.assert_not_called() + self.assertEqual(len(mock_distro.load_instrumentor.call_args_list), 1) + (ep,) = mock_distro.load_instrumentor.call_args.args + self.assertEqual(ep.name, "fastapi") + assert ( + self._instrumentation_loaded_successfully_call() + not in mock_logger.debug.call_args_list + ) + mock_logger.debug.assert_has_calls( + [self._instrumentation_failed_to_load_call(dependency_conflict)] + ) def _create_app(self): # instrumentation is handled by the instrument call diff --git a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/_load.py b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/_load.py index 3d602b2a1d..5084879faa 100644 --- a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/_load.py +++ b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/_load.py @@ -12,13 +12,10 @@ # See the License for the specific language governing permissions and # limitations under the License. -from functools import cached_property from logging import getLogger from os import environ -from opentelemetry.instrumentation.dependencies import ( - get_dist_dependency_conflicts, -) +from opentelemetry.instrumentation.dependencies import DependencyConflictError from opentelemetry.instrumentation.distro import BaseDistro, DefaultDistro from opentelemetry.instrumentation.environment_variables import ( OTEL_PYTHON_CONFIGURATOR, @@ -26,36 +23,11 @@ OTEL_PYTHON_DISTRO, ) from opentelemetry.instrumentation.version import __version__ -from opentelemetry.util._importlib_metadata import ( - EntryPoint, - distributions, - entry_points, -) +from opentelemetry.util._importlib_metadata import entry_points _logger = getLogger(__name__) -class _EntryPointDistFinder: - @cached_property - def _mapping(self): - return { - self._key_for(ep): dist - for dist in distributions() - for ep in dist.entry_points - } - - def dist_for(self, entry_point: EntryPoint): - dist = getattr(entry_point, "dist", None) - if dist: - return dist - - return self._mapping.get(self._key_for(entry_point)) - - @staticmethod - def _key_for(entry_point: EntryPoint): - return f"{entry_point.group}:{entry_point.name}:{entry_point.value}" - - def _load_distro() -> BaseDistro: distro_name = environ.get(OTEL_PYTHON_DISTRO, None) for entry_point in entry_points(group="opentelemetry_distro"): @@ -83,7 +55,6 @@ def _load_distro() -> BaseDistro: def _load_instrumentors(distro): package_to_exclude = environ.get(OTEL_PYTHON_DISABLED_INSTRUMENTATIONS, []) - entry_point_finder = _EntryPointDistFinder() if isinstance(package_to_exclude, str): package_to_exclude = package_to_exclude.split(",") # to handle users entering "requests , flask" or "requests, flask" with spaces @@ -100,19 +71,17 @@ def _load_instrumentors(distro): continue try: - entry_point_dist = entry_point_finder.dist_for(entry_point) - conflict = get_dist_dependency_conflicts(entry_point_dist) - if conflict: - _logger.debug( - "Skipping instrumentation %s: %s", - entry_point.name, - conflict, - ) - continue - - # tell instrumentation to not run dep checks again as we already did it above - distro.load_instrumentor(entry_point, skip_dep_check=True) + distro.load_instrumentor( + entry_point, raise_exception_on_conflict=True + ) _logger.debug("Instrumented %s", entry_point.name) + except DependencyConflictError as exc: + _logger.debug( + "Skipping instrumentation %s: %s", + entry_point.name, + exc.conflict, + ) + continue except ImportError: # in scenarios using the kubernetes operator to do autoinstrumentation some # instrumentors (usually requiring binary extensions) may fail to load diff --git a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/dependencies.py b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/dependencies.py index b7e4cff400..57a78c23c5 100644 --- a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/dependencies.py +++ b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/dependencies.py @@ -40,6 +40,14 @@ def __str__(self): return f'DependencyConflict: requested: "{self.required}" but found: "{self.found}"' +class DependencyConflictError(Exception): + def __init__(self, conflict: DependencyConflict): + self.conflict = conflict + + def __str__(self): + return str(self.conflict) + + def get_dist_dependency_conflicts( dist: Distribution, ) -> DependencyConflict | None: diff --git a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/instrumentor.py b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/instrumentor.py index cf079dbfb7..dead45fbac 100644 --- a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/instrumentor.py +++ b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/instrumentor.py @@ -28,6 +28,7 @@ ) from opentelemetry.instrumentation.dependencies import ( DependencyConflict, + DependencyConflictError, get_dependency_conflicts, ) @@ -104,10 +105,15 @@ def instrument(self, **kwargs: Any): # check if instrumentor has any missing or conflicting dependencies skip_dep_check = kwargs.pop("skip_dep_check", False) + raise_exception_on_conflict = kwargs.pop( + "raise_exception_on_conflict", False + ) if not skip_dep_check: conflict = self._check_dependency_conflicts() if conflict: _LOG.error(conflict) + if raise_exception_on_conflict: + raise DependencyConflictError(conflict) return None # initialize semantic conventions opt-in if needed diff --git a/opentelemetry-instrumentation/tests/auto_instrumentation/test_load.py b/opentelemetry-instrumentation/tests/auto_instrumentation/test_load.py index ce9abe1365..2653651d84 100644 --- a/opentelemetry-instrumentation/tests/auto_instrumentation/test_load.py +++ b/opentelemetry-instrumentation/tests/auto_instrumentation/test_load.py @@ -17,13 +17,16 @@ from unittest.mock import Mock, call, patch from opentelemetry.instrumentation.auto_instrumentation import _load +from opentelemetry.instrumentation.dependencies import ( + DependencyConflict, + DependencyConflictError, +) from opentelemetry.instrumentation.environment_variables import ( OTEL_PYTHON_CONFIGURATOR, OTEL_PYTHON_DISABLED_INSTRUMENTATIONS, OTEL_PYTHON_DISTRO, ) from opentelemetry.instrumentation.version import __version__ -from opentelemetry.util._importlib_metadata import EntryPoint, entry_points class TestLoad(TestCase): @@ -200,17 +203,20 @@ def test_load_distro_error(self, iter_mock, isinstance_mock): # Confirm method raises exception if it fails to load a distro. self.assertRaises(Exception, _load._load_distro) + @staticmethod + def _instrumentation_failed_to_load_call(entry_point, dependency_conflict): + return call( + "Skipping instrumentation %s: %s", entry_point, dependency_conflict + ) + @patch.dict( "os.environ", {OTEL_PYTHON_DISABLED_INSTRUMENTATIONS: " instr1 , instr3 "}, ) - @patch( - "opentelemetry.instrumentation.auto_instrumentation._load.get_dist_dependency_conflicts" - ) @patch( "opentelemetry.instrumentation.auto_instrumentation._load.entry_points" ) - def test_load_instrumentors(self, iter_mock, dep_mock): + def test_load_instrumentors(self, iter_mock): # Mock opentelemetry_pre_instrument entry points # pylint: disable=too-many-locals pre_ep_mock1 = Mock() @@ -255,8 +261,6 @@ def test_load_instrumentors(self, iter_mock, dep_mock): (ep_mock1, ep_mock2, ep_mock3, ep_mock4), (post_ep_mock1, post_ep_mock2), ] - # No dependency conflict - dep_mock.return_value = None _load._load_instrumentors(distro_mock) # All opentelemetry_pre_instrument entry points should be loaded pre_mock1.assert_called_once() @@ -265,8 +269,8 @@ def test_load_instrumentors(self, iter_mock, dep_mock): # Only non-disabled instrumentations should be loaded distro_mock.load_instrumentor.assert_has_calls( [ - call(ep_mock2, skip_dep_check=True), - call(ep_mock4, skip_dep_check=True), + call(ep_mock2, raise_exception_on_conflict=True), + call(ep_mock4, raise_exception_on_conflict=True), ] ) self.assertEqual(distro_mock.load_instrumentor.call_count, 2) @@ -278,13 +282,11 @@ def test_load_instrumentors(self, iter_mock, dep_mock): "os.environ", {OTEL_PYTHON_DISABLED_INSTRUMENTATIONS: " instr1 , instr3 "}, ) - @patch( - "opentelemetry.instrumentation.auto_instrumentation._load.get_dist_dependency_conflicts" - ) + @patch("opentelemetry.instrumentation.auto_instrumentation._load._logger") @patch( "opentelemetry.instrumentation.auto_instrumentation._load.entry_points" ) - def test_load_instrumentors_dep_conflict(self, iter_mock, dep_mock): # pylint: disable=no-self-use + def test_load_instrumentors_dep_conflict(self, iter_mock, mock_logger): # pylint: disable=no-self-use ep_mock1 = Mock() ep_mock1.name = "instr1" @@ -297,27 +299,36 @@ def test_load_instrumentors_dep_conflict(self, iter_mock, dep_mock): # pylint: ep_mock4 = Mock() ep_mock4.name = "instr4" + dependency_conflict = DependencyConflict("1.2.3", None) + distro_mock = Mock() + distro_mock.load_instrumentor.side_effect = [ + None, + DependencyConflictError(dependency_conflict), + ] iter_mock.return_value = (ep_mock1, ep_mock2, ep_mock3, ep_mock4) - # If a dependency conflict is raised, that instrumentation should not be loaded, but others still should. - dep_mock.side_effect = [None, "DependencyConflict"] _load._load_instrumentors(distro_mock) distro_mock.load_instrumentor.assert_has_calls( [ - call(ep_mock2, skip_dep_check=True), + call(ep_mock2, raise_exception_on_conflict=True), + call(ep_mock4, raise_exception_on_conflict=True), + ] + ) + assert distro_mock.load_instrumentor.call_count == 2 + mock_logger.debug.assert_has_calls( + [ + self._instrumentation_failed_to_load_call( + ep_mock4.name, dependency_conflict + ) ] ) - distro_mock.load_instrumentor.assert_called_once() - @patch( - "opentelemetry.instrumentation.auto_instrumentation._load.get_dist_dependency_conflicts" - ) @patch( "opentelemetry.instrumentation.auto_instrumentation._load.entry_points" ) def test_load_instrumentors_import_error_does_not_stop_everything( - self, iter_mock, dep_mock + self, iter_mock ): ep_mock1 = Mock(name="instr1") ep_mock2 = Mock(name="instr2") @@ -331,25 +342,21 @@ def test_load_instrumentors_import_error_does_not_stop_everything( (ep_mock1, ep_mock2), (), ] - dep_mock.return_value = None _load._load_instrumentors(distro_mock) distro_mock.load_instrumentor.assert_has_calls( [ - call(ep_mock1, skip_dep_check=True), - call(ep_mock2, skip_dep_check=True), + call(ep_mock1, raise_exception_on_conflict=True), + call(ep_mock2, raise_exception_on_conflict=True), ] ) self.assertEqual(distro_mock.load_instrumentor.call_count, 2) - @patch( - "opentelemetry.instrumentation.auto_instrumentation._load.get_dist_dependency_conflicts" - ) @patch( "opentelemetry.instrumentation.auto_instrumentation._load.entry_points" ) - def test_load_instrumentors_raises_exception(self, iter_mock, dep_mock): + def test_load_instrumentors_raises_exception(self, iter_mock): ep_mock1 = Mock(name="instr1") ep_mock2 = Mock(name="instr2") @@ -362,14 +369,13 @@ def test_load_instrumentors_raises_exception(self, iter_mock, dep_mock): (ep_mock1, ep_mock2), (), ] - dep_mock.return_value = None with self.assertRaises(ValueError): _load._load_instrumentors(distro_mock) distro_mock.load_instrumentor.assert_has_calls( [ - call(ep_mock1, skip_dep_check=True), + call(ep_mock1, raise_exception_on_conflict=True), ] ) self.assertEqual(distro_mock.load_instrumentor.call_count, 1) @@ -379,31 +385,3 @@ def test_load_instrumentors_no_entry_point_mocks(self): _load._load_instrumentors(distro_mock) # this has no specific assert because it is run for every instrumentation self.assertTrue(distro_mock) - - def test_entry_point_dist_finder(self): - entry_point_finder = _load._EntryPointDistFinder() - self.assertTrue(entry_point_finder._mapping) - entry_point = list( - entry_points(group="opentelemetry_environment_variables") - )[0] - self.assertTrue(entry_point) - self.assertTrue(entry_point.dist) - - # this will not hit cache - entry_point_dist = entry_point_finder.dist_for(entry_point) - self.assertTrue(entry_point_dist) - # dist are not comparable so we are sure we are not hitting the cache - self.assertEqual(entry_point.dist, entry_point_dist) - - # this will hit cache - entry_point_without_dist = EntryPoint( - name=entry_point.name, - group=entry_point.group, - value=entry_point.value, - ) - self.assertIsNone(entry_point_without_dist.dist) - new_entry_point_dist = entry_point_finder.dist_for( - entry_point_without_dist - ) - # dist are not comparable, being truthy is enough - self.assertTrue(new_entry_point_dist) diff --git a/opentelemetry-instrumentation/tests/test_instrumentor.py b/opentelemetry-instrumentation/tests/test_instrumentor.py index dee32c34e4..ed60dff60c 100644 --- a/opentelemetry-instrumentation/tests/test_instrumentor.py +++ b/opentelemetry-instrumentation/tests/test_instrumentor.py @@ -15,7 +15,12 @@ from logging import WARNING from unittest import TestCase +from unittest.mock import patch +from opentelemetry.instrumentation.dependencies import ( + DependencyConflict, + DependencyConflictError, +) from opentelemetry.instrumentation.instrumentor import BaseInstrumentor @@ -48,3 +53,17 @@ def test_protect(self): def test_singleton(self): self.assertIs(self.Instrumentor(), self.Instrumentor()) + + @patch('opentelemetry.instrumentation.instrumentor.BaseInstrumentor._check_dependency_conflicts') + def test_instrument_missing_dependency_raise(self, mock__check_dependency_conflicts): + instrumentor = self.Instrumentor() + + mock__check_dependency_conflicts.return_value = DependencyConflict( + "missing", "missing" + ) + + self.assertRaises( + DependencyConflictError, + instrumentor.instrument, + raise_exception_on_conflict=True, + )