Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 1 addition & 3 deletions opentelemetry-api/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,7 @@ classifiers = [
]
dependencies = [
"Deprecated >= 1.2.6",
# FIXME This should be able to be removed after 3.12 is released if there is a reliable API
# in importlib.metadata.
"importlib-metadata >= 6.0, <= 8.4.0",
"importlib-metadata >= 6.0, <= 8.4.0; python_version < '3.10'",
]
dynamic = [
"version",
Expand Down
94 changes: 84 additions & 10 deletions opentelemetry-api/src/opentelemetry/util/_importlib_metadata.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,92 @@
# See the License for the specific language governing permissions and
# limitations under the License.

# FIXME: Use importlib.metadata when support for 3.11 is dropped if the rest of
# the supported versions at that time have the same API.
from importlib_metadata import ( # type: ignore
EntryPoint,
EntryPoints,
entry_points,
version,
)

# The importlib-metadata library has introduced breaking changes before to its
# API, this module is kept just to act as a layer between the
# importlib-metadata library and our project if in any case it is necessary to
# do so.

__all__ = ["entry_points", "version", "EntryPoint", "EntryPoints"]
from sys import version_info

if version_info < (3, 10):
Copy link
Member

@aabmass aabmass Sep 5, 2024

Choose a reason for hiding this comment

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

What is the benefit of not using importlib_metadata for newer Python versions?

I think this branch alone would solve all of our problems and you wouldn't need the custom def entry_points() since importlib_metadata has already provided a python version agnostic API for us

Copy link
Member

Choose a reason for hiding this comment

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

I might have missed something, but I see the main benefit as simply relying on a built-in module instead of external dependency, given that importlib.metadata is no longer provisional since Python 3.10 and we have a stable API. Also minimize issues related to the pinned version of importlib-metadata package

Copy link
Member

@pmcollins pmcollins Sep 6, 2024

Choose a reason for hiding this comment

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

I definitely see your point, Emidio, but my vote is for the simpler solution here until importlib_metadata is no longer needed. Seems like the cost is quite small.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on only relying on one library throughout the codebase. I am FOR using importlib_metadata for now for the reasons @aabmass pointed out in terms of consistency. I am concerned that future contributions will not always use this library and use the standard built-in one instead, after which we must always remember as reviewers that we still support Python versions in which that will not be possible to do. This might be a calculated risk, as the contributions related to these libraries are relatively small, so if the vast majority of us agrees on that and the benefits of using a single, simple library out weigh the review/mental overhead of forcing users to use the backport then we can go ahead with this suggestion.

Keep in mind that if we do somehow want to go with the "custom built in entrypoint()" function of the current PR as of writing this review, our message to contributors in the future will be "use this custom thing we built", which technically is amounts to the same amount of headache/overhead as the previous problem.


# pylint: disable=import-error
from importlib_metadata import version # type: ignore
from importlib_metadata import (
Distribution,
EntryPoint,
EntryPoints,
PackageNotFoundError,
distributions,
)
from importlib_metadata import (
entry_points as importlib_metadata_entry_points,
)
from importlib_metadata import requires

else:

from importlib.metadata import ( # type: ignore
Distribution,
EntryPoint,
EntryPoints,
PackageNotFoundError,
distributions,
)
from importlib.metadata import (
entry_points as importlib_metadata_entry_points,
)

from importlib.metadata import requires, version # isort: skip


def entry_points(**params) -> EntryPoints: # type: ignore
"""
Same as entry_points but requires at least one argument
For Python 3.8 or 3.9:
Copy link
Contributor

Choose a reason for hiding this comment

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

there was a proposal for a unified solution for the entry_points API in the pkg_resources removal PR in the contrib repo which works for all currently supported python versions.
maybe this one could be used here?

isinstance(
importlib_metadata.entry_points(), importlib_metadata.EntryPoints
)
evaluates to True.
For Python 3.10, 3.11:
isinstance(
importlib.metadata.entry_points(), importlib.metadata.SelectableGroups
)
evaluates to True.
For Python 3.12:
isinstance(
importlib.metadata.entry_points(), importlib.metadata.EntryPoints
)
evaluates to True.
So, when called with no arguments, entry_points returns objects of
different types depending on the Python version that is being used. This is
obviously very problematic. Nevertheless, in our code we don't ever call
entry_points without arguments, so the approach here is to redefine
entry_points so that it requires at least one argument.
"""

if not params: # type: ignore
raise ValueError("entry_points requires at least one argument")
return importlib_metadata_entry_points(**params) # type: ignore


__all__ = [
"entry_points",
"version",
"EntryPoint",
"EntryPoints",
"requires",
"Distribution",
"distributions",
"PackageNotFoundError",
]
15 changes: 15 additions & 0 deletions opentelemetry-api/test-requirements-1.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
asgiref==3.7.2
Deprecated==1.2.14
iniconfig==2.0.0
packaging==24.0
pluggy==1.5.0
py-cpuinfo==9.0.0
pytest==7.4.4
tomli==2.0.1
typing_extensions==4.10.0
wrapt==1.16.0
zipp==3.19.2
-e opentelemetry-sdk
-e opentelemetry-semantic-conventions
-e tests/opentelemetry-test-utils
-e opentelemetry-api
69 changes: 53 additions & 16 deletions opentelemetry-api/tests/util/test__importlib_metadata.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,46 @@
# See the License for the specific language governing permissions and
# limitations under the License.

from sys import version_info
from unittest import TestCase

from opentelemetry.metrics import MeterProvider
from opentelemetry.util._importlib_metadata import EntryPoint, EntryPoints
from opentelemetry.util._importlib_metadata import (
entry_points as importlib_metadata_entry_points,
)
from opentelemetry.util._importlib_metadata import version


class TestDependencies(TestCase):

def test_dependencies(self):

# pylint: disable=import-outside-toplevel
# pylint: disable=unused-import
# pylint: disable=import-error
if version_info < (3, 10):
try:
import importlib_metadata # type: ignore

except ImportError:
self.fail(
"importlib_metadata not installed when testing with "
f"{version_info.major}.{version_info.minor}"
)

else:
try:
import importlib_metadata # noqa

except ImportError:
pass

else:
self.fail(
"importlib_metadata installed when testing with "
f"{version_info.major}.{version_info.minor}"
)


class TestEntryPoints(TestCase):
Expand All @@ -40,27 +73,29 @@ def test_uniform_behavior(self):
"""
Test that entry_points behaves the same regardless of the Python
version.
"""

entry_points = importlib_metadata_entry_points()

self.assertIsInstance(entry_points, EntryPoints)
importlib.metadata was introduced in 3.8 as a replacement for
pkg_resources. The problem is that the API of importlib.metadata
changed in subsequent versions.

entry_points = entry_points.select(group="opentelemetry_propagator")
self.assertIsInstance(entry_points, EntryPoints)
For example, in 3.8 or 3.9 importlib.metadata.entry_points does not
support the keyword arguments group or name, but those keyword
arguments are supported in 3.10, 3.11 and 3.12.

entry_points = entry_points.select(name="baggage")
self.assertIsInstance(entry_points, EntryPoints)
There exists a package named importlib-metadata that has an API that
includes a function named importlib_metadata.entry_points which
supports the keyword arguments group and name, so we use
importlib_metadata.entry_points when running in 3.8 or 3.9.

entry_point = next(iter(entry_points))
self.assertIsInstance(entry_point, EntryPoint)
importlib_metadata.entry_points and importlib.metadata.entry_points do
not return objects of the same type when called without any arguments.
That is why the implementation of the
opentelemetry.util._importlib_metadata redefines entry_points so that
it is mandatory to use an argument.
"""

self.assertEqual(entry_point.name, "baggage")
self.assertEqual(entry_point.group, "opentelemetry_propagator")
self.assertEqual(
entry_point.value,
"opentelemetry.baggage.propagation:W3CBaggagePropagator",
)
with self.assertRaises(ValueError):
importlib_metadata_entry_points()

entry_points = importlib_metadata_entry_points(
group="opentelemetry_propagator"
Expand Down Expand Up @@ -106,3 +141,5 @@ def test_uniform_behavior(self):
entry_points = importlib_metadata_entry_points(group="abc", name="abc")
self.assertIsInstance(entry_points, EntryPoints)
self.assertEqual(len(entry_points), 0)

self.assertIsInstance(version("opentelemetry-api"), str)
5 changes: 4 additions & 1 deletion tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,10 @@ commands_pre =

mypy,mypyinstalled: pip install -r {toxinidir}/mypy-requirements.txt

api: pip install -r {toxinidir}/opentelemetry-api/test-requirements.txt
py3{8,9}-test-opentelemetry-api: pip install -r {toxinidir}/opentelemetry-api/test-requirements-0.txt
pypy3-test-opentelemetry-api: pip install -r {toxinidir}/opentelemetry-api/test-requirements-0.txt
py3{10,11,12}-test-opentelemetry-api: pip install -r {toxinidir}/opentelemetry-api/test-requirements-1.txt
lint-opentelemetry-api: pip install -r {toxinidir}/opentelemetry-api/test-requirements-1.txt

sdk: pip install -r {toxinidir}/opentelemetry-sdk/test-requirements.txt
benchmark-opentelemetry-sdk: pip install -r {toxinidir}/opentelemetry-sdk/benchmark-requirements.txt
Expand Down