litestar

litestar copied to clipboard

So, as the title says. We need to consider how to best support OpenTelemetry. We might need to add some hooks to allow for optimal instrumentation. We should also consider creating an OT instrumentationn library, see for example: https://opentelemetry-python-contrib.readthedocs.io/en/latest/instrumentation/fastapi/fastapi.html

@cofin do you want to take this one?

Yep. I'll take a look at it over the weekend.

I've made some progress on a local branch. I'll submit a draft PR this evening after I write a few test cases.

Un-assigning for now until we dial in the instrumentation enhancements we've been discussing.

depends on #350

@cofin - are u doing this?

I had planned to, but I don't have the bandwidth currently. I can try to pick it back up soon or hand off.

It would be good if you could do this when you have bandwidth. We need this - and I too am somewhat busy 😉

Understood. I need it soon as well, so I'll make some time for it. I'll ping you on discord when I'm getting started.

@seladb you interested in picking this one perhaps?

Sure @Goldziher I think I can take it! Might take me some time though because I'm busy with other stuff at the moment

@Goldziher @cofin I looked up the OT integration with FastAPI and here is what I understand:

• The instrumentation class is implemented in the OT repo (and NOT in the FastAPI repo):
  • https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/init.py
• The way it's used is outside of the FastAPI app:

   import fastapi
   from opentelemetry.instrumentation.fastapi import FastAPIInstrumentor
  
   app = fastapi.FastAPI()
  
   @app.get("/foobar")
   async def foobar():
       return {"message": "hello world"}
  
   FastAPIInstrumentor.instrument_app(app)
  

• What the instrumentor does is add a middleware to the app:
  • https://github.com/open-telemetry/opentelemetry-python-contrib/blob/744851b3cf3a58471e44c3a20f143275fa16cdce/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/init.py#L184
• This is an API that Starlite doesn't have. Actually FastAPI doesn't have it either - it inherits it from the Starlette app:
  • https://github.com/encode/starlette/blob/858629f5188bc79d452600b1eb90eaa0045f6454/starlette/applications.py#L165
• Then the instrumentor adds a middleware that is implemented in the OT repo (and NOT in the FastAPI repo):
  • https://github.com/open-telemetry/opentelemetry-python-contrib/blob/744851b3cf3a58471e44c3a20f143275fa16cdce/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/init.py#L368
• It also maintains a list of all instrumented apps, I'm not sure for what purpose (looks like something internal to how OT works);
  • https://github.com/open-telemetry/opentelemetry-python-contrib/blob/744851b3cf3a58471e44c3a20f143275fa16cdce/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/init.py#L243

I think this design doesn't fit Starlite. Do you have any design in mind? What do you think should be implemented on the Starlite side what on the OT side?

Hmm, we should start with opening an issue in the open telemtry app.

As for what to do - you can see this draft PR of mine in the sentry repository, it does basically the same: https://github.com/getsentry/sentry-python/pull/1597

@Goldziher I have to say I don't fully understand the architecture of these integrations (both OT and Sentry) 😕

Wouldn't it be easier to add an "OT plugin" or "Sentry plugin" to the Starlite repo that handles the instrumentation (e.g add a middleware, listen to relevant events, etc.)? I'm not familiar with our current plugin system, but it sounds feasible...

Or maybe expose Starlite's plugin in easy-to-use APIs that will provide everything these integrations need?

I assume there are solid answers to why these suggestions are not feasible, so I apologize in advance for my lack of knowledge...

I started work on this @seladb --> https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1388

If you find the time, you're welcome to help with this. Tests are not in place yet etc.

Thanks @Goldziher ! I can see you're on top of this so I can probably work on some other tasks 😃 Let me know if you need my help, but TBH I still don't really understand the design...

Hello 👋 ,

I am one of the co-maintainers of OpenTelemetry. The instrumentations are maintained in our repo for various reasons. We could provide some value fast without putting in a lot of effort to convince and implement the tracing directly in third-party libraries. Ideally, the direction we would like to see in the long term is the support baked directly, and users who install the SDK will have the real spans out of the box.

I think it's best for starlite to ship the support directly with some middleware or plugin mechanism. I am not very familiar with the starlite internal but I would be willing to review the OTEL parts if you decide to implement the changes here.

Fwiw, the falcon also showed interest in adding support directly https://github.com/falconry/falcon/issues/1828. If package authors adopt OTEL gradually, we can start deprecating the instrumentation package that monkey patch the original packages, which is in the best interest of everyone.

Fwiw, the falcon also showed interest in adding support directly https://github.com/falconry/falcon/issues/1828. If package authors adopt OTEL gradually, we can start deprecating the instrumentation package that monkey patch the original packages, which is in the best interest of everyone.

Hi, sure this would be easier than finishing the OTEL instrumentation . We should make it a separate package in that case.

@srikanthccv - can you assist with reviewing the PR linked above?

@Goldziher, I took a quick look at the pull request. I will review it again tomorrow when I am on my computer. I see that you are extending the ASGI instrumentation middleware. Are your contrib components also follow the rules of semver? because the upstream middleware is still in beta and can possibly introduce breaking changes as it gets improved.

@Goldziher, I took a quick look at the pull request. I will review it again tomorrow when I am on my computer. I see that you are extending the ASGI instrumentation middleware. Are your contrib components also follow the rules of semver? because the upstream middleware is still in beta and can possibly introduce breaking changes as it gets improved.

Hi, we are following semver of course. I am not actually extending the middleware- I am wrapping it. The middleware internals are not touched.