Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make auto instrumentation use the same dependency resolver as manual instrumentation does #3202

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Make auto instrumentation use the same dependency resolver as manual instrumentation does #3202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ae1003d
Make auto instrumentation use the same dependency resolver as manual …
rjduffner Jan 21, 2025
aad1679
Remove get_dist_dependency_conflicts method as well
rjduffner Jan 28, 2025
2371493
Add type hints
rjduffner Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
93 changes: 54 additions & 39 deletions instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
55 changes: 12 additions & 43 deletions ...telemetry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/_load.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,50 +12,22 @@
# 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,
OTEL_PYTHON_DISABLED_INSTRUMENTATIONS,
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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why you are removing this cache? I'm not sure this cache really helps with anything :)

Copy link
Contributor Author

@rjduffner rjduffner Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I removed line 103 below, I couldn't find any other usages of this class so I removed this class just to keep things tidy.

If I missed a usage or a use case somewhere I am happy to re-add it. I just pulled it since my code search told me it was dead code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think get_dist_dependency_conflicts is now unused too

> ag get_dist_dependency_conflicts
opentelemetry-instrumentation/tests/test_dependencies.py
23:    get_dist_dependency_conflicts,
68:    def test_get_dist_dependency_conflicts(self):
82:        conflict = get_dist_dependency_conflicts(dist)
90:    def test_get_dist_dependency_conflicts_requires_none(self):
103:        conflict = get_dist_dependency_conflicts(dist)

opentelemetry-instrumentation/src/opentelemetry/instrumentation/dependencies.py
51:def get_dist_dependency_conflicts(

CHANGELOG.md
64:- `opentelemetry-instrumentation` Fix `get_dist_dependency_conflicts` if no distribution requires

@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"):
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
26 changes: 8 additions & 18 deletions opentelemetry-instrumentation/src/opentelemetry/instrumentation/dependencies.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
from packaging.requirements import InvalidRequirement, Requirement

from opentelemetry.util._importlib_metadata import (
Distribution,
PackageNotFoundError,
version,
)
Expand All @@ -40,23 +39,14 @@ def __str__(self):
return f'DependencyConflict: requested: "{self.required}" but found: "{self.found}"'


def get_dist_dependency_conflicts(
dist: Distribution,
) -> DependencyConflict | None:
instrumentation_deps = []
extra = "extra"
instruments = "instruments"
instruments_marker = {extra: instruments}
if dist.requires:
for dep in dist.requires:
if extra not in dep or instruments not in dep:
continue

req = Requirement(dep)
if req.marker.evaluate(instruments_marker):
instrumentation_deps.append(req)

return get_dependency_conflicts(instrumentation_deps)
class DependencyConflictError(Exception):
conflict: DependencyConflict

def __init__(self, conflict: DependencyConflict):
self.conflict = conflict

def __str__(self):
return str(self.conflict)


def get_dependency_conflicts(
Expand Down
6 changes: 6 additions & 0 deletions opentelemetry-instrumentation/src/opentelemetry/instrumentation/instrumentor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
)
from opentelemetry.instrumentation.dependencies import (
DependencyConflict,
DependencyConflictError,
get_dependency_conflicts,
)

Expand Down Expand Up @@ -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
Expand Down
Loading