Skip to content

Add ASGI support to Django#2656

Merged
majorgreys merged 11 commits intoDataDog:masterfrom
Kyle-Verhoog:django-asgi
Aug 9, 2021
Merged

Add ASGI support to Django#2656
majorgreys merged 11 commits intoDataDog:masterfrom
Kyle-Verhoog:django-asgi

Conversation

@Kyle-Verhoog
Copy link
Member

@Kyle-Verhoog Kyle-Verhoog commented Jul 16, 2021

Motivation

In Django 3.1 the internal implementation was modified to follow a completely different codepath for async requests. Our integration only patches get_response and not get_response_async which means that the top level django.request span doesn't get generated.

Solution

Using the ASGI middleware we can relatively cleanly add async/ASGI
support for Django.

The previous method of instrumentation was first attempted but it was
decided against for a few reasons:

  - Tracing the ASGI application would require more than just wrapping
    the get_response_async method (although this would provide most of
    the visibility desired) as we'd need to trace the invocation of the
    response method.
  - We already have an ASGI middleware implementation.
  - The ASGI middleware provides more accurate timings as it is able to run 
    before the Django application.

Builds off of #1812

This was referenced Jul 21, 2021
@Kyle-Verhoog Kyle-Verhoog force-pushed the django-asgi branch 3 times, most recently from 2c66fbd to df3694f Compare July 26, 2021 19:23
@Kyle-Verhoog Kyle-Verhoog force-pushed the django-asgi branch 2 times, most recently from 52d3338 to db445e7 Compare July 27, 2021 13:46
Using the ASGI middleware we can relatively cleanly add async/ASGI
support for Django.

The previous method of instrumentation was first attempted but it was
decided against for a couple reasons:

  - Tracing the ASGI application would require more than just wrapping
    the get_response_async method (although this would provide most of
    the visibility desired) as we'd need to trace the invocation of the
    response method.
  - We already have an ASGI middleware implementation.
@Kyle-Verhoog Kyle-Verhoog marked this pull request as ready for review July 27, 2021 18:13
@Kyle-Verhoog Kyle-Verhoog requested a review from a team as a code owner July 27, 2021 18:13
@majorgreys
Copy link
Contributor

I was wondering whether we could use asgiref.sync.sync_to_async in cases like this to reduce the repeated code here:

https://docs.djangoproject.com/en/3.2/topics/async/#asgiref.sync.sync_to_async

@mergify
Copy link
Contributor

mergify bot commented Jul 28, 2021

@Kyle-Verhoog this pull request is now in conflict 😩

@mergify mergify bot added conflict and removed conflict labels Jul 28, 2021
@Kyle-Verhoog
Copy link
Member Author

@majorgreys looking at the implementation of sync_to_async it seems as though it'll introduce quite a bit of overhead unfortunately

Copy link
Contributor

@mabdinur mabdinur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from Gab's comments LGTM

@mergify
Copy link
Contributor

mergify bot commented Jul 29, 2021

@Kyle-Verhoog this pull request is now in conflict 😩

@mergify mergify bot added the conflict label Jul 29, 2021
@Kyle-Verhoog Kyle-Verhoog added this to the 0.52.0 milestone Aug 5, 2021
@codecov-commenter
Copy link

codecov-commenter commented Aug 6, 2021

Codecov Report

❌ Patch coverage is 50.00000% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.93%. Comparing base (c695018) to head (495588a).

Files with missing lines Patch % Lines
ddtrace/contrib/django/_asgi.py 46.66% 8 Missing ⚠️
ddtrace/contrib/django/patch.py 56.25% 7 Missing ⚠️
tests/contrib/django/asgi.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2656      +/-   ##
==========================================
- Coverage   84.95%   84.93%   -0.03%     
==========================================
  Files         590      591       +1     
  Lines       43083    43113      +30     
==========================================
+ Hits        36601    36617      +16     
- Misses       6482     6496      +14     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

brettlangdon
brettlangdon previously approved these changes Aug 6, 2021
@majorgreys
Copy link
Contributor

If I'm understanding correctly, enabling the ASGI middleware will generate a trace with two django.request spans with some variation on the tags:

https://github.com/DataDog/dd-trace-py/blob/e85e959fdd966692b93f4fb8384672524471ce8c/tests/snapshots/tests.contrib.django.test_django_snapshots.test_asgi_200_30.snap#L21-L38

vs.

https://github.com/DataDog/dd-trace-py/blob/e85e959fdd966692b93f4fb8384672524471ce8c/tests/snapshots/tests.contrib.django.test_django_snapshots.test_asgi_200_30.snap#L1-L20

I'm concerned that this could create confusion to have two django-request span.

Can we have just one?

@Kyle-Verhoog
Copy link
Member Author

Oh good catch @majorgreys, I missed that! Yeah we should add checks to only do the middleware if get_response_async is available. What's happening here is that we're essentially double patching because get_response is being used for async

majorgreys
majorgreys previously approved these changes Aug 9, 2021
brettlangdon
brettlangdon previously approved these changes Aug 9, 2021
@brettlangdon brettlangdon dismissed stale reviews from majorgreys and themself via 495588a August 9, 2021 15:54
@majorgreys majorgreys merged commit 71ca68d into DataDog:master Aug 9, 2021
@majorgreys
Copy link
Contributor

@Mergifyio backport 0.51

mergify bot pushed a commit that referenced this pull request Aug 9, 2021
* Add ASGI support to Django

Using the ASGI middleware we can relatively cleanly add async/ASGI
support for Django.

The previous method of instrumentation was first attempted but it was
decided against for a couple reasons:

  - Tracing the ASGI application would require more than just wrapping
    the get_response_async method (although this would provide most of
    the visibility desired) as we'd need to trace the invocation of the
    response method.
  - We already have an ASGI middleware implementation.

* Store span in ASGI scope

* Don't override distributed tracing setting

It gets reused by the ASGI middleware

* Don't wrap the asgi application when get_response is used for async

* Update ddtrace/contrib/django/patch.py

Co-authored-by: Tahir H. Butt <tahir.butt@datadoghq.com>
Co-authored-by: Brett Langdon <me@brett.is>
(cherry picked from commit 71ca68d)

# Conflicts:
#	tests/contrib/django/asgi.py
#	tests/snapshots/tests.contrib.django.test_django_snapshots.test_asgi_200_30.snap
#	tests/snapshots/tests.contrib.django.test_django_snapshots.test_asgi_200_31.snap
#	tests/snapshots/tests.contrib.django.test_django_snapshots.test_asgi_200_3x.snap
#	tests/snapshots/tests.contrib.django.test_django_snapshots.test_asgi_500_30.snap
#	tests/snapshots/tests.contrib.django.test_django_snapshots.test_asgi_500_31.snap
#	tests/snapshots/tests.contrib.django.test_django_snapshots.test_asgi_500_3x.snap
@mergify
Copy link
Contributor

mergify bot commented Aug 9, 2021

Command backport 0.51: success

Backports have been created

mergify bot added a commit that referenced this pull request Aug 10, 2021
* Add ASGI support to Django

Using the ASGI middleware we can relatively cleanly add async/ASGI
support for Django.

The previous method of instrumentation was first attempted but it was
decided against for a couple reasons:

  - Tracing the ASGI application would require more than just wrapping
    the get_response_async method (although this would provide most of
    the visibility desired) as we'd need to trace the invocation of the
    response method.
  - We already have an ASGI middleware implementation.

* Store span in ASGI scope

* Don't override distributed tracing setting

It gets reused by the ASGI middleware

* Don't wrap the asgi application when get_response is used for async

* Update ddtrace/contrib/django/patch.py

Co-authored-by: Tahir H. Butt <tahir.butt@datadoghq.com>
Co-authored-by: Brett Langdon <me@brett.is>
(cherry picked from commit 71ca68d)

# Conflicts:
#	tests/contrib/django/asgi.py
#	tests/snapshots/tests.contrib.django.test_django_snapshots.test_asgi_200_30.snap
#	tests/snapshots/tests.contrib.django.test_django_snapshots.test_asgi_200_31.snap
#	tests/snapshots/tests.contrib.django.test_django_snapshots.test_asgi_200_3x.snap
#	tests/snapshots/tests.contrib.django.test_django_snapshots.test_asgi_500_30.snap
#	tests/snapshots/tests.contrib.django.test_django_snapshots.test_asgi_500_31.snap
#	tests/snapshots/tests.contrib.django.test_django_snapshots.test_asgi_500_3x.snap

Co-authored-by: Kyle Verhoog <kyle@verhoog.ca>
Co-authored-by: Brett Langdon <brett.langdon@datadoghq.com>
@Kyle-Verhoog Kyle-Verhoog deleted the django-asgi branch August 16, 2021 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No "django.request" traces being captured in Django 3.1

6 participants