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

Django instrumentation silently doesn't work for ASGI requests #472

Closed
alexmojaki opened this issue Oct 4, 2024 · 7 comments
Closed

Django instrumentation silently doesn't work for ASGI requests #472

alexmojaki opened this issue Oct 4, 2024 · 7 comments
Assignees
Labels
bug Bug related to the Logfire Python SDK

Comments

@alexmojaki
Copy link
Contributor

alexmojaki commented Oct 4, 2024

A user couldn't get the Django instrumentation working (https://pydanticlogfire.slack.com/archives/C06EDRBSAH3/p1722460460137619?thread_ts=1722440635.372479&cid=C06EDRBSAH3). With difficulty we realised that it was because they were using ASGI. In opentelemetry/instrumentation/django/middleware/otel_middleware.py you can see that ASGI requests are silently ignored when opentelemetry-instrumentation-asgi isn't installed.

Firstly, we should document this loudly.

Then for people who miss this point, we should either:

  1. Try to detect ASGI requests and warn the user if needed. Not sure how easy this is.
  2. Add a mandatory is_asgi parameter to instrument_django to force the user to tell us the situation, then raise an error if needed.
  3. Always install opentelemetry-instrumentation-asgi with the django extra.
  4. Remove the django extra and replace it with django-wsgi and django-asgi.

@samuelcolvin what do you think?

@sydney-runkle sydney-runkle added the bug Bug related to the Logfire Python SDK label Nov 5, 2024
@Kludex
Copy link
Member

Kludex commented Nov 12, 2024

Remove the django extra and replace it with django-wsgi and django-asgi.

We can also just add django-async.

@Kludex
Copy link
Member

Kludex commented Nov 12, 2024

The first step here is to document this.

@Kludex Kludex self-assigned this Nov 12, 2024
@alexmojaki
Copy link
Contributor Author

Remove the django extra and replace it with django-wsgi and django-asgi.

We can also just add django-async.

The idea is that the user has to be aware of the distinction and make an explicit choice. If they don't check docs carefully they may easily think that pip install logfire[django] should just work, they won't get an error at any point.

@Kludex
Copy link
Member

Kludex commented Nov 27, 2024

@Kludex
Copy link
Member

Kludex commented Nov 27, 2024

The easiest here is to add opentelemetry-instrumentation-asgi to the django extras...

@Kludex
Copy link
Member

Kludex commented Nov 27, 2024

In the meantime, while we don't solve this... I've added a note to the docs: #630

@Kludex
Copy link
Member

Kludex commented Nov 28, 2024

We now have a note on the top of the Django page about this:

Screenshot 2024-11-28 at 12 48 48

@Kludex Kludex closed this as completed Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug related to the Logfire Python SDK
Projects
None yet
Development

No branches or pull requests

3 participants