Skip to content
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

Instrumentation runtime checks #475

Merged
merged 2 commits into from
May 27, 2021

Conversation

owais
Copy link
Contributor

@owais owais commented Apr 27, 2021

Description

This change updates instrumentations to remove the instrumented libraries as install time dependencies. It also adds runtime checks to instrumentations so they don't try to instrument unsupported versions of a library.

Effectively this means installing instrumentations will not automatically install the instrumented libraries. For example:

  • Installing opentelemetry-instrumentation-requests will not install requests.

Manual instrumentation

  • If requests is not installed and a user still attempts to manually instrument requests in code, it'll result in ImportError.
  • If a version of requests is installed that is not supported by the instrumenation, the instrumentation will be a noop and will log a debug message.

Auto instrumentation (via opentelemetry-instrument command)

  • If requests is not installed or an incompatible version is installed, the instrumentation will not be used. In fact it won't be imported at all. No errors are raised for missing or incompatible instrumentations when auto-instrumenting.

Updated logic is mostly contained in the opentelemetry-instrumentation package. Look at instrumentors.BaseInsrumentor and sitecustomize._load_instrumentors. Rest of the changes are just about updating packaging/layout for all instrumentations.

Implements a solution for: open-telemetry/opentelemetry-python#1729

Includes changes from #474. Will get simpler once that is merged.

Blocks #514

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

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

  • Unit Test

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • 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
  • Documentation has been updated

@owais owais force-pushed the instrumentation-runtime-checks branch 21 times, most recently from 5b811ae to 8829586 Compare April 28, 2021 08:10
@owais owais marked this pull request as ready for review April 28, 2021 08:37
@owais owais requested review from a team, codeboten and srikanthccv and removed request for a team April 28, 2021 08:37
@owais owais force-pushed the instrumentation-runtime-checks branch 4 times, most recently from 49e25cb to f84cbdf Compare April 28, 2021 10:03
@owais owais force-pushed the instrumentation-runtime-checks branch 4 times, most recently from 9377231 to e00338d Compare May 26, 2021 01:35
@owais owais mentioned this pull request May 26, 2021
11 tasks
# limitations under the License.


_instruments = ("boto~=2.0",)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you include in CONTRIBUTING.MD for the instrumentation checklist that _instruments needs to be included?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I'll do it in #514 as it includes some more changes wrt to contributing instrumentations.

Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

Just a minor typo 👍

@owais owais force-pushed the instrumentation-runtime-checks branch 3 times, most recently from e8a1574 to 0fafe94 Compare May 26, 2021 19:49
@owais owais force-pushed the instrumentation-runtime-checks branch from 0fafe94 to 35e7010 Compare May 26, 2021 21:56
@owais owais mentioned this pull request May 27, 2021
11 tasks
@lzchen lzchen merged commit daa7238 into open-telemetry:main May 27, 2021
adamantike added a commit to adamantike/opentelemetry-python-contrib that referenced this pull request Jul 22, 2021
Since the introduction of the `_instruments` runtime checks in open-telemetry#475, the
FastAPI instrumentation has stopped working for versions >= `0.59.0`.
However the current test suite passes even for the latest released
version at the moment (`0.67.0`).

It seems this isn't related to a limitation in the instrumentation code,
but actually because of it being created when `0.58` was the latest version:
open-telemetry/opentelemetry-python@7bec76a.
adamantike added a commit to adamantike/opentelemetry-python-contrib that referenced this pull request Jul 22, 2021
Since the introduction of the `_instruments` runtime checks in open-telemetry#475, the
FastAPI instrumentation has stopped working for versions >= `0.59.0`.
However the current test suite passes even for the latest released
version at the moment (`0.67.0`).

It seems this isn't related to a limitation in the instrumentation code,
but actually because of it being created when `0.58` was the latest version:
open-telemetry/opentelemetry-python@7bec76a.
owais added a commit that referenced this pull request Aug 24, 2021
* Allow instrumentation of newer FastAPI versions

Since the introduction of the `_instruments` runtime checks in #475, the
FastAPI instrumentation has stopped working for versions >= `0.59.0`.
However the current test suite passes even for the latest released
version at the moment (`0.67.0`).

It seems this isn't related to a limitation in the instrumentation code,
but actually because of it being created when `0.58` was the latest version:
open-telemetry/opentelemetry-python@7bec76a.

* Add changelog entry

Co-authored-by: Owais Lone <owais@users.noreply.github.com>
@lzchen lzchen mentioned this pull request Sep 20, 2021
6 tasks
@owais owais deleted the instrumentation-runtime-checks branch January 26, 2022 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants