fix: fastapi memory leak only #3688
Conversation
…ist to avoid memory leaks
… WeakSet and safe discard
| @@ -358,6 +359,10 @@ def uninstrument_app(app: fastapi.FastAPI): | |||
| app.middleware_stack = app.build_middleware_stack() | |||
| app._is_instrumented_by_opentelemetry = False | |||
|
|
|||
| # Remove the app from the set of instrumented apps to prevent memory leaks | |||
There was a problem hiding this comment.
Since WeakSet accomplishes avoiding memory leaks here, I think this is more Avoid calling u instrument twice if the instrumentation is later disabled or such
There was a problem hiding this comment.
Update comment to clarify purpose of removing app from WeakSet
artemziborev
changed the title
|
Hi @anuraaga, just a gentle reminder — could you take a look at this PR? All checks have passed 👍 |
|
Hi @artemziborev - LGTM. I had already approved since was just a comment nit, but I'm not a maintainer so it's a grey check |
CHANGELOG.md
Outdated
| @@ -15,6 +15,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|
|||
| ### Fixed | |||
|
|
|||
| - `opentelemetry-instrumentation-fastapi`: Fix memory leak in `uninstrument_app()` by properly removing apps from the tracking set | |||
There was a problem hiding this comment.
Need to move that to # Unreleased section
There was a problem hiding this comment.
Moved to #Unreleased section
CHANGELOG.md
Outdated
| @@ -15,6 +15,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|
|||
| ### Fixed | |||
|
|
|||
| - `opentelemetry-instrumentation-fastapi`: Fix memory leak in `uninstrument_app()` by properly removing apps from the tracking set | |||
| ([#3688](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3688), [#3683](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3683)) | |||
There was a problem hiding this comment.
| ([#3688](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3688), [#3683](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3683)) | |
| ([#3688](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3688) |
There was a problem hiding this comment.
Added correct link to PR
…to fix/fastapi-memory-leak-only
Description
Fix FastAPI instrumentation memory leak by tracking instrumented apps with a WeakSet instead of a strong-referenced set. This avoids retaining strong references to
fastapi.FastAPIinstances, allowing them to be garbage-collected even ifuninstrument_app()isn’t called. Also aligns with the approach used in the Starlette instrumentation.Scope minimized to FastAPI only:
_instrumented_fastapi_appsanddiscard()on removaluninstrument_app()test_fastapi_instrumentation.pyFixes # (issue)
Type of change
How Has This Been Tested?
test_fastapi_app_is_collected_after_instrumentintest_fastapi_instrumentation.py, which:gc.collect()Does This PR Require a Core Repo Change?
Checklist: