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

Allow instrumentation of newer FastAPI versions #602

Merged
merged 5 commits into from
Aug 24, 2021

Conversation

adamantike
Copy link
Contributor

@adamantike adamantike commented Jul 22, 2021

Description

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.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • tox -e test-instrumentation-fastapi, which now installs latest FastAPI version.

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
  • Documentation has been updated

@adamantike adamantike requested review from a team, codeboten and aabmass and removed request for a team July 22, 2021 03:30
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 adamantike force-pushed the fastapi-newer-versions branch from d054d9a to e5b40b0 Compare July 22, 2021 15:31
@adamantike
Copy link
Contributor Author

Gentle ping to some of the reviewers for the initial PR with FastAPI support, just to increase awareness and visibility, as it would be great to allow newer FastAPI versions in the next minor/patch version of the library!

cc @codeboten @lzchen

@lzchen
Copy link
Contributor

lzchen commented Jul 30, 2021

@adamantike
Which versions are actually not working? Just 0.58.1?

@owais
Copy link
Contributor

owais commented Jul 31, 2021

  • Does FastAPI follow semver?
  • Can we may be declare compatibility with all FastAPI versions that are <1.0?

@potatochip
Copy link

@adamantike
Which versions are actually not working? Just 0.58.1?

Anything greater than 0.58 is no longer working. Without testing, pretty sure that anything less than 0.58 is not working either. The fastapi instrumentor only works for fastapi version 0.58.x now.

@potatochip
Copy link

  • Does FastAPI follow semver?
  • Can we may be declare compatibility with all FastAPI versions that are <1.0?

They follow semver. Should pin to less than v1 like you propose.

@owais
Copy link
Contributor

owais commented Aug 3, 2021

They follow semver. Should pin to less than v1 like you propose.

I assumed all newer version were working. Sorry about the confusion. We should pin to the specific known working versions only and open an issue to support newer unsupported versions.

@adamantike
Copy link
Contributor Author

Does FastAPI follow semver?
Can we may be declare compatibility with all FastAPI versions that are <1.0?

They do follow SemVer, according to https://fastapi.tiangolo.com/deployment/versions/. However, as FastAPI is still at pre-1.0, any new minor version could include breaking changes.

I haven't found any issues when testing version 0.66.1, and I think allowing ~=0.58 (to support >=0.58,<1.0) allows us to support newer minor versions that don't break the existing contracts for middlewares. As soon as a new minor version adds breaking changes that affect this instrumentation, we can add a stricter pin on FastAPI.

@adamantike
Copy link
Contributor Author

@owais, is there any context or changes I can provide here, to move forward with this PR?

@owais owais merged commit 9c4eb69 into open-telemetry:main Aug 24, 2021
@owais
Copy link
Contributor

owais commented Aug 24, 2021

@adamantike the CI was having multiple issues one after the other. Looks like we've sorted out most of the issues now. Thanks for your patience.

@adamantike adamantike deleted the fastapi-newer-versions branch August 25, 2021 18:27
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