diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f252e8290..5c4d925be3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +### Fixed + +- `opentelemetry-instrumentation-fastapi`: Fix memory leak in `uninstrument_app()` method by properly removing apps from the tracking set + ([#XXXX](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/XXXX)) + ## Version 1.36.0/0.57b0 (2025-07-29) ### Fixed diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/demo_memory_leak_fix.py b/instrumentation/opentelemetry-instrumentation-fastapi/demo_memory_leak_fix.py new file mode 100644 index 0000000000..a75766d7e5 --- /dev/null +++ b/instrumentation/opentelemetry-instrumentation-fastapi/demo_memory_leak_fix.py @@ -0,0 +1,137 @@ +#!/usr/bin/env python3 +""" +Demo script to demonstrate the memory leak fix in FastAPIInstrumentor.uninstrument_app() + +This script shows the problem described in the issue: +- Calling FastAPIInstrumentor.uninstrument_app() doesn't remove the app parameter + from the _InstrumentedFastAPI._instrumented_fastapi_apps set +- This can lead to memory leaks when instrumenting and uninstrumenting repeatedly + +The fix adds code to remove the app from the set during uninstrument_app(). +""" + +import sys + +import fastapi + +from opentelemetry.instrumentation.fastapi import ( + FastAPIInstrumentor, + _InstrumentedFastAPI, +) + + +def demonstrate_problem(): + """Demonstrate the memory leak problem""" + print("=== Demonstrating Memory Leak Problem ===") + + app = fastapi.FastAPI() + print(f"Initial refcount: {sys.getrefcount(app)}") + print( + f"Initial set size: {len(_InstrumentedFastAPI._instrumented_fastapi_apps)}" + ) + + # Instrument the app + FastAPIInstrumentor.instrument_app(app) + print(f"After instrument - refcount: {sys.getrefcount(app)}") + print( + f"After instrument - set size: {len(_InstrumentedFastAPI._instrumented_fastapi_apps)}" + ) + print( + f"App in set: {app in _InstrumentedFastAPI._instrumented_fastapi_apps}" + ) + + # Uninstrument the app (before fix, this wouldn't remove from set) + FastAPIInstrumentor.uninstrument_app(app) + print(f"After uninstrument - refcount: {sys.getrefcount(app)}") + print( + f"After uninstrument - set size: {len(_InstrumentedFastAPI._instrumented_fastapi_apps)}" + ) + print( + f"App in set: {app in _InstrumentedFastAPI._instrumented_fastapi_apps}" + ) + + # With the fix, the app should be removed from the set + if app not in _InstrumentedFastAPI._instrumented_fastapi_apps: + print("FIXED: App was properly removed from the set") + else: + print("BUG: App is still in the set (memory leak)") + + +def demonstrate_multiple_cycles(): + """Demonstrate multiple instrument/uninstrument cycles""" + print("\n=== Multiple Instrument/Uninstrument Cycles ===") + + app = fastapi.FastAPI() + initial_refcount = sys.getrefcount(app) + print(f"Initial refcount: {initial_refcount}") + + # Perform multiple cycles + for cycle_num in range(3): + FastAPIInstrumentor.instrument_app(app) + FastAPIInstrumentor.uninstrument_app(app) + current_refcount = sys.getrefcount(app) + set_size = len(_InstrumentedFastAPI._instrumented_fastapi_apps) + print( + f"Cycle {cycle_num+1}: refcount={current_refcount}, set_size={set_size}" + ) + + final_refcount = sys.getrefcount(app) + final_set_size = len(_InstrumentedFastAPI._instrumented_fastapi_apps) + + print(f"Final refcount: {final_refcount}") + print(f"Final set size: {final_set_size}") + + if final_set_size == 0: + print("FIXED: No memory leak - set is empty") + else: + print("BUG: Memory leak - set still contains apps") + + +def demonstrate_multiple_apps(): + """Demonstrate multiple apps""" + print("\n=== Multiple Apps Test ===") + + apps = [fastapi.FastAPI() for _ in range(3)] + + print("Instrumenting all apps...") + for app_idx, app in enumerate(apps): + FastAPIInstrumentor.instrument_app(app) + print(f"App {app_idx}: refcount={sys.getrefcount(app)}") + + print( + f"Set size after instrumenting: {len(_InstrumentedFastAPI._instrumented_fastapi_apps)}" + ) + + print("Uninstrumenting all apps...") + for app_idx, app in enumerate(apps): + FastAPIInstrumentor.uninstrument_app(app) + print(f"App {app_idx}: refcount={sys.getrefcount(app)}") + + final_set_size = len(_InstrumentedFastAPI._instrumented_fastapi_apps) + print(f"Final set size: {final_set_size}") + + if final_set_size == 0: + print("FIXED: All apps properly removed from set") + else: + print("BUG: Some apps still in set") + + +if __name__ == "__main__": + print("FastAPIInstrumentor Memory Leak Fix Demo") + print("=" * 50) + + demonstrate_problem() + demonstrate_multiple_cycles() + demonstrate_multiple_apps() + + print("\n" + "=" * 50) + print("Summary:") + print( + "- The fix adds code to remove apps from _instrumented_fastapi_apps during uninstrument_app()" + ) + print( + "- This prevents memory leaks when instrumenting/uninstrumenting repeatedly" + ) + print( + "- The fix is backward compatible and doesn't break existing functionality" + ) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py b/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py index 8ba83985c6..e0e1e0038a 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py @@ -358,6 +358,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 + if app in _InstrumentedFastAPI._instrumented_fastapi_apps: + _InstrumentedFastAPI._instrumented_fastapi_apps.remove(app) + def instrumentation_dependencies(self) -> Collection[str]: return _instruments @@ -388,7 +392,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 diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_memory_leak.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_memory_leak.py new file mode 100644 index 0000000000..5eef1262a3 --- /dev/null +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_memory_leak.py @@ -0,0 +1,155 @@ +# Copyright The OpenTelemetry Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import sys +import unittest + +import fastapi + +from opentelemetry.instrumentation.fastapi import ( + FastAPIInstrumentor, + _InstrumentedFastAPI, +) + +# Check if sys.getrefcount is available (not available in PyPy) +HAS_GETREFCOUNT = hasattr(sys, "getrefcount") + + +class TestFastAPIMemoryLeak(unittest.TestCase): + """Test for memory leak in FastAPIInstrumentor.uninstrument_app()""" + + def test_refcount_after_uninstrument(self): + """Test that refcount is restored after uninstrument_app()""" + if not HAS_GETREFCOUNT: + self.skipTest( + "sys.getrefcount not available in this Python implementation" + ) + + app = fastapi.FastAPI() + + # Instrument the app + FastAPIInstrumentor.instrument_app(app) + refcount_after_instrument = sys.getrefcount(app) + + # Uninstrument the app + FastAPIInstrumentor.uninstrument_app(app) + refcount_after_uninstrument = sys.getrefcount(app) + + # The refcount should be reduced after uninstrument (may not be exactly initial due to Python internals) + self.assertLess( + refcount_after_uninstrument, + refcount_after_instrument, + "Refcount should be reduced after uninstrument_app()", + ) + + # Verify that the app was removed from the set + self.assertNotIn( + app, + _InstrumentedFastAPI._instrumented_fastapi_apps, + "App should be removed from _instrumented_fastapi_apps after uninstrument_app()", + ) + + def test_multiple_instrument_uninstrument_cycles(self): + """Test that multiple instrument/uninstrument cycles don't leak memory""" + if not HAS_GETREFCOUNT: + self.skipTest( + "sys.getrefcount not available in this Python implementation" + ) + + app = fastapi.FastAPI() + + initial_refcount = sys.getrefcount(app) + + # Perform multiple instrument/uninstrument cycles + for cycle_num in range(5): + FastAPIInstrumentor.instrument_app(app) + FastAPIInstrumentor.uninstrument_app(app) + + final_refcount = sys.getrefcount(app) + + # The refcount should not grow significantly after multiple cycles + # (may not be exactly initial due to Python internals) + self.assertLessEqual( + final_refcount, + initial_refcount + + 2, # Allow small increase due to Python internals + f"Refcount after {cycle_num+1} instrument/uninstrument cycles should not grow significantly", + ) + + # Verify that the app is not in the set + self.assertNotIn( + app, + _InstrumentedFastAPI._instrumented_fastapi_apps, + "App should not be in _instrumented_fastapi_apps after uninstrument_app()", + ) + + def test_multiple_apps_instrument_uninstrument(self): + """Test that multiple apps can be instrumented and uninstrumented without leaks""" + if not HAS_GETREFCOUNT: + self.skipTest( + "sys.getrefcount not available in this Python implementation" + ) + + apps = [fastapi.FastAPI() for _ in range(3)] + initial_refcounts = [sys.getrefcount(app) for app in apps] + + # Instrument all apps + for app in apps: + FastAPIInstrumentor.instrument_app(app) + + # Uninstrument all apps + for app in apps: + FastAPIInstrumentor.uninstrument_app(app) + + # Check that refcounts are not significantly increased + for app_idx, app in enumerate(apps): + final_refcount = sys.getrefcount(app) + self.assertLessEqual( + final_refcount, + initial_refcounts[app_idx] + + 2, # Allow small increase due to Python internals + f"App {app_idx} refcount should not grow significantly", + ) + + # Verify that no apps are in the set + for app in apps: + self.assertNotIn( + app, + _InstrumentedFastAPI._instrumented_fastapi_apps, + "All apps should be removed from _instrumented_fastapi_apps", + ) + + def test_demonstrate_fix(self): + """Demonstrate the fix for the memory leak issue""" + app = fastapi.FastAPI() + + # Before the fix: app would remain in _instrumented_fastapi_apps after uninstrument_app() + # After the fix: app should be removed from _instrumented_fastapi_apps + + FastAPIInstrumentor.instrument_app(app) + + # Verify app is in the set after instrumentation + self.assertIn(app, _InstrumentedFastAPI._instrumented_fastapi_apps) + + FastAPIInstrumentor.uninstrument_app(app) + + # Verify app is removed from the set after uninstrumentation + self.assertNotIn(app, _InstrumentedFastAPI._instrumented_fastapi_apps) + self.assertEqual( + len(_InstrumentedFastAPI._instrumented_fastapi_apps), 0 + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py b/instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py index 12db5b9a68..d9df8f6107 100644 --- a/instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py @@ -273,12 +273,14 @@ def response_hook(span: Span, status: str, response_headers: List): ) from opentelemetry.instrumentation.utils import _start_internal_or_server_span from opentelemetry.metrics import get_meter -from opentelemetry.semconv.attributes.http_attributes import HTTP_ROUTE +from opentelemetry.semconv._incubating.attributes.http_attributes import ( + HTTP_ROUTE, + HTTP_TARGET, +) from opentelemetry.semconv.metrics import MetricInstruments from opentelemetry.semconv.metrics.http_metrics import ( HTTP_SERVER_REQUEST_DURATION, ) -from opentelemetry.semconv.trace import SpanAttributes from opentelemetry.util._importlib_metadata import version from opentelemetry.util.http import ( get_excluded_urls, @@ -404,9 +406,7 @@ def _start_response(status, response_headers, *args, **kwargs): if request_route: # http.target to be included in old semantic conventions - duration_attrs_old[SpanAttributes.HTTP_TARGET] = str( - request_route - ) + duration_attrs_old[HTTP_TARGET] = str(request_route) duration_histogram_old.record( max(round(duration_s * 1000), 0), duration_attrs_old @@ -449,7 +449,7 @@ def _before_request(): if flask.request.url_rule: # For 404 that result from no route found, etc, we # don't have a url_rule. - attributes[SpanAttributes.HTTP_ROUTE] = flask.request.url_rule.rule + attributes[HTTP_ROUTE] = flask.request.url_rule.rule span, token = _start_internal_or_server_span( tracer=tracer, span_name=span_name, diff --git a/instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py b/instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py index afcf750e0e..60f08c7cd2 100644 --- a/instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py +++ b/instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py @@ -40,8 +40,34 @@ NumberDataPoint, ) from opentelemetry.sdk.resources import Resource +from opentelemetry.semconv._incubating.attributes.http_attributes import ( + HTTP_FLAVOR, + HTTP_HOST, + HTTP_METHOD, + HTTP_REQUEST_METHOD, + HTTP_RESPONSE_STATUS_CODE, + HTTP_ROUTE, + HTTP_SCHEME, + HTTP_SERVER_NAME, + HTTP_STATUS_CODE, + HTTP_TARGET, +) +from opentelemetry.semconv._incubating.attributes.net_attributes import ( + NET_HOST_NAME, + NET_HOST_PORT, +) +from opentelemetry.semconv._incubating.attributes.network_attributes import ( + NETWORK_PROTOCOL_VERSION, +) +from opentelemetry.semconv._incubating.attributes.server_attributes import ( + SERVER_ADDRESS, + SERVER_PORT, +) +from opentelemetry.semconv._incubating.attributes.url_attributes import ( + URL_PATH, + URL_SCHEME, +) from opentelemetry.semconv.attributes.error_attributes import ERROR_TYPE -from opentelemetry.semconv.trace import SpanAttributes from opentelemetry.test.wsgitestutil import WsgiTestBase from opentelemetry.util.http import ( OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SANITIZE_FIELDS, @@ -57,15 +83,15 @@ def expected_attributes(override_attributes): default_attributes = { - SpanAttributes.HTTP_METHOD: "GET", - SpanAttributes.HTTP_SERVER_NAME: "localhost", - SpanAttributes.HTTP_SCHEME: "http", - SpanAttributes.NET_HOST_PORT: 80, - SpanAttributes.NET_HOST_NAME: "localhost", - SpanAttributes.HTTP_HOST: "localhost", - SpanAttributes.HTTP_TARGET: "/", - SpanAttributes.HTTP_FLAVOR: "1.1", - SpanAttributes.HTTP_STATUS_CODE: 200, + HTTP_METHOD: "GET", + HTTP_SERVER_NAME: "localhost", + HTTP_SCHEME: "http", + NET_HOST_PORT: 80, + NET_HOST_NAME: "localhost", + HTTP_HOST: "localhost", + HTTP_TARGET: "/", + HTTP_FLAVOR: "1.1", + HTTP_STATUS_CODE: 200, } for key, val in override_attributes.items(): default_attributes[key] = val @@ -74,12 +100,12 @@ def expected_attributes(override_attributes): def expected_attributes_new(override_attributes): default_attributes = { - SpanAttributes.HTTP_REQUEST_METHOD: "GET", - SpanAttributes.SERVER_PORT: 80, - SpanAttributes.SERVER_ADDRESS: "localhost", - SpanAttributes.URL_PATH: "/hello/123", - SpanAttributes.NETWORK_PROTOCOL_VERSION: "1.1", - SpanAttributes.HTTP_RESPONSE_STATUS_CODE: 200, + HTTP_REQUEST_METHOD: "GET", + SERVER_PORT: 80, + SERVER_ADDRESS: "localhost", + URL_PATH: "/hello/123", + NETWORK_PROTOCOL_VERSION: "1.1", + HTTP_RESPONSE_STATUS_CODE: 200, } for key, val in override_attributes.items(): default_attributes[key] = val @@ -220,8 +246,8 @@ def assert_environ(): def test_simple(self): expected_attrs = expected_attributes( { - SpanAttributes.HTTP_TARGET: "/hello/123", - SpanAttributes.HTTP_ROUTE: "/hello/", + HTTP_TARGET: "/hello/123", + HTTP_ROUTE: "/hello/", } ) self.client.get("/hello/123") @@ -235,8 +261,8 @@ def test_simple(self): def test_simple_new_semconv(self): expected_attrs = expected_attributes_new( { - SpanAttributes.HTTP_ROUTE: "/hello/", - SpanAttributes.URL_SCHEME: "http", + HTTP_ROUTE: "/hello/", + URL_SCHEME: "http", } ) self.client.get("/hello/123") @@ -250,15 +276,15 @@ def test_simple_new_semconv(self): def test_simple_both_semconv(self): expected_attrs = expected_attributes( { - SpanAttributes.HTTP_TARGET: "/hello/123", - SpanAttributes.HTTP_ROUTE: "/hello/", + HTTP_TARGET: "/hello/123", + HTTP_ROUTE: "/hello/", } ) expected_attrs.update( expected_attributes_new( { - SpanAttributes.HTTP_ROUTE: "/hello/", - SpanAttributes.URL_SCHEME: "http", + HTTP_ROUTE: "/hello/", + URL_SCHEME: "http", } ) ) @@ -301,9 +327,9 @@ def test_not_recording(self): def test_404(self): expected_attrs = expected_attributes( { - SpanAttributes.HTTP_METHOD: "POST", - SpanAttributes.HTTP_TARGET: "/bye", - SpanAttributes.HTTP_STATUS_CODE: 404, + HTTP_METHOD: "POST", + HTTP_TARGET: "/bye", + HTTP_STATUS_CODE: 404, } ) @@ -319,10 +345,10 @@ def test_404(self): def test_404_new_semconv(self): expected_attrs = expected_attributes_new( { - SpanAttributes.HTTP_REQUEST_METHOD: "POST", - SpanAttributes.HTTP_RESPONSE_STATUS_CODE: 404, - SpanAttributes.URL_PATH: "/bye", - SpanAttributes.URL_SCHEME: "http", + HTTP_REQUEST_METHOD: "POST", + HTTP_RESPONSE_STATUS_CODE: 404, + URL_PATH: "/bye", + URL_SCHEME: "http", } ) @@ -338,18 +364,18 @@ def test_404_new_semconv(self): def test_404_both_semconv(self): expected_attrs = expected_attributes( { - SpanAttributes.HTTP_METHOD: "POST", - SpanAttributes.HTTP_TARGET: "/bye", - SpanAttributes.HTTP_STATUS_CODE: 404, + HTTP_METHOD: "POST", + HTTP_TARGET: "/bye", + HTTP_STATUS_CODE: 404, } ) expected_attrs.update( expected_attributes_new( { - SpanAttributes.HTTP_REQUEST_METHOD: "POST", - SpanAttributes.HTTP_RESPONSE_STATUS_CODE: 404, - SpanAttributes.URL_PATH: "/bye", - SpanAttributes.URL_SCHEME: "http", + HTTP_REQUEST_METHOD: "POST", + HTTP_RESPONSE_STATUS_CODE: 404, + URL_PATH: "/bye", + URL_SCHEME: "http", } ) ) @@ -366,9 +392,9 @@ def test_404_both_semconv(self): def test_internal_error(self): expected_attrs = expected_attributes( { - SpanAttributes.HTTP_TARGET: "/hello/500", - SpanAttributes.HTTP_ROUTE: "/hello/", - SpanAttributes.HTTP_STATUS_CODE: 500, + HTTP_TARGET: "/hello/500", + HTTP_ROUTE: "/hello/", + HTTP_STATUS_CODE: 500, } ) resp = self.client.get("/hello/500") @@ -383,11 +409,11 @@ def test_internal_error(self): def test_internal_error_new_semconv(self): expected_attrs = expected_attributes_new( { - SpanAttributes.URL_PATH: "/hello/500", - SpanAttributes.HTTP_ROUTE: "/hello/", - SpanAttributes.HTTP_RESPONSE_STATUS_CODE: 500, + URL_PATH: "/hello/500", + HTTP_ROUTE: "/hello/", + HTTP_RESPONSE_STATUS_CODE: 500, ERROR_TYPE: "500", - SpanAttributes.URL_SCHEME: "http", + URL_SCHEME: "http", } ) resp = self.client.get("/hello/500") @@ -402,18 +428,18 @@ def test_internal_error_new_semconv(self): def test_internal_error_both_semconv(self): expected_attrs = expected_attributes( { - SpanAttributes.HTTP_TARGET: "/hello/500", - SpanAttributes.HTTP_ROUTE: "/hello/", - SpanAttributes.HTTP_STATUS_CODE: 500, + HTTP_TARGET: "/hello/500", + HTTP_ROUTE: "/hello/", + HTTP_STATUS_CODE: 500, } ) expected_attrs.update( expected_attributes_new( { - SpanAttributes.URL_PATH: "/hello/500", - SpanAttributes.HTTP_RESPONSE_STATUS_CODE: 500, + URL_PATH: "/hello/500", + HTTP_RESPONSE_STATUS_CODE: 500, ERROR_TYPE: "500", - SpanAttributes.URL_SCHEME: "http", + URL_SCHEME: "http", } ) )