fastapi: fix wrapping of middlewares by adriangb · Pull Request #3012 · open-telemetry/opentelemetry-python-contrib

adriangb · 2024-11-15T15:31:01Z

MRE:

import anyio
import fastapi
import httpx
import uvicorn
from opentelemetry import trace
from opentelemetry.instrumentation.fastapi import FastAPIInstrumentor
from opentelemetry.sdk.resources import SERVICE_NAME, Resource
from opentelemetry.sdk.trace import TracerProvider
from opentelemetry.sdk.trace.export import BatchSpanProcessor, ConsoleSpanExporter

resource = Resource(attributes={SERVICE_NAME: '🐛'})

traceProvider = TracerProvider(resource=resource)
processor = BatchSpanProcessor(ConsoleSpanExporter())
traceProvider.add_span_processor(processor)
trace.set_tracer_provider(traceProvider)

app = fastapi.FastAPI()

@app.get('/ok')
async def ok() -> int:
    return 123

@app.get('/error')
async def error() -> int:
    raise Exception('An error occurred!')

FastAPIInstrumentor.instrument_app(app)


async def main() -> None:
    config = uvicorn.Config(app=app, port=9999, log_config=None)
    server = uvicorn.Server(config)

    async with anyio.create_task_group() as tg:
        tg.start_soon(server.serve)
        await anyio.sleep(1)  # Wait for the server to start
        async with httpx.AsyncClient(base_url='http://localhost:9999') as client:
            response = await client.get('/ok')
            assert response.status_code == 200, f'{response.status_code}: {response.text}'
            response = await client.get('/error')
            assert response.status_code == 500,  f'{response.status_code}: {response.text}'
            tg.cancel_scope.cancel()


if __name__ == '__main__':
    anyio.run(main)

With this change it shows up properly

Run this and you'll see that there is no "http.status_code": 500 in the logs.

codefromthecrypt

nice comment and test!

xrmx · 2024-11-19T08:34:26Z

@adriangb Please take a look at the failing tests

Kludex · 2024-11-20T08:25:30Z

fixes #795

adriangb · 2024-11-27T00:35:41Z

@adriangb Please take a look at the failing tests

@xrmx see #3012 (comment)

adriangb · 2024-11-27T00:37:28Z

@Kludex could you update the Starlette instrumentations after we finish up this PR? I'm assuming they have the same bugs.

Kludex · 2024-11-27T10:12:51Z

The pipeline has been running for some hours... 👀

vanyae-cqc · 2024-11-27T15:12:06Z

Hello! Just wondering if you'd expect this to enable getting the trace_id in a FastAPI exception_handler?

This has been discussed in more detail here: open-telemetry/opentelemetry-python#3477, but currently OpenTelemetryMiddleware's context seems to be removed by the time an exception gets to the ServerErrorMiddleware.

Would be great to enable users to report back a trace_id when they get an unexpected 500 status response. Have tried to verify this PR provides this by running it locally but haven't had any luck - could be something wrong in my setup though.

[Update]: Have now verified this achieves the above behaviour - this fix is greatly appreciated!

adriangb · 2024-11-27T16:19:52Z

Fixed issue with test shutdown hanging. All checks passing now.

xrmx

I trust you on the fastapi knowledge but would be nice to add a test to check that the change does what it's supposed to fix

adriangb · 2024-12-03T16:01:29Z

Ups looks like that got lost, I had added it in 4220b09

adriangb · 2024-12-03T16:05:40Z

Added back and updated the branch

xrmx · 2024-12-04T15:00:36Z

ICYMI the added test is red

adriangb · 2024-12-04T15:39:52Z

Thanks. I hadn't understood how the tests are parametrized and that _create_app is overridden in a subclass.

adriangb · 2024-12-13T15:18:57Z

@xrmx @lzchen can we get this merged please? I believe this is a major issue that deserves attention.

lzchen · 2025-01-10T17:23:56Z

@adriangb

I believe I still have an outstanding question

adriangb · 2025-01-23T14:38:21Z

@adriangb

I believe I still have an outstanding question

I've answered it. Please re-review.

outergod · 2025-05-15T07:26:24Z

Anything else in the way of merging now?

Signed-off-by: emdneto <[email protected]>

emdneto

@adriangb I fixed the changelog conflict and added this note:
opentelemetry-instrumentation-fastapi: Drop support for FastAPI versions earlier than 0.92

Signed-off-by: emdneto <[email protected]>

emdneto

@outergod @adriangb can you please fix the conflict?

adriangb · 2025-05-19T14:59:57Z

I gave it a shot, lets see what CI says

xrmx · 2025-05-19T15:04:05Z

@adriangb we merged a PR stopping using SpanAttributes in favor of semconv._incubating.attributes.*

adriangb · 2025-05-19T15:05:20Z

Ok sounds like this 2 year old PR now needs another round of adjusting for upstream changes 😢. Any chance you can make the change here since you're familiar with the implications?

Signed-off-by: emdneto <[email protected]>

alexmojaki · 2025-05-20T13:56:28Z

Does @codefromthecrypt need to approve?

xrmx · 2025-05-20T14:03:41Z

@alexmojaki github was saying so but your approval was good enough 😅

alexmojaki · 2025-05-20T14:04:58Z

Thanks everyone for getting this across!

adriangb · 2025-05-20T14:06:46Z

Amazing folks thanks so much!!

outergod · 2025-05-22T09:08:56Z

Finally! Thanks to everyone involved, especially @adriangb!

alexmojaki · 2025-07-16T13:25:50Z

+                if isinstance(
+                    inner_server_error_middleware, ServerErrorMiddleware
+                ):  # usually true
+                    outer_server_error_middleware = ServerErrorMiddleware(
+                        app=otel_middleware,
+                    )
+                else:
+                    # Something else seems to have patched things, or maybe Starlette changed.
+                    # Just create a default ServerErrorMiddleware.
+                    outer_server_error_middleware = ServerErrorMiddleware(
+                        app=otel_middleware
+                    )


these branches are the same now

I feel stupid for not realizing this when I changed the code

* fastapi: fix wrapping of middlewares * fix import, super * add test * changelog * lint * lint * fix * ci * fix wip * fix * fix * lint * lint * Exit? * Update test_fastapi_instrumentation.py Co-authored-by: Riccardo Magliocchetti <[email protected]> * remove break * fix * remove dunders * add test * lint * add endpoint to class * fmt * pr feedback * move type ignores * fix sphinx? * Update CHANGELOG.md * update fastapi versions * fix? * generate * stop passing on user-supplied error handler This prevents potential side effects, such as logging, to be executed more than once per request handler exception. * fix ci Signed-off-by: emdneto <[email protected]> * fix ruff Signed-off-by: emdneto <[email protected]> * remove unused funcs Co-authored-by: Emídio Neto <[email protected]> * fix lint,ruff Signed-off-by: emdneto <[email protected]> * fix changelog Signed-off-by: emdneto <[email protected]> * add changelog note Signed-off-by: emdneto <[email protected]> * fix conflicts with main Signed-off-by: emdneto <[email protected]> --------- Signed-off-by: emdneto <[email protected]> Co-authored-by: Riccardo Magliocchetti <[email protected]> Co-authored-by: Alexander Dorn <[email protected]> Co-authored-by: Emídio Neto <[email protected]>

adriangb requested a review from a team as a code owner November 15, 2024 15:35

Kludex approved these changes Nov 15, 2024

View reviewed changes

codefromthecrypt approved these changes Nov 19, 2024

View reviewed changes

lzchen reviewed Nov 19, 2024

View reviewed changes

Comment thread .../opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py Outdated

adriangb force-pushed the fix-asgi-middleware branch from e406be8 to 3d8dd4f Compare November 27, 2024 00:29

adriangb commented Nov 27, 2024

View reviewed changes

Comment thread instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py

adriangb requested a review from lzchen November 27, 2024 00:35

adriangb requested a review from codefromthecrypt November 27, 2024 00:36

xrmx reviewed Nov 27, 2024

View reviewed changes

Comment thread .../opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py

Comment thread instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py Outdated

adriangb requested a review from xrmx November 27, 2024 16:20

xrmx reviewed Dec 3, 2024

View reviewed changes

adriangb force-pushed the fix-asgi-middleware branch from 7497d32 to f825c76 Compare December 3, 2024 16:04

lzchen reviewed Dec 3, 2024

View reviewed changes

Comment thread .../opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py

adriangb requested review from lzchen and xrmx December 13, 2024 15:24

Kludex approved these changes Dec 18, 2024

View reviewed changes

Comment thread .../opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py

Comment thread instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py

emdneto added 2 commits May 16, 2025 10:20

Merge branch 'main' into fix-asgi-middleware

87aa3f6

fix changelog

2a150fd

Signed-off-by: emdneto <[email protected]>

emdneto approved these changes May 16, 2025

View reviewed changes

add changelog note

808d42a

Signed-off-by: emdneto <[email protected]>

emdneto reviewed May 19, 2025

View reviewed changes

Merge branch 'main' into fix-asgi-middleware

18b8962

emdneto and others added 2 commits May 19, 2025 12:27

fix conflicts with main

60e1706

Signed-off-by: emdneto <[email protected]>

Merge branch 'main' into fix-asgi-middleware

a648666

xrmx enabled auto-merge (squash) May 20, 2025 13:17

alexmojaki approved these changes May 20, 2025

View reviewed changes

xrmx merged commit 4d6893e into open-telemetry:main May 20, 2025
720 checks passed

github-project-automation Bot moved this from Approved PRs to Done in Python PR digest May 20, 2025

phillipuniverse mentioned this pull request May 28, 2025

FastAPI/Starlette - Inconsistent placement of the OpenTelemetryMiddleware #795

Closed

alexmojaki reviewed Jul 16, 2025

View reviewed changes

This was referenced Jul 16, 2025

FastAPI instrumentation: errors in hooks not handled properly #3642

Closed

FastAPI instrumentor stops recording exception event starting v0.55b0 #3637

Closed

lannuttia mentioned this pull request Aug 4, 2025

FastAPI Instrumentation Bypasses Middleware for Custom Router #3671

Closed

This was referenced Aug 15, 2025

REQUEST: New membership for @alexmojaki open-telemetry/community#2926

Closed

Rewrite FastAPI instrumentor middleware stack to be failsafe #3664

Merged

Kludex mentioned this pull request Aug 20, 2025

Don't pass bounded server_request_hook when using FastAPIInstrumentor.instrument(server_request_hook=...) #3701

Merged

Conversation

adriangb commented Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codefromthecrypt left a comment

Choose a reason for hiding this comment

Uh oh!

xrmx commented Nov 19, 2024

Uh oh!

Uh oh!

Kludex commented Nov 20, 2024

Uh oh!

Uh oh!

adriangb commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adriangb commented Nov 27, 2024

Uh oh!

Kludex commented Nov 27, 2024

Uh oh!

Uh oh!

Uh oh!

vanyae-cqc commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adriangb commented Nov 27, 2024

Uh oh!

xrmx left a comment

Choose a reason for hiding this comment

Uh oh!

adriangb commented Dec 3, 2024

Uh oh!

adriangb commented Dec 3, 2024

Uh oh!

Uh oh!

xrmx commented Dec 4, 2024

Uh oh!

adriangb commented Dec 4, 2024

Uh oh!

adriangb commented Dec 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lzchen commented Jan 10, 2025

Uh oh!

adriangb commented Jan 23, 2025

Uh oh!

outergod commented May 15, 2025

Uh oh!

emdneto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emdneto left a comment

Choose a reason for hiding this comment

Uh oh!

adriangb commented May 19, 2025

Uh oh!

xrmx commented May 19, 2025

Uh oh!

adriangb commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexmojaki commented May 20, 2025

Uh oh!

Uh oh!

xrmx commented May 20, 2025

Uh oh!

alexmojaki commented May 20, 2025

Uh oh!

adriangb commented May 20, 2025

Uh oh!

outergod commented May 22, 2025

Uh oh!

alexmojaki Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

outergod Jul 29, 2025

Choose a reason for hiding this comment

adriangb commented Nov 15, 2024 •

edited

Loading

adriangb commented Nov 27, 2024 •

edited

Loading

vanyae-cqc commented Nov 27, 2024 •

edited

Loading

adriangb commented Dec 13, 2024 •

edited

Loading

emdneto left a comment •

edited

Loading

adriangb commented May 19, 2025 •

edited

Loading