Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Middleware for Starlette/FastAPI #710

Closed
hawkaa opened this issue May 19, 2020 · 5 comments
Closed

Middleware for Starlette/FastAPI #710

hawkaa opened this issue May 19, 2020 · 5 comments

Comments

@hawkaa
Copy link
Contributor

hawkaa commented May 19, 2020

Hi,

Is your feature request related to a problem?

I recently discovered FastAPI and I see a bright future for this project. I for sure will no longer use Flask to develop any micro backends anymore, as I find FastAPI better in all aspects.

No matter if you agree or not, I wonder if we need an OpenTelemetry middleware here. It does not exist at the moment, but I believe FastAPI is such of a big project that an integration should be provided by this package.

Describe alternatives you've considered

FastAPI builds on Starlette, so a middleware for Starlette will work with no problem for FastAPI as well. I have created a proof of concept middleware that looks like this:

from fastapi import FastAPI
from opentelemetry.trace import Tracer
from starlette.middleware.base import BaseHTTPMiddleware, RequestResponseEndpoint
from starlette.requests import Request


class OpenTelemetryMiddleware(BaseHTTPMiddleware):
    def __init__(self, app: FastAPI, tracer: Tracer):
        super().__init__(app)
        self._tracer = tracer

    async def dispatch(self, request: Request, call_next: RequestResponseEndpoint):
        span_name = str(request["path"])
        with self._tracer.start_as_current_span(span_name):
            return await call_next(request)

It is included in the FastAPI app like this:

from opentelemetry import trace
tracer = trace.get_tracer(__name__)
app.add_middleware(OpenTelemetryMiddleware, tracer=tracer)

However, as you probably can see, it doesn't read the headers and set the context correctly, as you have probably seen. Also, I'm not sure if it's the python-esque or opentelemetry way to include/inject the objects.

I'm thinking a combination of the opentelemetry-ext-flask package and the starlette-opentracing package is the right way to go.

Additional context
I'm no active open source contributor, and I don't have a lot of time on my hands, but I guess I could take a look at making such package if you are interested.

@toumorokoshi
Copy link
Member

toumorokoshi commented May 20, 2020

Thanks! I think agreed on the need, opentelemetry is not in the business of dictating frameworks, and we want enable telemetry wherever it's valuable

Regarding the approach: as fastapi / starlette have ASGI interfaces, I think we're pretty close with existing PRs. We have this one as a generic ASGI middleware:

#402

This provides most of what is desired in a span (start/end times, various http attributes). The missing component is the span name, which is typically defined to map to the route in the web framework itself.

To that end, there was a discussion around exposing a hook in the ASGI integration, and allowing updates to the span name there:

#402 (comment)

I would argue any focus we put on these tickets should work on delivering those (ASGI, and a light wrapper for fastapi/starlette that just utilizes ASGI and the hook mechanism to update name)

@hawkaa
Copy link
Contributor Author

hawkaa commented May 21, 2020

Good work. Yes, a generic ASGI middleware is definitely the way to go. I'll take a closer look at the code when I have a little more time.

I guess we can close this ticket?

@majorgreys
Copy link
Contributor

@hawkaa We recently merged and released the ASGI middleware from #716.

We can close this ticket.

@hawkaa hawkaa closed this as completed May 29, 2020
@toumorokoshi
Copy link
Member

Sorry, re-opening this issue, as the asgi instrumentation has landed (awesome!), but there's still some work to be done to make FastAPI / Starlette seamless.

@lzchen
Copy link
Contributor

lzchen commented Jun 22, 2020

I believe #777 addresses this.

@lzchen lzchen closed this as completed Jun 22, 2020
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this issue Nov 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants