-
Notifications
You must be signed in to change notification settings - Fork 606
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
Remove pkg_resource and replace for importlib.metadata in autoinstrumentation #2181
Conversation
@Rodrigo-Novas please sign the CLA ✌️ |
...telemetry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/__init__.py
Outdated
Show resolved
Hide resolved
opentelemetry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/_load.py
Outdated
Show resolved
Hide resolved
opentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap.py
Outdated
Show resolved
Hide resolved
opentelemetry-instrumentation/src/opentelemetry/instrumentation/dependencies.py
Show resolved
Hide resolved
opentelemetry-instrumentation/src/opentelemetry/instrumentation/distro.py
Show resolved
Hide resolved
...telemetry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/__init__.py
Show resolved
Hide resolved
opentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap.py
Show resolved
Hide resolved
opentelemetry-instrumentation/src/opentelemetry/instrumentation/dependencies.py
Outdated
Show resolved
Hide resolved
...telemetry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/__init__.py
Outdated
Show resolved
Hide resolved
Is there an expected ETA on this? |
I believe this would also fix: |
Hope this gets picked back up! Great work so far! |
@@ -19,7 +19,7 @@ | |||
from re import sub | |||
from shutil import which | |||
|
|||
from pkg_resources import iter_entry_points | |||
from importlib.metadata import entry_points |
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.
You are adding importlib-metadata
to the dependencies, but this import is from the Python standard library. There is a discussion here: open-telemetry/opentelemetry-python#3234 about using importlib-metadata
and its implications. Note my comment at the end.
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.
Hello, sorry for the delay.
If I understand correctly, you mean having a standard library with its own constraint within Opentelemetry would be wrong. I think that the change of importlib-metadata version could be made later if an error is reported.
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.
IMHO its fine to depend on importlib-metadata
, as its the python-version independent backport of the stdlib feature.
it will be easier to develop against.
Hey there! Could you prioritize the code review for this PR? It's for migrating to Python 3.12. Thanks a bunch! :D |
Any ETA on this? |
Are you still working on this? If so we can prioritize getting this reviewed and merged. It is difficult to know what the current status is so please close the conversations that you think have been resolved and note down the open topics that still need to be discussed. Feel free to also join the Python SIG if you want to bring this issue up to the community specifically. |
We are also getting hit with this on every unit test run. It would be great to get a fix in. |
Its ok for me, you can review it! |
i will try to fix it |
"wrapt >= 1.0.0, < 2.0.0", | ||
"importlib-metadata >= 6.0, < 8.0", |
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:
"importlib-metadata >= 6.0, <= 8.4.0",
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.
Switching from |
When is this PR expected to be approved and merged? |
When it's ready |
814b7d0
to
2c94ac4
Compare
15a2a47
to
8dc274f
Compare
We are trying to get more eyes on this PR, we realize it is important for many people. The issue at hand here is not particularly easy to solve (kudos to @Rodrigo-Novas for the work so far!) since it deals with replacing an old library with a new one where there is not necessarily a 1:1 relationship between old library and new library components. To make matters even more complicated, the new library has been implemented very inconsistently, so its behavior for Python 3.x is not the same as for Python 3.y. We are working on this and we'll merge as soon as we can 🙂 |
For anyone following - this is currently blocked by (at minimum) open-telemetry/opentelemetry-python#4177, as well as a new release needed for open-telemetry/opentelemetry-python#4181 . Some of our assumptions in the previous discussions mentioned need to be re-addressed if we swap to relying on the stdlib in python <= 3.11, which will delay this a bit further. |
We're keeping importlib_metadata as dependency and PRs run against main of branch of core repo so no need for releases. The current blocker for me is this #2181 (comment) |
Closing, since this has been included in #2871. Thanks everyone! |
Description
Pkgresource dependency is deprecated, on this pr i replaced pkg_resource for importlib.metadata.
Fixes #2180
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.