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

logfire.instrument_*() methods #193

Closed
alexmojaki opened this issue May 19, 2024 · 8 comments
Closed

logfire.instrument_*() methods #193

alexmojaki opened this issue May 19, 2024 · 8 comments
Assignees

Comments

@alexmojaki
Copy link
Contributor

e.g. logfire.instrument_sqlalchemy() instead of from opentelemetry.instrumentation.sqlalchemy import SQLAlchemyInstrumentor; SQLAlchemyInstrumentor().instrument(). Same for all integrations documented in https://docs.pydantic.dev/logfire/integrations/#opentelemetry-integrations.

@alexmojaki
Copy link
Contributor Author

alexmojaki commented May 22, 2024

Remaining methods to add:

  • logfire.instrument_flask()
  • logfire.instrument_starlette()
  • logfire.instrument_aiohttp()
  • logfire.instrument_sqlalchemy()
  • logfire.instrument_pymongo()
  • logfire.instrument_redis()
  • logfire.instrument_celery()
  • logfire.instrument_asgi_middleware()
  • logfire.instrument_wsgi_middleware()
  • logfire.instrument_logging_handler()
  • logfire.instrument_loguru_handler()
  • logfire.instrument_structlog_processor()

I'm not sure about the names of the last 5. It is important that they're methods of logfire objects rather than classes imported from logfire. It would be nice for everything to start with instrument_ but instrument_logging_handler sounds like it's instrumenting a logging handler, rather than returning a handler that 'instruments logging'.

@Kludex I'm going to start working on the first 6, please do the other half.

@alexmojaki
Copy link
Contributor Author

@Kludex I think we should have logfire.instrument_loguru() which calls logger.add(**logfire.loguru_handler()). That makes the naming less weird and should be easier for users, avoiding problems like #228. logfire.loguru_handler() can be kept for compatibility but not recommended.

@Kludex
Copy link
Member

Kludex commented Jul 15, 2024

Is this done? @alexmojaki

@alexmojaki
Copy link
Contributor Author

No because I did half and I asked you to do the other half.

@Kludex
Copy link
Member

Kludex commented Jul 19, 2024

About the ASGI and WSGI, I'm not sure if it makes sense to create those methods.

Maybe wrapping them, and providing a logfire.InstrumentASGIMiddleware and logfire.InstrumentWSGIMiddleware makes more sense. wdyt @alexmojaki ?

@alexmojaki
Copy link
Contributor Author

They need to be methods so that you can use them with different logfire objects that have different configs.

We could maybe just call them instrument_asgi/wsgi. Then the docs would suggest app = instrument_asgi(app) which doesn't look weird. It's only 'middleware' in the context of higher level frameworks which choose that terminology. For example the starlette OTEL instrumentation has app.add_middleware(OpenTelemetryMiddleware, ...). But we don't really have to call it that.

@Kludex
Copy link
Member

Kludex commented Nov 13, 2024

The asgi and wsgi were merged. Do we need the logging ones as well here?

@alexmojaki
Copy link
Contributor Author

nah

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants