dabapps / django-log-request-id

Django middleware and log filter to attach a unique ID to every log message generated as part of a request
BSD 2-Clause "Simplified" License
371 stars 64 forks source link

Thoughts abt how this works when ASGI rolls out in 3.2? #45

Closed simkimsia closed 2 years ago

JonasKs commented 4 years ago

I tried to e-mail @j4mie directly about this since I didn't want to step on any toes and promote another(mine) package in here, but unfortunately I never got a reply.

The solution for passing values through async (and sync with asgiref) in Django>=3.1.1 is ContextVars. I've created a similar package to this one (for pre-Django 3 as well) called Django-GUID, which now has a full rewrite using ContextVars for Django>=3.1. You can use this for both sync and async Django>=3.1.1, which obviously means it works for Django3.2 and above. :)

Most people using packages like Django-GUID or django-log-request-id have waited to port to Django3.1 and async because they would lose their correlation-id/request-id, so after a few days of waiting I've decided to comment on this issue. I hope that @j4mie will be excited to see that there is another project that supports newer versions of Django already out there. I assume a solution was never found, since this issue is inactive.

Have a good weekend!

j4mie commented 4 years ago

It's not that I never replied @JonasKs, I just haven't replied yet! You only sent the email two days ago.. I'll get back to you next week 🙂

I'm happy for you to "promote" your package here - open source is not a competition! If someone wants to use your package instead of this one, great!

The simple reason I haven't responded to this issue yet is that I haven't had time. Async Django doesn't interest me all that much as I don't tend to work on the ~1% of applications it's appropriate for. I'll probably get round to researching this issue at some point, and I'm very happy to accept pull requests along these lines.

JonasKs commented 4 years ago

Ah, I'm really happy you took it this way then, and I'm sorry if it seemed agressive, it wasn't my intention. I really wanted to do the right thing and e-mail you first.

open source is not a competition!

Nothing else to say than this: :heart:

Async Django doesn't interest me all that much as I don't tend to work on the ~1% of applications it's appropriate for

That makes sense! Personally I use Django to connect other backend systems and I'm not a heavy user of the ORM so I might be the 1% - which is why I really tried to find a solution for this. If you see this issue it took me a good while :) Andrew Godwin's last talk at PyCon AU made it all click, fortunately.

simkimsia commented 4 years ago

@JonasKs how is your django-guid different from this package?

Also does it work with django 2.2?

If I should repost this question at your package I be happy to ask it there

JonasKs commented 4 years ago

Posting here is fine by me! :)

how is your django-guid different from this package?

For Django2.2:
Django-GUID and this package is pretty much the same, but with a bit different implementation. Django-GUID stored the "request ID" to an object, and access it by using the thread ID. The ID is then cleaned up using signals on request_finish so it don't leak over to the next request, where this uses thread.local's. The django-guid==2.2.0 version will work with Django2.2 and sync/WSGI.

For Django3.1 and 3.2:
For ASGI and async which is the "big new thing" to Django, both this package and Django-GUID had approaches that won't work any more, since many requests will be processed on the same thread asynchronously. We now have to use ContextVars. This is what's implemented in the newest release of Django-GUID. So for Django 3.1 and 3.2, Django-GUID version 3.0.0 will work, both for WSGI/sync and for ASGI/async.

Also does it work with django 2.2?

Yes, Django-GUID version 2.2.0 works with Django 2.2, and so does this :)

j4mie commented 3 years ago

Had a stab at this: #53

Anyone who understands this stuff want to take a look at it for me? 😆

j4mie commented 2 years ago

This can be closed now as #53 is merged.