-
Notifications
You must be signed in to change notification settings - Fork 796
django: handle cancelled requests more cleanly #3848
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
base: main
Are you sure you want to change the base?
Conversation
except ImportError: | ||
from django.urls import Resolver404, resolve | ||
|
||
DJANGO_2_0 = django_version >= (2, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a look pyproject.toml instruments
and opentelemetry.instrumentation.django.package
, when you change supported versions you should also change them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I went in and removed references to DJANGO_2_0
, updated the pyproject.toml
instruments
and package
files.
) | ||
request_start_time = request.META.pop(self._environ_timer_key, None) | ||
|
||
if activation and span: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the case where this method is run, there's definitely an activation
and span
, so we don't need this if
block anymore
Alright I handled the Django 1.x deprecations across the package, and went ahead and added changelog entries. If you want for me to squash the commits I can, but it looks like maintainers are the ones managing commit creation based on the |
Description
Django's ASGI handler cancels tasks that have seen a disconnect. This manifests itself in requests potentially opening an OTel span through the Django middleware but not having
process_response
run in the existing mode, meaning theuse_span
(this one) closure + contextdetach
call isn't run.This results in dreaded "this token was created in a different context" errors, which happen because:
use_span
activation gets__exit__
'd inget_response
use_span
generator instead gets garbage collected after the factfinally
clause inside the context management code ofuse_span
detach
many miles away from the request contextThe current Django Middleware design dates back to supporting "old style" middleware, making this problem hard to solve cleanly. Given Django 1.x is now 5 years out of support, I think it's reasonable to drop Django 1.x support to make context management here clearer, through using the new-style
__call__
middleware along with atry/finally
block.Fixes # (issue)
This PR does some cleanup to the middleware to make sure that our context and span cleanup always happen, including when there's an
asyncio.CancelledError
during process handling. But this PR also drops support for Django 1.x in the process, by expecting this middleware to be called via__call__
(I'll handle doc changes etc if I can get approval on dropping Django 1.x support)
Type of change
How Has This Been Tested?
In order to see this "in the wild", I added the following sort of thing to a random Django view:
I then ran this app with
uvicorn
, and made requests. When I saw "sleeping" I would close the browser tabs, cancelling the requests. This would lead to the context error appearing.Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.