fix(django): trace mro chain for Django views (DataDog#1625)

CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## 0.41.2 (25/08/2020)
+
+- Fix for an issue introduced by patching classes in the MRO of a Django View class (#1625).
+
+---
+
 ## 0.41.1 (25/08/2020)
 
 - reintroduce wrapt for patching Django view methods. ([#1622](https://github.com/DataDog/dd-trace-py/pull/1622))
@@ -27,8 +33,17 @@
 
 https://github.com/DataDog/dd-trace-py/compare/v0.40.0...v0.41.0
 
+
 ---
 
+
+## 0.40.2 (25/08/2020)
+
+- Fix for an issue introduced by patching classes in the MRO of a Django View class (#1625).
+
+---
+
+
 ## 0.40.1 (25/08/2020)
 
 - reintroduce wrapt for patching Django view methods. ([#1622](https://github.com/DataDog/dd-trace-py/pull/1622))
@@ -49,7 +64,7 @@ This release includes performance improvements in the core library, updates prof
 
 * Use `ddtrace.config.redis["service"]` to set the service name for the `redis` integration. The environment variable `DD_REDIS_SERVICE` can also be used.
 
-## httplib 
+## httplib
 
 * To enable distributed tracing, use `ddtrace.config.httplib["distributed_tracing"]`. By default, distributed tracing for `httplib` is disabled.
 
@@ -83,6 +98,12 @@ Read the [full changeset](https://github.com/DataDog/dd-trace-py/compare/v0.39.0
 
 ---
 
+## 0.39.2 (25/08/2020)
+
+- Fix for an issue introduced by patching classes in the MRO of a Django View class (#1625).
+
+---
+
 ## 0.39.1 (25/08/2020)
 
 - reintroduce wrapt for patching Django view methods. ([#1622](https://github.com/DataDog/dd-trace-py/pull/1622))
@@ -180,6 +201,13 @@ https://github.com/DataDog/dd-trace-py/milestone/57?closed=1
 
 ---
 
+
+## 0.38.4 (25/08/2020)
+
+- Fix for an issue introduced by patching classes in the MRO of a Django View class (#1625).
+
+---
+
 ## 0.38.3 (25/08/2020)
 
 - reintroduce wrapt for patching Django view methods. ([#1622](https://github.com/DataDog/dd-trace-py/pull/1622))
@@ -268,6 +296,12 @@ version. (#1468)
 
 ---
 
+## 0.37.3 (25/08/2020)
+
+- Fix for an issue introduced by patching classes in the MRO of a Django View class (#1625).
+
+---
+
 ## 0.37.2 (25/08/2020)
 
 - reintroduce wrapt for patching Django view methods. ([#1622](https://github.com/DataDog/dd-trace-py/pull/1622))
@@ -343,6 +377,12 @@ New environment variables have been added to allow you to easily configure and d
 
 ---
 
+## 0.36.3 (25/08/2020)
+
+- Fix for an issue introduced by patching classes in the MRO of a Django View class (#1625).
+
+---
+
 ## 0.36.2 (25/08/2020)
 
 - reintroduce wrapt for patching Django view methods. ([#1622](https://github.com/DataDog/dd-trace-py/pull/1622))
@@ -431,7 +471,7 @@ If you are using the profiler, please note that `ddtrace.profile` has been renam
 
 ## 0.35.1 (25/08/2020)
 
-- reintroduce wrapt for patching Django view methods. ([#1622](https://github.com/DataDog/dd-trace-py/pull/1622))
+- Fix for an issue introduced by patching classes in the MRO of a Django View class (#1625).
 
 ---
 
@@ -495,6 +535,12 @@ This release adds:
 Read the [full changeset](https://github.com/DataDog/dd-trace-py/compare/v0.34.1...v0.35.0).
 ---
 
+## 0.34.2 (25/08/2020)
+
+- Fix for an issue introduced by patching classes in the MRO of a Django View class (#1625).
+
+---
+
 ## 0.34.1 (09/03/2020)
 ## Changes
 

ddtrace/contrib/django/patch.py

@@ -8,7 +8,7 @@
 """
 import sys
 
-from inspect import isclass, isfunction
+from inspect import isclass, isfunction, getmro
 
 from ddtrace import config, Pin
 from ddtrace.vendor import debtcollector, six, wrapt
@@ -476,17 +476,31 @@ def traced_template_render(django, pin, wrapped, instance, args, kwargs):
 
 
 def instrument_view(django, view):
+    """
+    Helper to wrap Django views.
+
+    We want to wrap all lifecycle/http method functions for every class in the MRO for this view
+    """
+    if isfunction(view):
+        return _instrument_view(django, view)
+
+    for cls in reversed(getmro(view)):
+        _instrument_view(django, cls)
+
+    return view
+
+
+def _instrument_view(django, view):
     """Helper to wrap Django views."""
     # All views should be callable, double check before doing anything
-    if not callable(view) or isinstance(view, wrapt.ObjectProxy):
+    if not callable(view):
         return view
 
     # Patch view HTTP methods and lifecycle methods
     http_method_names = getattr(view, "http_method_names", ("get", "delete", "post", "options", "head"))
     lifecycle_methods = ("setup", "dispatch", "http_method_not_allowed")
     for name in list(http_method_names) + list(lifecycle_methods):
         try:
-            # View methods can be staticmethods
             func = getattr(view, name, None)
             if not func or isinstance(func, wrapt.ObjectProxy):
                 continue
@@ -514,8 +528,10 @@ def instrument_view(django, view):
             except Exception:
                 log.debug("Failed to instrument Django response %r function %s", response_cls, name, exc_info=True)
 
-    # Return a wrapped version of this view
-    return wrapt.FunctionWrapper(view, traced_func(django, "django.view", resource=func_name(view)))
+    # If the view itself is not wrapped, wrap it
+    if not isinstance(view, wrapt.ObjectProxy):
+        view = wrapt.FunctionWrapper(view, traced_func(django, "django.view", resource=func_name(view)))
+    return view
 
 
 @with_traced_module

tests/contrib/django/django1_app/urls.py

@@ -1,4 +1,5 @@
 from django.conf.urls import url
+from django.views.generic import TemplateView
 from django.views.decorators.cache import cache_page
 
 from .. import views
@@ -20,6 +21,9 @@
     url(r"^template-view/$", views.template_view, name="template-view"),
     url(r"^template-simple-view/$", views.template_simple_view, name="template-simple-view"),
     url(r"^template-list-view/$", views.template_list_view, name="template-list-view"),
+    # This must precede composed tests.
+    url(r"some-static-view/", TemplateView.as_view(template_name="my-template.html")),
     url(r"^composed-template-view/$", views.ComposedTemplateView.as_view(), name="composed-template-view"),
     url(r"^composed-get-view/$", views.ComposedGetView.as_view(), name="composed-get-view"),
+    url(r"^composed-view/$", views.ComposedView.as_view(), name="composed-view"),
 ]

tests/contrib/django/django_app/urls.py

@@ -2,6 +2,7 @@
 from django.contrib.auth import login
 from django.contrib.auth.models import User
 from django.http import HttpResponse
+from django.views.generic import TemplateView
 from django.views.decorators.cache import cache_page
 from django.urls import include, path, re_path
 
@@ -49,6 +50,9 @@ def authenticated_view(request):
     re_path(r"re-path.*/", repath_view),
     path("path/", path_view),
     path("include/", include("tests.contrib.django.django_app.extra_urls")),
+    # This must precede composed-view.
+    url(r"^some-static-view/$", TemplateView.as_view(template_name="my-template.html")),
     url(r"^composed-template-view/$", views.ComposedTemplateView.as_view(), name="composed-template-view"),
     url(r"^composed-get-view/$", views.ComposedGetView.as_view(), name="composed-get-view"),
+    url(r"^composed-view/$", views.ComposedView.as_view(), name="composed-view"),
 ]

tests/contrib/django/test_django.py

@@ -1,10 +1,12 @@
 import django
+from django.views.generic import TemplateView
 from django.test import modify_settings, override_settings
 import os
 import pytest
 
 from ddtrace import config
 from ddtrace.constants import ANALYTICS_SAMPLE_RATE_KEY, SAMPLING_PRIORITY_KEY
+from ddtrace.contrib.django.patch import instrument_view
 from ddtrace.ext import http, errors
 from ddtrace.ext.priority import USER_KEEP
 from ddtrace.propagation.http import HTTP_HEADER_TRACE_ID, HTTP_HEADER_PARENT_ID, HTTP_HEADER_SAMPLING_PRIORITY
@@ -581,7 +583,7 @@ def test_simple_view_options(client, test_spans):
     assert len(spans) == 1
     span = spans[0]
     span.assert_matches(
-        resource="tests.contrib.django.views.BasicView.options", error=0,
+        resource="django.views.generic.base.View.options", error=0,
     )
 
 
@@ -1377,7 +1379,9 @@ def test_custom_dispatch_template_view(client, test_spans):
 
     spans = test_spans.get_spans()
     assert [s.resource for s in spans if s.resource.endswith("dispatch")] == [
-        "tests.contrib.django.views.ComposedTemplateView.dispatch",
+        "tests.contrib.django.views.CustomDispatchMixin.dispatch",
+        "tests.contrib.django.views.AnotherCustomDispatchMixin.dispatch",
+        "django.views.generic.base.View.dispatch",
     ]
 
 
@@ -1392,8 +1396,40 @@ def test_custom_dispatch_get_view(client, test_spans):
 
     spans = test_spans.get_spans()
     assert [s.resource for s in spans if s.resource.endswith("dispatch")] == [
-        "tests.contrib.django.views.ComposedGetView.dispatch",
+        "tests.contrib.django.views.CustomDispatchMixin.dispatch",
+        "django.views.generic.base.View.dispatch",
     ]
     assert [s.resource for s in spans if s.resource.endswith("get")] == [
         "tests.contrib.django.views.ComposedGetView.get",
+        "tests.contrib.django.views.CustomGetView.get",
     ]
+
+
+def test_view_mixin(client, test_spans):
+    from tests.contrib.django import views
+
+    assert views.DISPATCH_CALLED is False
+    resp = client.get("/composed-view/")
+    assert views.DISPATCH_CALLED is True
+
+    assert resp.status_code == 200
+    assert resp.content.strip() == b"custom dispatch"
+
+
+def test_template_view_patching():
+    """
+    Test to ensure that patching a view does not give it properties it did not have before
+    """
+    # DEV: `vars(cls)` will give you only the properties defined on that class,
+    #      it will not traverse through __mro__ to find the property from a parent class
+
+    # We are not starting with a "dispatch" property
+    assert "dispatch" not in vars(TemplateView)
+
+    # Manually call internal method for patching
+    instrument_view(django, TemplateView)
+    assert "dispatch" not in vars(TemplateView)
+
+    # Patch via `as_view()`
+    TemplateView.as_view()
+    assert "dispatch" not in vars(TemplateView)

tests/contrib/django/views.py

@@ -142,3 +142,17 @@ def get(self, request):
         if self.dispatch_call_counter == 1:
             return super(ComposedGetView, self).get(request)
         raise Exception("Custom dispatch not called.")
+
+
+DISPATCH_CALLED = False
+
+
+class CustomDispatchView(View):
+    def dispatch(self, request):
+        global DISPATCH_CALLED
+        DISPATCH_CALLED = True
+        return super(CustomDispatchView, self).dispatch(request)
+
+
+class ComposedView(TemplateView, CustomDispatchView):
+    template_name = "custom_dispatch.html"

tests/contrib/djangorestframework/test_djangorestframework.py

@@ -23,11 +23,11 @@ def test_trace_exceptions(client, test_spans):  # noqa flake8 complains about sh
     assert_span_http_status_code(sp, 500)
     assert sp.get_tag("http.method") == "GET"
 
-    # the DRF integration should set the traceback on the django.view span
+    # the DRF integration should set the traceback on the django.view.dispatch span
     # (as it's the current span when the exception info is set)
-    view_spans = list(test_spans.filter_spans(name="django.view"))
-    assert len(view_spans) == 1
-    err_span = view_spans[0]
+    view_dispatch_spans = list(test_spans.filter_spans(name="django.view.dispatch"))
+    assert len(view_dispatch_spans) == 1
+    err_span = view_dispatch_spans[0]
     assert err_span.error == 1
     assert err_span.get_tag("error.msg") == "Authentication credentials were not provided."
     assert "NotAuthenticated" in err_span.get_tag("error.stack")