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

Author: Kyle-Verhoog

ddtrace/contrib/django/patch.py

@@ -9,7 +9,7 @@
 import os
 import sys
 
-from inspect import isclass, isfunction
+from inspect import isclass, isfunction, getmro
 
 from ddtrace import config, Pin
 from ddtrace.vendor import debtcollector, wrapt
@@ -432,17 +432,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
@@ -470,8 +484,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
@@ -17,6 +18,9 @@
     url(r"^partial-view/$", views.partial_view, name="partial-view"),
     url(r"^lambda-view/$", views.lambda_view, name="lambda-view"),
     url(r"^error-500/$", views.error_500, name="error-500"),
+    # 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

@@ -1,5 +1,6 @@
 from django.conf.urls import url
 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
 
@@ -30,6 +31,9 @@ def path_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,9 +1,11 @@
 import django
+from django.views.generic import TemplateView
 from django.test import modify_settings, override_settings
 import os
 import pytest
 
 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
@@ -492,7 +494,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,
     )
 
 
@@ -1239,7 +1241,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",
     ]
 
 
@@ -1254,8 +1258,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

@@ -118,3 +118,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")