Bug: An instrument with name ... has been created already (as OTEL middleware gets initialized per each handler) about litestar HOT 7 CLOSED

This is an interesting case because it shows that there are a few different interpretations about how middlewares should behave.

Reading the issue @cofin linked, it seems to me that the assumption over there is that middlewares seen as long-lived objects that have state. Whereas Litestar (intentionally) treats them as ephemeral. This assumption, for Litestar, means that it's okay to instantiate middlewares repeatedly. The reason it does this is because Litestar only has 2 callable ASGI application units: The root app and the routes themselves. The root application only serves to dispatch the routes, with the routes handling everything else ASGI, including all the middlewares, which also means that every route gets its own middleware stack (among other things).

What this means though is that if we want to explicitly support stateful middleware, we'd have to rework basically our entire application stack.

I think a better way to solve this would be to:

• Add a custom OpenTelemetry instrumentation middleware that's stateless
• Change our OpenTelemetryInstrumentationMiddleware such that it acts as a singleton bound to an application instance

Adding more context from this comment open-telemetry/opentelemetry-python-contrib#1335 (comment):

Yes, the warning is from the creation of metric data models, and there is nothing wrong with initialising the middleware again and again. The model creation method throws a warning if there is already an existing data model with the same name, unit and detail and does not create another, which is the expected behaviour.
And I agree it is weird from a user's point of view to get a warning if nothing is wrong from their side. To avoid this, we can add a check for the existing data model if it exists before creating or else we can also have an API like 'get_histogram', which will create one if it doesn't exist or it'll return the existing one.

This reads to me like they agree with my Litestar's perspective on middleware and that this isn't actually a bug but valid behaviour. Seeing as how fixing it on their side also has been discussed (and we're not the only framework affected by this), I'm inclined to close this as "not a bug" and mark it as an upstream issue.

@litestar-org/maintainers?

from litestar.

Here's my current workaround, season to taste:

import copy
from typing import ClassVar

from litestar.contrib.opentelemetry import (
    OpenTelemetryConfig,
    OpenTelemetryInstrumentationMiddleware,
)
from litestar.middleware import AbstractMiddleware
from litestar.types import ASGIApp
from opentelemetry.instrumentation.asgi import OpenTelemetryMiddleware

class OpenTelemetrySingletonMiddleware(OpenTelemetryInstrumentationMiddleware):
    __open_telemetry_middleware__: ClassVar[OpenTelemetryMiddleware]

    def __init__(self, app: ASGIApp, config: OpenTelemetryConfig) -> None:
        cls = self.__class__
        if singleton_middleware := getattr(cls, "__open_telemetry_middleware__", None):
            AbstractMiddleware.__init__(
                self,
                app,
                scopes=config.scopes,
                exclude=config.exclude,
                exclude_opt_key=config.exclude_opt_key,
            )
            self.open_telemetry_middleware = copy.copy(singleton_middleware)
            self.open_telemetry_middleware.app = app
        else:
            super().__init__(app, config)
            cls.__open_telemetry_middleware__ = self.open_telemetry_middleware

EDIT: I updated the original non-working version, this seems to be working.

from litestar.

I believe this is related for additional context.

from litestar.

Yeah, that's basically what I was suggesting here:

Change our OpenTelemetryInstrumentationMiddleware such that it acts as a singleton bound to an application instance

from litestar.

What this means though is that if we want to explicitly support stateful middleware, we'd have to rework basically our entire application stack.

From my perspective the goal would be to avoid loss of CPU cycles / watsing RAM having to re-build the middleware over and over again. State is sort of a side-effect.

Following the discussion and links, Starlette seems to have patched this (build once and lazily).

Yes, the warning is from the creation of metric data models, and there is nothing wrong with initialising the middleware again and again.

This is from the perspective of that (OTEL) middleware, I don't think it takes a stance towards the idea and behavior of middlewares, in general.

from litestar.

From my perspective the goal would be to avoid loss of CPU cycles / watsing RAM having to re-build the middleware over and over again. State is sort of a side-effect.

If being stateful is the intention or just a side-effect doesn't really make a difference regarding how we would have to implement this. Litestar currently does not have the concept of a global middleware stack, as I laid out in my comment above. We'd still have to change that.

Following the discussion and links, Starlette seems to have patched this (build once and lazily).

Starlette does have multiple "active layers" though, so it's not that fundamental of a change for them to make. I think for now we'll just have to accept that this is a limitation of Litestar's architecture (although I'd argue that it's not really a limitation but rather just Litestar being strict about what kind of patterns it supports)

This is from the perspective of that (OTEL) middleware, I don't think it takes a stance towards the idea and behavior of middlewares, in general.

But Litestar does 🙂

IMO, since middlewares are just ASGI apps, they should behave like ASGI apps as well. That is, being basically stateless, functional components. They take in a scope and the two server callables and act on them. If they have requirements that exceed this, the burden is on them to handle this. The upside of restricting middlewares to this pattern is that it's highly adaptable, simple, performant, and doesn't require other middlewares to know anything about anything but themselves.

from litestar.

A draft idea, would this work:

class OpenTelemetryInstrumentationMiddleware(AbstractMiddleware):

    def __init__(self, app: ASGIApp, config: AbstractMiddlewareConfig, middleware: OpenTelemetryMiddleware) -> None:
        super().__init__(app=app, scopes=config.scopes, exclude=config.exclude, exclude_opt_key=config.exclude_opt_key)
        self.open_telemetry_middleware = middleware

Ie. construct the actual middleware class outside and pass it here. The OpenTelemetryInstrumentationMiddleware would act mostly as a proxy to the actual middleware. Maybe the whole class would turn useless then... don't know, gotta think/try.

from litestar.