Skip to content

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

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

Merged

Conversation

rjduffner
Copy link
Contributor

@rjduffner rjduffner commented Jan 22, 2025

Description

In order for the auto instrumentation to use the same dependency checker as the instrumentation itself, I introduce an exception that allows for an early exit from instrument function through the distro and back to the _load.

Fixes #3201

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Updated existing tests to handle the case of the new dependency check
  • Added a new test to make sure that when a dependency conflict gets returned to the instrument function, it raises an exception.

Does This PR Require a Core Repo Change?

  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added

@rjduffner rjduffner force-pushed the rjduffner-autoinstrumentation-2 branch 3 times, most recently from 9cf2a02 to 49762d1 Compare January 27, 2025 22:22
@rjduffner rjduffner changed the title Add a DependencyConflictError Exception To Allow For Instrumentation To Stop When Dependcies Are Not Installed Make auto instrumentation use the same dependency resolver as manual instrumentation does Jan 27, 2025
@rjduffner rjduffner marked this pull request as ready for review January 27, 2025 22:34
@rjduffner rjduffner requested a review from a team as a code owner January 27, 2025 22:34
@rjduffner rjduffner force-pushed the rjduffner-autoinstrumentation-2 branch 3 times, most recently from 00362cf to 149f84f Compare January 28, 2025 00:02
@rjduffner
Copy link
Contributor Author

@xrmx, Here is an option for making the auto instrumentation use the same dependency checker as the manual instrumentation.

There are a few rough edges but I think ironing those out gets us down a path of a major rewrite pretty quickly so maybe this little PR could work.

@rjduffner rjduffner force-pushed the rjduffner-autoinstrumentation-2 branch from 3533e55 to aad1679 Compare January 28, 2025 22:00
@rjduffner
Copy link
Contributor Author

@xrmx just checking in, have you had a chance to look at this and my responses to your comment?

@rjduffner rjduffner force-pushed the rjduffner-autoinstrumentation-2 branch 2 times, most recently from 28e57e5 to 1b6d07e Compare February 6, 2025 04:47
@xrmx
Copy link
Contributor

xrmx commented Feb 6, 2025

@xrmx just checking in, have you had a chance to look at this and my responses to your comment?

Not yet, sorry

@rjduffner rjduffner force-pushed the rjduffner-autoinstrumentation-2 branch 2 times, most recently from d8af135 to a5c86fa Compare February 13, 2025 04:21
@rjduffner rjduffner force-pushed the rjduffner-autoinstrumentation-2 branch 2 times, most recently from 87a167d to eecb02f Compare February 19, 2025 22:40
lzchen
lzchen previously approved these changes Feb 21, 2025
@lzchen lzchen self-requested a review February 21, 2025 21:54
@lzchen lzchen dismissed their stale review February 21, 2025 21:55

Accident

@rjduffner rjduffner force-pushed the rjduffner-autoinstrumentation-2 branch from eecb02f to e0d489f Compare February 24, 2025 19:35
@rjduffner
Copy link
Contributor Author

@xrmx changelog fixed up

@rjduffner
Copy link
Contributor Author

rjduffner commented Mar 3, 2025

@emdneto,

I added some testing but stopped short of the full auto instrumentation test. I stopped short because the maintainer of kafka-python has come back and is now releasing updates again. The maintainer of kafka-python-ng is reccomending switching back to kafka-python and will be archiving kafka-python-ng soon (kafka-python-ng/kafka-python-ng#210 (comment)).

These two packages were the reason this whole bug came about. But in a future PR (very soon) I'll be back with an update to the kafka-python instrumentation so it will support py312 and removal of the kafka-python-ng part of this instrumentation.

In that vein, I don't think we need to add a test for a use case that will no longer happen here shortly.

However, i still believe this PR should be merged and pushed out as I think using the same dependency resolver everywhere is the better option long term as there is one other instrumentation that also has this same behavior.

cc @xrmx

@rjduffner rjduffner force-pushed the rjduffner-autoinstrumentation-2 branch from 7fa0e85 to 97db979 Compare March 4, 2025 00:07
@xrmx
Copy link
Contributor

xrmx commented Mar 6, 2025

@emdneto,

I added some testing but stopped short of the full auto instrumentation test. I stopped short because the maintainer of kafka-python has come back and is now releasing updates again. The maintainer of kafka-python-ng is reccomending switching back to kafka-python and will be archiving kafka-python-ng soon (kafka-python-ng/kafka-python-ng#210 (comment)).

These two packages were the reason this whole bug came about. But in a future PR (very soon) I'll be back with an update to the kafka-python instrumentation so it will support py312 and removal of the kafka-python-ng part of this instrumentation.

In that vein, I don't think we need to add a test for a use case that will no longer happen here shortly.

However, i still believe this PR should be merged and pushed out as I think using the same dependency resolver everywhere is the better option long term as there is one other instrumentation that also has this same behavior.

cc @xrmx

psycopg2 / psycopg2-binary won't go away so we can write a test case for that

@rjduffner
Copy link
Contributor Author

Ok ill take a look at those tomorrow... hopefully. :)

@rjduffner rjduffner force-pushed the rjduffner-autoinstrumentation-2 branch from 97db979 to 379a9f2 Compare March 11, 2025 20:54
@rjduffner rjduffner force-pushed the rjduffner-autoinstrumentation-2 branch from 214f9fe to 92d697a Compare March 12, 2025 00:09
@rjduffner
Copy link
Contributor Author

@xrmx and @emdneto, when ever you get a chance, this is ready for another look. Thank you!

@rjduffner rjduffner force-pushed the rjduffner-autoinstrumentation-2 branch from 92d697a to f9b310b Compare March 26, 2025 23:00
@rjduffner
Copy link
Contributor Author

@emdneto thanks for the review. Everything should be updated.

@rjduffner rjduffner force-pushed the rjduffner-autoinstrumentation-2 branch 2 times, most recently from 7e3db32 to 91d41d7 Compare March 31, 2025 14:38
@rjduffner rjduffner force-pushed the rjduffner-autoinstrumentation-2 branch from 3eed921 to cbb765d Compare April 2, 2025 18:41
@rjduffner rjduffner force-pushed the rjduffner-autoinstrumentation-2 branch from cbb765d to 5d983b8 Compare April 3, 2025 16:37
Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

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

LGTM but unfortunately on conflict this now reproduces open-telemetry/opentelemetry-python#4509 . I'm working on a PR to fix that

UPDATE: PR here open-telemetry/opentelemetry-python#4528

@rjduffner
Copy link
Contributor Author

Thanks @xrmx, i can rebase once that PR gets merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants