Add ASGI & WSGI instrument methods

[Kludex]

Is this what you had in mind? @alexmojaki

I don't like it.

[cloudflare-workers-and-pages]

Deploying logfire-docs with    Cloudflare Pages

Latest commit:	389c912
Status:	✅  Deploy successful!
Preview URL:	https://4fbd4907.logfire-docs.pages.dev
Branch Preview URL:	https://asgi-wsgi.logfire-docs.pages.dev

View logs

[alexmojaki]

Pretty much. What would you prefer? Just a different name?

[Kludex]

I don't like that every instrument_ method is returns None, but those 2 here need to return the app itself.

[alexmojaki]

I agree with that. The difference is more obvious in OTEL because it's not e.g. ASGIInstrumentor().instrument(app) like other things. We could maybe have instrumented_asgi but people might not notice the difference. If people copy the code from the docs then they'll probably get it right, but they might not do that.

If it was name logfire.asgi_middleware(app) or something else which didn't start with instrument_, but the rest was the same, how would you feel?

[Kludex]

If it was name logfire.asgi_middleware(app) or something else which didn't start with instrument_, but the rest was the same, how would you feel?

I think I'll prefer more if it was a class itself e.g. logfire.ASGIMiddleware(). Because then it's clear for those familiar with ASGI that it returns an ASGI application.

[alexmojaki]

But an ASGI application is just a callable, often a function.

[Kludex]

It's a callable, but I don't think it's usually a function. In any case, I think it's irrelevant if an app itself is usually a function or a class, but ASGI middlewsres in the ecosystem are usually presented as classes.

[alexmojaki]

Currently you can do this:

import logfire

logfire_instance = logfire.with_tags('foo')

logfire.instrument_bar()
# or
logfire_instance.instrument_bar()

Right now that's not very useful, but we may add other logfire.with_* methods. More importantly I want to eventually implement #195 which would allow logfire_instance = logfire.configure(..., global=False).

I want the instrument methods to be instances of the Logfire class so that they all support logfire_instance.instrument_bar(). If logfire.ASGIMiddleware is a class then logfire_instance.ASGIMiddleware will be an error, and the user will have to write app = logfire.ASGIMiddleware(app, logfire_instance=logfire_instance). That feels a bit sad. BUT maybe it's fine if we're already giving up on a name like logfire.instrument_asgi anyway. WDYT?

[Kludex]

I want the instrument methods to be instances of the Logfire class so that they all support logfire_instance.instrument_bar(). If logfire.ASGIMiddleware is a class then logfire_instance.ASGIMiddleware will be an error, and the user will have to write app = logfire.ASGIMiddleware(app, logfire_instance=logfire_instance). That feels a bit sad. BUT maybe it's fine if we're already giving up on a name like logfire.instrument_asgi anyway. WDYT?

Ah, I see. Then I think we can just have it as functions.

[alexmojaki]

Do you mean methods of the Logfire class?

[Kludex]

Do you mean methods of the Logfire class?

Yes.

[sydney-runkle]

@Kludex, ping me when this is ready for review, I'd love to take a look.

[Kludex]

I'll work on this today.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (4615e9d) to head (389c912).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #324   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          132       133    +1     
  Lines        10175     10206   +31     
  Branches      1395      1395           
=========================================
+ Hits         10175     10206   +31     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[alexmojaki]

The note here should refer to instrument_asgi, not OpenTelemetryMiddleware.

In fact we should probably just implement the env var ourselves.

[Kludex]

@alexmojaki something else here?

[alexmojaki]

Thanks!!! 🙏