-
Notifications
You must be signed in to change notification settings - Fork 761
fix: fastapi memory leak only #3688
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
base: main
Are you sure you want to change the base?
Changes from 9 commits
0a74f67
af9c720
7218064
1f5b5c9
48799a3
a0546d4
b5149ac
f3e93c8
ea27204
834c6b7
fc60799
7be0d5b
4b6f0b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added correct link to PR |
||||||
|
||||||
- `opentelemetry-instrumentation`: Fix dependency conflict detection when instrumented packages are not installed by moving check back to before instrumentors are loaded. Add "instruments-any" feature for instrumentations that target multiple packages. | ||||||
([#3610](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3610)) | ||||||
- infra(ci): Fix git pull failures in core contrib test | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -186,6 +186,7 @@ def client_response_hook(span: Span, scope: dict[str, Any], message: dict[str, A | |
import logging | ||
import types | ||
from typing import Collection, Literal | ||
from weakref import WeakSet as _WeakSet | ||
|
||
import fastapi | ||
from starlette.applications import Starlette | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since WeakSet accomplishes avoiding memory leaks here, I think this is more There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update comment to clarify purpose of removing app from WeakSet |
||
# Use discard to avoid KeyError if already GC'ed | ||
_InstrumentedFastAPI._instrumented_fastapi_apps.discard(app) | ||
|
||
def instrumentation_dependencies(self) -> Collection[str]: | ||
return _instruments | ||
|
||
|
@@ -388,7 +393,11 @@ def _instrument(self, **kwargs): | |
fastapi.FastAPI = _InstrumentedFastAPI | ||
|
||
def _uninstrument(self, **kwargs): | ||
for instance in _InstrumentedFastAPI._instrumented_fastapi_apps: | ||
# Create a copy of the set to avoid RuntimeError during iteration | ||
instances_to_uninstrument = list( | ||
_InstrumentedFastAPI._instrumented_fastapi_apps | ||
) | ||
for instance in instances_to_uninstrument: | ||
self.uninstrument_app(instance) | ||
_InstrumentedFastAPI._instrumented_fastapi_apps.clear() | ||
fastapi.FastAPI = self._original_fastapi | ||
|
@@ -406,7 +415,8 @@ class _InstrumentedFastAPI(fastapi.FastAPI): | |
_http_capture_headers_sanitize_fields: list[str] | None = None | ||
_exclude_spans: list[Literal["receive", "send"]] | None = None | ||
|
||
_instrumented_fastapi_apps = set() | ||
# Track instrumented app instances using weak references to avoid GC leaks | ||
_instrumented_fastapi_apps = _WeakSet() | ||
_sem_conv_opt_in_mode = _StabilityMode.DEFAULT | ||
|
||
def __init__(self, *args, **kwargs): | ||
|
@@ -426,10 +436,6 @@ def __init__(self, *args, **kwargs): | |
) | ||
_InstrumentedFastAPI._instrumented_fastapi_apps.add(self) | ||
|
||
def __del__(self): | ||
if self in _InstrumentedFastAPI._instrumented_fastapi_apps: | ||
_InstrumentedFastAPI._instrumented_fastapi_apps.remove(self) | ||
|
||
|
||
def _get_route_details(scope): | ||
""" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to move that to
# Unreleased
sectionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to #Unreleased section