-
Notifications
You must be signed in to change notification settings - Fork 641
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.
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
Remove pkg_resource and replace for importlib.metadata in autoinstrumentation #2181
Changes from all commits
070d566
9cae805
bf10494
99502c9
1358c12
780bcd5
86f56ef
2364780
9750c8b
11926df
bf44915
8dc274f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,14 +13,13 @@ | |
# limitations under the License. | ||
|
||
from argparse import REMAINDER, ArgumentParser | ||
from importlib.metadata import entry_points | ||
from logging import getLogger | ||
from os import environ, execl, getcwd | ||
from os.path import abspath, dirname, pathsep | ||
from re import sub | ||
from shutil import which | ||
|
||
from pkg_resources import iter_entry_points | ||
xrmx marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
from opentelemetry.instrumentation.version import __version__ | ||
|
||
_logger = getLogger(__name__) | ||
|
@@ -48,8 +47,8 @@ def run() -> None: | |
|
||
argument_otel_environment_variable = {} | ||
|
||
for entry_point in iter_entry_points( | ||
"opentelemetry_environment_variables" | ||
for entry_point in entry_points( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this method of selection is supported for Python 3.8 and 3.9 -> https://docs.python.org/3/library/importlib.metadata.html#entry-points There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Related to my earlier comment #2181 (comment) - if we use the importlib_metadata already taken as a dependency in opentelemetry-api, it's version >=6.4.0 which supports this syntax. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, we should update the import path though 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I'm misunderstanding but we discuss about that here #2181 (comment). And here #2181 (comment) about the version constraint.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm suggesting replacing all uses of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, my bad. It seems reasonable to use the alias. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, please use BTW, since there is a lot of interest in moving this forward, I added some commits on top of your PR in #2854. I cannot continue debugging the failing test cases right now but I encourage you to use the code in #2854, which removes usage of Also, you'll notice that the |
||
group="opentelemetry_environment_variables" | ||
): | ||
environment_variable_module = entry_point.load() | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,11 +12,10 @@ | |
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
from importlib.metadata import EntryPoint, distributions, entry_points | ||
from logging import getLogger | ||
from os import environ | ||
|
||
from pkg_resources import iter_entry_points | ||
|
||
from opentelemetry.instrumentation.dependencies import ( | ||
get_dist_dependency_conflicts, | ||
) | ||
|
@@ -31,9 +30,32 @@ | |
_logger = getLogger(__name__) | ||
|
||
|
||
class _EntryPointDistFinder: | ||
def __int__(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo? (Can probably just be deleted if https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2181/files#r1739126880 is accepted) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes it is a typo |
||
self._mapping = None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider making this a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something like this?:
This would always return the same dist, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant the _mapping attribute within _EntrypointDistFinder - it'll do exactly what you're already doing and only compute _mapping once. So it'd be
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay yes, I agree There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've tried this (with the two for lines swapped) but it looks like the code is not used with our current tests, see So the question is: do we really need this cache? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I popped a breakpoint on dist_for - we only ever pass it |
||
|
||
def dist_for(self, entry_point: EntryPoint): | ||
dist = getattr(entry_point, "dist", None) | ||
if dist: | ||
return dist | ||
|
||
if self._mapping is None: | ||
self._mapping = { | ||
self._key_for(ep): dist | ||
for ep in dist.entry_points | ||
lzchen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
for dist in distributions() | ||
} | ||
|
||
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 iter_entry_points("opentelemetry_distro"): | ||
for entry_point in entry_points(group="opentelemetry_distro"): | ||
try: | ||
# If no distro is specified, use first to come up. | ||
if distro_name is None or distro_name == entry_point.name: | ||
|
@@ -58,23 +80,25 @@ 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 | ||
package_to_exclude = [x.strip() for x in package_to_exclude] | ||
|
||
for entry_point in iter_entry_points("opentelemetry_pre_instrument"): | ||
for entry_point in entry_points(group="opentelemetry_pre_instrument"): | ||
entry_point.load()() | ||
|
||
for entry_point in iter_entry_points("opentelemetry_instrumentor"): | ||
for entry_point in entry_points(group="opentelemetry_instrumentor"): | ||
if entry_point.name in package_to_exclude: | ||
_logger.debug( | ||
"Instrumentation skipped for library %s", entry_point.name | ||
) | ||
continue | ||
|
||
try: | ||
conflict = get_dist_dependency_conflicts(entry_point.dist) | ||
entry_point_dist = entry_point_finder.dist_for(entry_point) | ||
conflict = get_dist_dependency_conflicts(entry_point_dist) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why we are not using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. entry_point.dist doesn't seem to exist in older python versions and is an optional attribute in the latest version. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we are going to use importlib_metadata (via opentelemetry api util) we just need to be assured that it's there since 6+ no? Looking at some old pkg_resources code, dist was default None there too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're not going to be guaranteed 6+ if open-telemetry/opentelemetry-python#4177 goes through - we'll only have 6 on python 3.12 and higher. |
||
if conflict: | ||
_logger.debug( | ||
"Skipping instrumentation %s: %s", | ||
|
@@ -90,14 +114,14 @@ def _load_instrumentors(distro): | |
_logger.exception("Instrumenting of %s failed", entry_point.name) | ||
raise exc | ||
|
||
for entry_point in iter_entry_points("opentelemetry_post_instrument"): | ||
for entry_point in entry_points(group="opentelemetry_post_instrument"): | ||
entry_point.load()() | ||
|
||
|
||
def _load_configurators(): | ||
configurator_name = environ.get(OTEL_PYTHON_CONFIGURATOR, None) | ||
configured = None | ||
for entry_point in iter_entry_points("opentelemetry_configurator"): | ||
for entry_point in entry_points(group="opentelemetry_configurator"): | ||
if configured is not None: | ||
_logger.warning( | ||
"Configuration of %s not loaded, %s already loaded", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
importlib-metadata is already aliased in opentelemetry-api (https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-api/src/opentelemetry/util/_importlib_metadata.py) - should we reuse this rather than re-introduce the dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is at:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#2181 (comment)