[django] support for Django Rest Framework #389

gabsn · 2017-12-19T17:07:22Z

This PR is an integration for the rest_framework django app.
Note: Only DRF >= 3.4 is supported.

When the user was using a custom exception handler, the exception was handled before it comes to our TraceExceptionMiddleware.
Now we patch handle_exception used in rest_framework to trace those exceptions.

Before

After

gabsn · 2017-12-19T17:12:57Z

tests/contrib/django/restframework/settings.py

+        'rest_framework.permissions.IsAdminUser',
+    ],
+
+    'EXCEPTION_HANDLER': 'restframework.exceptions.custom_exception_handler'


Here I define the custom exception handler.

gabsn · 2017-12-19T17:13:50Z

tests/contrib/django/restframework/tests.py

+        sp = spans[0]
+        eq_(sp.name, 'django.request')
+        eq_(sp.resource, 'restframework.views.UserViewSet')
+        eq_(sp.error, 1)


This test would break without the rest_framework integration, and that's what we want.

gabsn · 2017-12-19T17:17:25Z

tox.ini

+setenv =
+    DATADOG_ENV = test
+    DJANGO_SETTINGS_MODULE = restframework.settings
+


I use a different app than for other django unit tests because using rest_framework would break most of our django unit tests.

This way we can only test in a specific app that we properly trace exceptions raised during rest_framework view processing.

LotharSee · 2017-12-21T13:00:43Z

ddtrace/contrib/django/apps.py


 # project
 from .db import patch_db
 from .conf import settings
 from .cache import patch_cache
 from .templates import patch_template
+from .rest_framework import patch_rest_framework


Not sure we want to import it all the time, because it would trigger this exception log https://github.com/DataDog/dd-trace-py/pull/389/files#diff-6f7dbfbaaae5d3b33ba40f455fe6c12fR7

We should probably test the existence or usage of REST framework before trying to import then patch it.

Totally agree!

bmermet · 2017-12-21T17:58:17Z

ddtrace/contrib/django/restframework.py

+def unpatch_rest_framework():
+    if hasattr(APIView, ORIGINAL_HANDLE_EXCEPTION):
+        original_handle_exception = getattr(APIView, ORIGINAL_HANDLE_EXCEPTION)
+        setattr(APIView, 'handle_exception', original_handle_exception)


Why not use the wrapt library to wrap the method as it's done in most other integrations? It would be safer, at least it has safegards in case the number of parameters mismatch.

True, gonna use it.

bmermet · 2017-12-22T10:11:27Z

tests/contrib/django/app/settings_untraced.py

@@ -97,10 +97,21 @@
    'django.contrib.auth',
    'django.contrib.contenttypes',
    'django.contrib.sessions',
+


Why is it not needed to have the if django.VERSION >= (1, 10): check here too?

bmermet · 2017-12-22T10:15:13Z

tests/contrib/django/test_restframework.py

+        self.unpatch_rest_framework()
+        ok_(not hasattr(self.APIView, self.ORIGINAL_HANDLE_EXCEPTION))
+
+    def test_tracer(self):


What is the purpose of that test ?

Testing if the tracer is enabled. Do you think I should remove it?

Did your code change the logic for that? Because otherwise I think it's already checked elsewhere in the tests.

bmermet · 2017-12-22T10:20:45Z

tests/contrib/django/test_restframework.py

+        ok_(apps.is_installed('rest_framework'))
+        ok_(hasattr(self.APIView, self.ORIGINAL_HANDLE_EXCEPTION))
+
+    def test_unpatch(self):


Can you check that no span is generated if you raise an exception after it is unpatched?

A span will be generated, but the error won't get caught.

Right sorry. Can you check that behavior then? It would validate that your instrumentation changes the existing behavior.

bmermet

LGTM 👍

palazzem

We should also update our documentation if we have a support limit for DRF: http://pypi.datadoghq.com/trace/docs/#supported-versions

palazzem · 2018-01-03T15:22:10Z

ddtrace/contrib/django/apps.py

+
+            # Instrument rest_framework app to trace custom exception handling.
+            # rest_framework only supports django 1.10, 1.11 and 2.0.
+            if apps.is_installed('rest_framework') and django.VERSION >= (1, 10):


I think we don't need the and django.VERSION >= (1, 10) clause, if Django 1.8 is used with an incompatible version of DRF, neither the user app will work so we can assume here that it's properly configured. Let's have these check only if we do a "special" import.

palazzem · 2018-01-03T15:23:36Z

.circleci/config.yml

@@ -296,8 +296,8 @@ jobs:
      - restore_cache:
          keys:
              - tox-cache-django-{{ checksum "tox.ini" }}
-      - run: tox -e '{py27,py34,py35,py36}-django{18,19,110,111}-djangopylibmc06-djangoredis45-pylibmc-redis{210}-memcached' --result-json /tmp/django.1.results
-      - run: tox -e '{py27,py34,py35,py36}-django-autopatch{18,19,110,111}-djangopylibmc06-djangoredis45-pylibmc-redis{210}-memcached' --result-json /tmp/django.2.results
+      - run: tox -e '{py27,py34,py35,py36}-django{18,19,110,111}-djangopylibmc06-djangoredis45-pylibmc-redis{210}-memcached-restframework' --result-json /tmp/django.1.results


