Teach fastapi instrumentation about fastapi-slim

[zhihali]

Description

Fixes #2683

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

test with running the fastapi locally with the updated code

Does This PR Require a Core Repo Change?

• Yes. - Link to PR:
• No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

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

[zhihali]

@xrmx Thanks for the review, always appreciate your quick reply and guidance!

[xrmx]

LGTM, I would just add a test requirements file with fastapi-slim installed so we check that we support that

[xrmx]

I would not add new tox environment, just add a test requirement for the current fastapi one Il lun 15 lug 2024, 16:12 Emídio Neto ***@***.***> ha scritto:

…

***@***.**** commented on this pull request. ------------------------------ In tox.ini <#2702 (comment)> : > @@ -548,6 +549,12 @@ commands_pre = fastapi: pip install opentelemetry-sdk@{env:CORE_REPO}\#egg=opentelemetry-sdk&subdirectory=opentelemetry-sdk fastapi: pip install opentelemetry-test-utils@{env:CORE_REPO}\#egg=opentelemetry-test-utils&subdirectory=tests/opentelemetry-test-utils fastapi: pip install -r {toxinidir}/instrumentation/opentelemetry-instrumentation-fastapi/test-requirements.txt + fastapi-slim: pip install opentelemetry-api@{env:CORE_REPO}\#egg=opentelemetry-api&subdirectory=opentelemetry-api + fastapi-slim: pip install opentelemetry-semantic-conventions@{env:CORE_REPO}\#egg=opentelemetry-semantic-conventions&subdirectory=opentelemetry-semantic-conventions + fastapi-slim: pip install opentelemetry-sdk@{env:CORE_REPO}\#egg=opentelemetry-sdk&subdirectory=opentelemetry-sdk + fastapi-slim: pip install opentelemetry-test-utils@{env:CORE_REPO}\#egg=opentelemetry-test-utils&subdirectory=tests/opentelemetry-test-utils + fastapi-slim: pip install -r {toxinidir}/instrumentation/opentelemetry-instrumentation-fastapi/test-requirements-slim.txt Are we missing running this on CI? — Reply to this email directly, view it on GitHub <#2702 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAADFBCZYXIN3QPYWFSPV5LZMPKDHAVCNFSM6AAAAABKYX3OZSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNZXHAZTIMBYGI> . You are receiving this because you were mentioned.Message ID: <open-telemetry/opentelemetry-python-contrib/pull/2702/review/2177834082@ github.com>

[zhihali]

I would not add new tox environment, just add a test requirement for the current fastapi one

Hey @xrmx, I think we should create a new tox environment for this. The issue is that FastAPI-Slim uses the same import statement as FastAPI (import fastapi). This means we can't just add a new test file under the current FastAPI environment and expect it to work correctly with FastAPI-Slim.

To properly test FastAPI-Slim, we need to:

1. Create a new tox environment.
2. Update the requirements.txt to include FastAPI-Slim but not FastAPI.
3. Run the FastAPI instrumentation tests in this new environment.
   We should also add this new environment to our CI pipeline.

[xrmx]

@zhihali So the issue is that fastapi and fastapi-slim does not conflict with each other and so installing one will not remove the other. Right?

[zhihali]

@zhihali So the issue is that fastapi and fastapi-slim does not conflict with each other and so installing one will not remove the other. Right?

@xrmx
So, the original issue is some user would like to use the fastapi-slim, and use the otel-fastapi-instrumentation, but our instrumentation doesn't support it. I add the support for fastapi-slim. This should solve the issue. In this case, I supporse add a new test env and a CI for fastapi-slim is reasonable.

But for now, I think we solve the OR problem, for AND, this is a bit complex.

Python's import system will typically use the package that was installed last or the one that appears first in the Python path.

So it's defo not recommand to use both fastapi and fastapi-slim(since the import all be import fastapi). I think the current change will support AND, but it's not recommand to use it.

What's your opinion? Any further discussion is welcomed!

[xrmx]

@zhihali So the issue is that fastapi and fastapi-slim does not conflict with each other and so installing one will not remove the other. Right?

@xrmx So, the original issue is some user would like to use the fastapi-slim, and use the otel-fastapi-instrumentation, but our instrumentation doesn't support it. I add the support for fastapi-slim. This should solve the issue. In this case, I supporse add a new test env and a CI for fastapi-slim is reasonable.

But for now, I think we solve the OR problem, for AND, this is a bit complex.

Python's import system will typically use the package that was installed last or the one that appears first in the Python path.

So it's defo not recommand to use both fastapi and fastapi-slim(since the import all be import fastapi). I think the current change will support AND, but it's not recommand to use it.

What's your opinion? Any further discussion is welcomed!

I was talking specifically to the inability to use the same tox environment, sorry for not being more clear. PR is fine for me