diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e894a4d70..75d248b3dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#3012](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3012)) - `opentelemetry-instrumentation-starlette` Remove max version constraint on starlette ([#3456](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3456)) +- `opentelemetry-instrumentation-starlette` Fix memory leak and double middleware + ([#3529](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3529)) - `opentelemetry-instrumentation-urllib3`: proper bucket boundaries in stable semconv http duration metrics ([#3518](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3518)) - `opentelemetry-instrumentation-urllib`: proper bucket boundaries in stable semconv http duration metrics diff --git a/instrumentation/opentelemetry-instrumentation-starlette/src/opentelemetry/instrumentation/starlette/__init__.py b/instrumentation/opentelemetry-instrumentation-starlette/src/opentelemetry/instrumentation/starlette/__init__.py index fa69fab5e3..e241c936a4 100644 --- a/instrumentation/opentelemetry-instrumentation-starlette/src/opentelemetry/instrumentation/starlette/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-starlette/src/opentelemetry/instrumentation/starlette/__init__.py @@ -177,6 +177,7 @@ def client_response_hook(span: Span, scope: dict[str, Any], message: dict[str, A from __future__ import annotations from typing import TYPE_CHECKING, Any, Collection, cast +from weakref import WeakSet from starlette import applications from starlette.routing import Match @@ -239,7 +240,7 @@ def instrument_app( meter_provider, schema_url="https://opentelemetry.io/schemas/1.11.0", ) - if not getattr(app, "is_instrumented_by_opentelemetry", False): + if not getattr(app, "_is_instrumented_by_opentelemetry", False): app.add_middleware( OpenTelemetryMiddleware, excluded_urls=_excluded_urls, @@ -251,11 +252,10 @@ def instrument_app( tracer=tracer, meter=meter, ) - app.is_instrumented_by_opentelemetry = True + app._is_instrumented_by_opentelemetry = True # adding apps to set for uninstrumenting - if app not in _InstrumentedStarlette._instrumented_starlette_apps: - _InstrumentedStarlette._instrumented_starlette_apps.add(app) + _InstrumentedStarlette._instrumented_starlette_apps.add(app) @staticmethod def uninstrument_app(app: applications.Starlette): @@ -300,7 +300,7 @@ class _InstrumentedStarlette(applications.Starlette): _server_request_hook: ServerRequestHook = None _client_request_hook: ClientRequestHook = None _client_response_hook: ClientResponseHook = None - _instrumented_starlette_apps: set[applications.Starlette] = set() + _instrumented_starlette_apps: WeakSet[applications.Starlette] = WeakSet() def __init__(self, *args: Any, **kwargs: Any): super().__init__(*args, **kwargs) @@ -331,9 +331,6 @@ def __init__(self, *args: Any, **kwargs: Any): # adding apps to set for uninstrumenting _InstrumentedStarlette._instrumented_starlette_apps.add(self) - def __del__(self): - _InstrumentedStarlette._instrumented_starlette_apps.discard(self) - def _get_route_details(scope: dict[str, Any]) -> str | None: """ diff --git a/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py b/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py index 484520c3ba..5c0f27c02b 100644 --- a/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py @@ -413,6 +413,15 @@ def test_no_op_tracer_provider(self): spans = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans), 0) + def test_manual_instrument_is_noop(self): + app = self._create_starlette_app() + self._instrumentor.instrument_app(app) + self.assertEqual(len(app.user_middleware), 1) + client = TestClient(app) + client.get("/foobar") + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 3) + def test_sub_app_starlette_call(self): """ !!! Attention: we need to override this testcase for the auto-instrumented variant