fastapi: Fix uninstrument memory leak

[artemziborev]

Description

Fixes a memory leak in the FastAPIInstrumentor.uninstrument_app() method, where the FastAPI app instance was not being removed from the internal _instrumented_apps list. This caused a retained reference even after uninstrumentation, potentially resulting in memory usage growth over time.

Fixes #3683

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Added a regression test to ensure that after uninstrument_app() is called, the app is no longer present in the _instrumented_apps list.

Steps to reproduce:

1. Instrument a FastAPI app via FastAPIInstrumentor.instrument_app()
2. Uninstrument it via uninstrument_app()
3. Assert that the app is no longer tracked (i.e., removed from _instrumented_apps)
4. Confirm no side effects in other instrumentation logic

Tested using:

• Python 3.x
• tox
• pytest

Does This PR Require a Core Repo Change?

• Yes. - Link to PR:
• No.

Checklist:

• Followed the style guidelines of this project
• Changelogs have been updated
• Unit tests have been added
• Documentation has been updated (not required)

[artemziborev]

Hi! All checks are green — would love to get a review when you have time 🙌 Let me know if you'd like anything changed!

[emdneto]

Thanks. SGTM. We don't need the attributes fix for that PR. I suggest we minimize the scope of this PR only to the actual fix for FastAPI.

[emdneto]

Suggested change

	([#XXXX](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/XXXX))
	([#3683](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3683))

[emdneto]

thank you for the repro, but can you instead create an issue with that instead and remove from the PR?

[emdneto]

I believe a better way to test would be just to add this to somewhere in test_fastapi_instrumentation.py:

    def test_fastapi_app_is_collected_after_uninstrument(self):
        import gc
        import weakref
        app = fastapi.FastAPI()
        otel_fastapi.FastAPIInstrumentor.instrument_app(app)
        app_ref = weakref.ref(app)
        del app
        gc.collect()
        self.assertIsNone(app_ref())

[anuraaga]

Just a note related to #3683 (comment), it's probably better to remove uninstrument_app from this test, since it should still not leak

[emdneto]

Makes sense @anuraaga

[anuraaga]

Thanks @artemziborev - I had randomly noticed the same issue in starlette and fixed by using a WeakSet, which would allow avoiding all explicit remove()

b9a78e7#diff-a4901479b7a21ad4f5d45a7f3e47b2ada3f9d8ae4ae7f9e68c2001e8b8067611R303

I think it's more reliable to use WeakSet and recommend updating here too since there is still the chance of an app being GC'd without uninstrument being called. This instrumentation has the same __del__ hook as starlette used to

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py#L429

but it's essentially dead code since __del__ won't be called as long as the set() has a reference to it anyways.

[emdneto]

Thanks @artemziborev - I had randomly noticed the same issue in starlette and fixed by using a WeakSet, which would allow avoiding all explicit remove()

b9a78e7#diff-a4901479b7a21ad4f5d45a7f3e47b2ada3f9d8ae4ae7f9e68c2001e8b8067611R303

I think it's more reliable to use WeakSet and recommend updating here too since there is still the chance of an app being GC'd without uninstrument being called. This instrumentation has the same __del__ hook as starlette used to

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py#L429

but it's essentially dead code since __del__ won't be called as long as the set() has a reference to it anyways.

Yup. +1 if the author is open to making this change