From 7fbec0fdabb8a88e5360cc6ad8cb1eba45026f30 Mon Sep 17 00:00:00 2001 From: Artem Ziborev Date: Fri, 23 May 2025 15:42:26 +0400 Subject: [PATCH 1/6] refactor(flask): replace SpanAttributes with semconv attributes --- .../instrumentation/flask/__init__.py | 9 +- .../tests/test_programmatic.py | 131 +++++++++++------- 2 files changed, 86 insertions(+), 54 deletions(-) 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..d3acaf1667 100644 --- a/instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py @@ -278,7 +278,10 @@ def response_hook(span: Span, status: str, response_headers: List): from opentelemetry.semconv.metrics.http_metrics import ( HTTP_SERVER_REQUEST_DURATION, ) -from opentelemetry.semconv.trace import SpanAttributes +from opentelemetry.semconv._incubating.attributes.http_attributes import ( + HTTP_TARGET, + HTTP_ROUTE +) from opentelemetry.util._importlib_metadata import version from opentelemetry.util.http import ( get_excluded_urls, @@ -404,7 +407,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( + duration_attrs_old[HTTP_TARGET] = str( request_route ) @@ -449,7 +452,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..5230ca2d6d 100644 --- a/instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py +++ b/instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py @@ -41,7 +41,36 @@ ) from opentelemetry.sdk.resources import Resource from opentelemetry.semconv.attributes.error_attributes import ERROR_TYPE -from opentelemetry.semconv.trace import SpanAttributes +from opentelemetry.semconv._incubating.attributes.http_attributes import ( + HTTP_METHOD, + HTTP_SERVER_NAME, + HTTP_SCHEME, + HTTP_HOST, + HTTP_TARGET, + HTTP_FLAVOR, + HTTP_STATUS_CODE, + HTTP_REQUEST_METHOD, + HTTP_RESPONSE_STATUS_CODE, + HTTP_ROUTE, + + +) +from opentelemetry.semconv._incubating.attributes.net_attributes import ( + NET_HOST_PORT, + NET_HOST_NAME, + +) +from opentelemetry.semconv._incubating.attributes.server_attributes import ( + SERVER_PORT, + SERVER_ADDRESS, +) +from opentelemetry.semconv._incubating.attributes.url_attributes import ( + URL_PATH, + URL_SCHEME, +) +from opentelemetry.semconv._incubating.attributes.network_attributes import ( + NETWORK_PROTOCOL_VERSION +) from opentelemetry.test.wsgitestutil import WsgiTestBase from opentelemetry.util.http import ( OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SANITIZE_FIELDS, @@ -57,15 +86,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 +103,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 +249,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 +264,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 +279,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 +330,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 +348,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 +367,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 +395,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 +412,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 +431,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", } ) ) From 85df3aea715cb4c496d1eddd1b6fafaace460990 Mon Sep 17 00:00:00 2001 From: Artem Ziborev Date: Fri, 23 May 2025 17:01:39 +0400 Subject: [PATCH 2/6] refactor(flask): replace SpanAttributes with semconv attributes (fix wrong import) --- .../src/opentelemetry/instrumentation/flask/__init__.py | 1 - 1 file changed, 1 deletion(-) 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 d3acaf1667..b7e40970a3 100644 --- a/instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py @@ -273,7 +273,6 @@ 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.metrics import MetricInstruments from opentelemetry.semconv.metrics.http_metrics import ( HTTP_SERVER_REQUEST_DURATION, From b93c54775694191d7595d634e05f8974d13bde26 Mon Sep 17 00:00:00 2001 From: Artem Ziborev Date: Wed, 28 May 2025 11:44:09 +0400 Subject: [PATCH 3/6] refactor(flask): replace SpanAttributes with semconv attributes (fix ruff linting) --- .../instrumentation/flask/__init__.py | 12 ++++----- .../tests/test_programmatic.py | 27 +++++++++---------- 2 files changed, 17 insertions(+), 22 deletions(-) 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 b7e40970a3..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,14 +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._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._incubating.attributes.http_attributes import ( - HTTP_TARGET, - HTTP_ROUTE -) from opentelemetry.util._importlib_metadata import version from opentelemetry.util.http import ( get_excluded_urls, @@ -406,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[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 diff --git a/instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py b/instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py index 5230ca2d6d..60f08c7cd2 100644 --- a/instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py +++ b/instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py @@ -40,37 +40,34 @@ NumberDataPoint, ) from opentelemetry.sdk.resources import Resource -from opentelemetry.semconv.attributes.error_attributes import ERROR_TYPE from opentelemetry.semconv._incubating.attributes.http_attributes import ( - HTTP_METHOD, - HTTP_SERVER_NAME, - HTTP_SCHEME, - HTTP_HOST, - HTTP_TARGET, HTTP_FLAVOR, - HTTP_STATUS_CODE, + 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_PORT, 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_PORT, SERVER_ADDRESS, + SERVER_PORT, ) from opentelemetry.semconv._incubating.attributes.url_attributes import ( URL_PATH, URL_SCHEME, ) -from opentelemetry.semconv._incubating.attributes.network_attributes import ( - NETWORK_PROTOCOL_VERSION -) +from opentelemetry.semconv.attributes.error_attributes import ERROR_TYPE from opentelemetry.test.wsgitestutil import WsgiTestBase from opentelemetry.util.http import ( OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SANITIZE_FIELDS, From 0b7906958b2aa4791cb17142fb1f33c6d704c3bf Mon Sep 17 00:00:00 2001 From: Artem Ziborev Date: Wed, 6 Aug 2025 09:15:39 +0400 Subject: [PATCH 4/6] fix(fastapi-instrumentation): properly remove app from instrumented list to avoid memory leaks --- CHANGELOG.md | 5 + .../demo_memory_leak_fix.py | 135 +++++++++++++++++ .../instrumentation/fastapi/__init__.py | 4 + .../tests/test_fastapi_memory_leak.py | 141 ++++++++++++++++++ 4 files changed, 285 insertions(+) create mode 100644 instrumentation/opentelemetry-instrumentation-fastapi/demo_memory_leak_fix.py create mode 100644 instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_memory_leak.py 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..c663f04321 --- /dev/null +++ b/instrumentation/opentelemetry-instrumentation-fastapi/demo_memory_leak_fix.py @@ -0,0 +1,135 @@ +#!/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 i 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 {i+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 i, app in enumerate(apps): + FastAPIInstrumentor.instrument_app(app) + print(f"App {i}: refcount={sys.getrefcount(app)}") + + print( + f"Set size after instrumenting: {len(_InstrumentedFastAPI._instrumented_fastapi_apps)}" + ) + + print("Uninstrumenting all apps...") + for i, app in enumerate(apps): + FastAPIInstrumentor.uninstrument_app(app) + print(f"App {i}: 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..18aaf12309 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 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..e3e7909d8b --- /dev/null +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_memory_leak.py @@ -0,0 +1,141 @@ +# 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 + + +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()""" + 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 + from opentelemetry.instrumentation.fastapi import _InstrumentedFastAPI + + 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""" + app = fastapi.FastAPI() + + initial_refcount = sys.getrefcount(app) + + # Perform multiple instrument/uninstrument cycles + for i 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 {i+1} instrument/uninstrument cycles should not grow significantly", + ) + + # Verify that the app is not in the set + from opentelemetry.instrumentation.fastapi import _InstrumentedFastAPI + + 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""" + 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 i, app in enumerate(apps): + final_refcount = sys.getrefcount(app) + self.assertLessEqual( + final_refcount, + initial_refcounts[i] + + 2, # Allow small increase due to Python internals + f"App {i} refcount should not grow significantly", + ) + + # Verify that no apps are in the set + from opentelemetry.instrumentation.fastapi import _InstrumentedFastAPI + + 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) + from opentelemetry.instrumentation.fastapi import _InstrumentedFastAPI + + # 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() From 34ed91680954e73ebffabfe13e1bb7017ea25dba Mon Sep 17 00:00:00 2001 From: Artem Ziborev Date: Wed, 6 Aug 2025 09:23:45 +0400 Subject: [PATCH 5/6] fix(fastapi-instrumentation): linter fix --- .../demo_memory_leak_fix.py | 14 +++++++----- .../tests/test_fastapi_memory_leak.py | 22 ++++++++----------- 2 files changed, 17 insertions(+), 19 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/demo_memory_leak_fix.py b/instrumentation/opentelemetry-instrumentation-fastapi/demo_memory_leak_fix.py index c663f04321..a75766d7e5 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/demo_memory_leak_fix.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/demo_memory_leak_fix.py @@ -66,12 +66,14 @@ def demonstrate_multiple_cycles(): print(f"Initial refcount: {initial_refcount}") # Perform multiple cycles - for i in range(3): + 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 {i+1}: refcount={current_refcount}, set_size={set_size}") + 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) @@ -92,18 +94,18 @@ def demonstrate_multiple_apps(): apps = [fastapi.FastAPI() for _ in range(3)] print("Instrumenting all apps...") - for i, app in enumerate(apps): + for app_idx, app in enumerate(apps): FastAPIInstrumentor.instrument_app(app) - print(f"App {i}: refcount={sys.getrefcount(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 i, app in enumerate(apps): + for app_idx, app in enumerate(apps): FastAPIInstrumentor.uninstrument_app(app) - print(f"App {i}: refcount={sys.getrefcount(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}") diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_memory_leak.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_memory_leak.py index e3e7909d8b..d4040d79a2 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_memory_leak.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_memory_leak.py @@ -17,7 +17,10 @@ import fastapi -from opentelemetry.instrumentation.fastapi import FastAPIInstrumentor +from opentelemetry.instrumentation.fastapi import ( + FastAPIInstrumentor, + _InstrumentedFastAPI, +) class TestFastAPIMemoryLeak(unittest.TestCase): @@ -43,8 +46,6 @@ def test_refcount_after_uninstrument(self): ) # Verify that the app was removed from the set - from opentelemetry.instrumentation.fastapi import _InstrumentedFastAPI - self.assertNotIn( app, _InstrumentedFastAPI._instrumented_fastapi_apps, @@ -58,7 +59,7 @@ def test_multiple_instrument_uninstrument_cycles(self): initial_refcount = sys.getrefcount(app) # Perform multiple instrument/uninstrument cycles - for i in range(5): + for cycle_num in range(5): FastAPIInstrumentor.instrument_app(app) FastAPIInstrumentor.uninstrument_app(app) @@ -70,12 +71,10 @@ def test_multiple_instrument_uninstrument_cycles(self): final_refcount, initial_refcount + 2, # Allow small increase due to Python internals - f"Refcount after {i+1} instrument/uninstrument cycles should not grow significantly", + f"Refcount after {cycle_num+1} instrument/uninstrument cycles should not grow significantly", ) # Verify that the app is not in the set - from opentelemetry.instrumentation.fastapi import _InstrumentedFastAPI - self.assertNotIn( app, _InstrumentedFastAPI._instrumented_fastapi_apps, @@ -96,18 +95,16 @@ def test_multiple_apps_instrument_uninstrument(self): FastAPIInstrumentor.uninstrument_app(app) # Check that refcounts are not significantly increased - for i, app in enumerate(apps): + for app_idx, app in enumerate(apps): final_refcount = sys.getrefcount(app) self.assertLessEqual( final_refcount, - initial_refcounts[i] + initial_refcounts[app_idx] + 2, # Allow small increase due to Python internals - f"App {i} refcount should not grow significantly", + f"App {app_idx} refcount should not grow significantly", ) # Verify that no apps are in the set - from opentelemetry.instrumentation.fastapi import _InstrumentedFastAPI - for app in apps: self.assertNotIn( app, @@ -123,7 +120,6 @@ def test_demonstrate_fix(self): # After the fix: app should be removed from _instrumented_fastapi_apps FastAPIInstrumentor.instrument_app(app) - from opentelemetry.instrumentation.fastapi import _InstrumentedFastAPI # Verify app is in the set after instrumentation self.assertIn(app, _InstrumentedFastAPI._instrumented_fastapi_apps) From b5a4e29f609098d35a303231fb24017f92438e53 Mon Sep 17 00:00:00 2001 From: Artem Ziborev Date: Wed, 6 Aug 2025 09:58:08 +0400 Subject: [PATCH 6/6] fix(fastapi-instrumentation): fix tests --- .../instrumentation/fastapi/__init__.py | 6 +++++- .../tests/test_fastapi_memory_leak.py | 18 ++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) 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 18aaf12309..e0e1e0038a 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py @@ -392,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 index d4040d79a2..5eef1262a3 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_memory_leak.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_memory_leak.py @@ -22,12 +22,20 @@ _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 @@ -54,6 +62,11 @@ def test_refcount_after_uninstrument(self): 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) @@ -83,6 +96,11 @@ def test_multiple_instrument_uninstrument_cycles(self): 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]