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

[tuukkamustonen]

Description

When using OpenTelemetryConfig, during the application launch the logs get spammed with messages like:

An instrument with name http.server.duration, type Histogram, unit ms and description measures the duration of the inbound HTTP request has been created already.
An instrument with name http.server.response.size, type Histogram, unit By and description measures the size of HTTP response messages (compressed). has been created already.
An instrument with name http.server.request.size, type Histogram, unit By and description Measures the size of HTTP request messages (compressed). has been created already.
An instrument with name http.server.active_requests, type UpDownCounter, unit requests and description measures the number of concurrent HTTP requests that are currently in-flight has been created already.
...repeats...

This is because the underlying OpenTelemetryMiddleware (from opentelemetry-instrumentation-asgi) constructs metrics objects in init.

The metrics are added to the global registry, so re-doing the same will throw a warning as they already exist there.

A separate middleware instance is created per route. Is there way to avoid that? I don't see why it would be needed. When the middleware is specified at the application level (ie. Litestar(middleware=[...])) wouldn't a single instance do?

Is there a way around?

MCVE

This is sufficient, because `OpenAPIController` is initialized by default, and the middleware wraps also it:


import uvicorn
from litestar import Litestar
from litestar.contrib.opentelemetry import OpenTelemetryConfig
from opentelemetry import metrics
from opentelemetry.sdk.metrics import MeterProvider

metric_provider = MeterProvider()
metrics.set_meter_provider(metric_provider)

otel = OpenTelemetryConfig()

app = Litestar(middleware=[otel.middleware])

uvicorn.run(app)

Steps to reproduce

1. `python app.py`
2. See error

Litestar Version

2.5.1

Platform

• Linux
• Mac
• Windows
• Other (Please specify in the description above)

---

Note

While we are open for sponsoring on GitHub Sponsors and
OpenCollective, we also utilize Polar.sh to engage in pledge-based sponsorship.

Check out all issues funded or available for funding on our Polar.sh dashboard

• If you would like to see an issue prioritized, make a pledge towards it!
• We receive the pledge once the issue is completed & verified
• This, along with engagement in the community, helps us know which features are a priority to our users.

[cofin]

I believe this is related for additional context.

[provinzkraut]

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?

[tuukkamustonen]

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.

[provinzkraut]

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.

[tuukkamustonen]

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.

[provinzkraut]

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

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

[gsakkis]

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.