Skip to content

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

Merged
xrmx merged 1 commit intoopen-telemetry:mainfrom
rjduffner:rjduffner-autoinstrumentation-2
Apr 8, 2025
Merged

Make auto instrumentation use the same dependency resolver as manual instrumentation does#3202
xrmx merged 1 commit intoopen-telemetry:mainfrom
rjduffner:rjduffner-autoinstrumentation-2

Conversation

@rjduffner
Copy link
Copy Markdown
Contributor

@rjduffner rjduffner commented Jan 22, 2025 •
edited
Loading

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 mentioned this pull request Jan 22, 2025
Closed
rjduffner
rjduffner commented Jan 22, 2025
View reviewed changes
Comment thread opentelemetry-instrumentation/src/opentelemetry/instrumentation/instrumentor.py Outdated
@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
Copy Markdown
Contributor Author

rjduffner commented Jan 28, 2025

@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
Copy Markdown
Contributor Author

rjduffner commented Feb 3, 2025

@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
Copy Markdown
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
View reviewed changes
@lzchen lzchen self-requested a review February 21, 2025 21:54
@lzchen lzchen dismissed their stale review February 21, 2025 21:55

Accident

xrmx
xrmx reviewed Feb 24, 2025
View reviewed changes
Comment thread CHANGELOG.md Outdated
@rjduffner rjduffner force-pushed the rjduffner-autoinstrumentation-2 branch from eecb02f to e0d489f Compare February 24, 2025 19:35
@rjduffner
Copy link
Copy Markdown
Contributor Author

rjduffner commented Feb 24, 2025

@xrmx changelog fixed up

emdneto
emdneto reviewed Feb 25, 2025
View reviewed changes
Comment thread opentelemetry-instrumentation/tests/auto_instrumentation/test_load.py
emdneto
emdneto reviewed Feb 25, 2025
View reviewed changes
Comment thread opentelemetry-instrumentation/tests/test_dependencies.py
Comment thread instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py
@rjduffner rjduffner force-pushed the rjduffner-autoinstrumentation-2 branch from 214f9fe to 92d697a Compare March 12, 2025 00:09
@rjduffner
Copy link
Copy Markdown
Contributor Author

rjduffner commented Mar 18, 2025

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

emdneto
emdneto reviewed Mar 26, 2025
View reviewed changes
Comment thread CHANGELOG.md Outdated
emdneto
emdneto reviewed Mar 26, 2025
View reviewed changes
Comment thread instrumentation/opentelemetry-instrumentation-kafka-python/tests/test_instrumentation.py Outdated
@rjduffner rjduffner force-pushed the rjduffner-autoinstrumentation-2 branch from 92d697a to f9b310b Compare March 26, 2025 23:00
@rjduffner
Copy link
Copy Markdown
Contributor Author

rjduffner commented Mar 26, 2025

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

emdneto
emdneto reviewed Mar 26, 2025
View reviewed changes
Comment thread instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_instrumentation.py
Comment thread instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_instrumentation.py Outdated
@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 commented Apr 3, 2025
View reviewed changes
Comment thread instrumentation/opentelemetry-instrumentation-kafka-python/tests/test_instrumentation.py Outdated
@rjduffner rjduffner force-pushed the rjduffner-autoinstrumentation-2 branch from cbb765d to 5d983b8 Compare April 3, 2025 16:37
xrmx
xrmx reviewed Apr 4, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@xrmx xrmx left a comment •
edited
Loading

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
Copy Markdown
Contributor Author

rjduffner commented Apr 4, 2025

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

@xrmx xrmx merged commit 6d5a514 into open-telemetry:main Apr 8, 2025
720 checks passed
@github-project-automation github-project-automation Bot moved this from Ready for review to Done in Python PR digest Apr 8, 2025
@mindspin mindspin mentioned this pull request Apr 10, 2025
Closed
@aabmass aabmass mentioned this pull request Apr 10, 2025
Closed
@emdneto emdneto mentioned this pull request Apr 14, 2025
Merged
This was referenced Apr 15, 2025
Merged
Closed
Closed
@jeremydvoss jeremydvoss mentioned this pull request Jul 3, 2025
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xrmx xrmx xrmx approved these changes

@pmcollins pmcollins pmcollins approved these changes

@emdneto emdneto emdneto approved these changes

@lzchen lzchen Awaiting requested review from lzchen

Assignees

No one assigned

Labels

None yet

Projects

Python PR digest
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Auto instrumentation and manual instrumentation should both leverage package.py inside the instrumentation for dependency checking

5 participants

@rjduffner @xrmx @pmcollins @emdneto @lzchen
⚡