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
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 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

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
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
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
@rjduffner rjduffner force-pushed the rjduffner-autoinstrumentation-2 branch from eecb02f to e0d489f Compare February 24, 2025 19:35
@rjduffner
Copy link
Contributor Author

rjduffner commented Feb 24, 2025

@xrmx changelog fixed up

@rjduffner rjduffner force-pushed the rjduffner-autoinstrumentation-2 branch from 214f9fe to 92d697a Compare March 12, 2025 00:09
@rjduffner
Copy link
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
@rjduffner rjduffner force-pushed the rjduffner-autoinstrumentation-2 branch from 92d697a to f9b310b Compare March 26, 2025 23:00
@rjduffner
Copy link
Contributor Author

rjduffner commented Mar 26, 2025

@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
xrmx
xrmx reviewed Apr 4, 2025
View reviewed changes
Copy link
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
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