I think we don't need all of this. Can you rollback this change and just create two new envs?

tox -e '{py27,py34,py35,py36}-django{18,19,110,111}-restframework{37,36,...} tox -e '{py27,py34,py35,py36}-django-autopatch{18,19,110,111}-restframework{37,36,...}

To test rest framework we don't need memcache (make the required changes in the settings.py if needed, or create simply a new config file just for DRF).

Also let's test the latest 3 or 4 versions of DRF, though having a best-effort support since 3.0 would be great.

See: pypa/virtualenv#596

…g DRF

gabsn added bug integrations labels Dec 19, 2017

gabsn force-pushed the gabin/rest_framework branch from 6aba73d to 8a229bb Compare December 19, 2017 17:10

gabsn requested a review from palazzem December 19, 2017 17:11

gabsn commented Dec 19, 2017

View reviewed changes

gabsn requested a review from LotharSee December 19, 2017 17:49

gabsn force-pushed the gabin/rest_framework branch from 7197223 to b93528d Compare December 20, 2017 10:56

gabsn requested review from bmermet and removed request for LotharSee and palazzem December 21, 2017 10:12

LotharSee reviewed Dec 21, 2017

View reviewed changes

gabsn force-pushed the gabin/rest_framework branch from 42d81db to e2a3b80 Compare December 21, 2017 17:45

bmermet reviewed Dec 21, 2017

View reviewed changes

bmermet reviewed Dec 22, 2017

View reviewed changes

bmermet previously approved these changes Dec 22, 2017

View reviewed changes

gabsn dismissed bmermet’s stale review via 5294266 December 22, 2017 16:33

palazzem changed the title ~~Trace rest_framework app~~ trace Django Rest Framework Jan 3, 2018

palazzem added this to the 0.10.1 milestone Jan 3, 2018

palazzem reviewed Jan 3, 2018

View reviewed changes

gabsn force-pushed the gabin/rest_framework branch from d573ca2 to ff99a5e Compare January 4, 2018 16:30

palazzem self-requested a review January 5, 2018 17:31

gabsn added 5 commits January 8, 2018 15:59

Patch rest_framework

6c7a0ec

Add a test app for rest_framework

49245ca

Add unit tests for rest_framework integration

c2b1456

Simplify the test app

f137f2d

Add new test to the CI

4532c7a

gabsn added 15 commits January 8, 2018 15:59

Use the existing django tox env to test rest_framework

8545366

Fix flake8

b46ee59

Fix unit test

1395abe

Put an upper bound on djangorestframework version

20a8c55

Add new unit test

03c3901

Use wrapt

5b1975a

Fix issue with test_tracing_disabled

1f5601c

Fix bug due to shebang length limit on linux hosts.

59a98bc

See: pypa/virtualenv#596

Last commit

97939b6

Add a test for djangorestframework 3.4

6089bfb

Assume that user's app should have a proper version of django if usin…

d080912

…g DRF

Use specific settings for testing drf

3566311

Rm restframework to django folder

6075759

wip

5bccbc9

Fix tests

cd33bd1

gabsn force-pushed the gabin/rest_framework branch from a38c9d1 to cd33bd1 Compare January 8, 2018 20:59

Modify the docs

dccea31

gabsn force-pushed the gabin/rest_framework branch from 5a71fd7 to dccea31 Compare January 9, 2018 14:32

palazzem approved these changes Jan 19, 2018

View reviewed changes

palazzem merged commit 91fb199 into master Jan 19, 2018

palazzem deleted the gabin/rest_framework branch January 19, 2018 22:42

palazzem changed the title ~~trace Django Rest Framework~~ [django] support for Django Rest Framework Jan 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[django] support for Django Rest Framework #389

[django] support for Django Rest Framework #389

gabsn commented Dec 19, 2017 •

edited

gabsn Dec 19, 2017

gabsn Dec 19, 2017 •

edited

gabsn Dec 19, 2017

LotharSee Dec 21, 2017 •

edited

gabsn Dec 21, 2017

bmermet Dec 21, 2017 •

edited

gabsn Dec 22, 2017

bmermet Dec 22, 2017

gabsn Dec 22, 2017

bmermet Dec 22, 2017

gabsn Dec 22, 2017

bmermet Dec 22, 2017

bmermet Dec 22, 2017

gabsn Dec 22, 2017

bmermet Dec 22, 2017

bmermet left a comment

palazzem left a comment

palazzem Jan 3, 2018

palazzem Jan 3, 2018

palazzem Jan 3, 2018

[django] support for Django Rest Framework #389

[django] support for Django Rest Framework #389

Conversation

gabsn commented Dec 19, 2017 • edited

Before

After

Choose a reason for hiding this comment

gabsn Dec 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LotharSee Dec 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmermet Dec 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmermet left a comment

Choose a reason for hiding this comment

palazzem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabsn commented Dec 19, 2017 •

edited

gabsn Dec 19, 2017 •

edited

LotharSee Dec 21, 2017 •

edited

bmermet Dec 21, 2017 •

edited