opentelemetry-instrumentation-starlette: fix memory leak and double middleware

Author: anuraaga

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:
     """

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