diff --git a/ddtrace/contrib/django/patch.py b/ddtrace/contrib/django/patch.py index 97534f9094c..a36e37e9dbb 100644 --- a/ddtrace/contrib/django/patch.py +++ b/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,9 +476,24 @@ 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 @@ -486,7 +501,6 @@ def instrument_view(django, view): 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 diff --git a/tests/contrib/django/django1_app/urls.py b/tests/contrib/django/django1_app/urls.py index d5e81e9e0b3..386b969fca9 100644 --- a/tests/contrib/django/django1_app/urls.py +++ b/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"), ] diff --git a/tests/contrib/django/django_app/urls.py b/tests/contrib/django/django_app/urls.py index 7d752d848b6..017369c0833 100644 --- a/tests/contrib/django/django_app/urls.py +++ b/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"), ] diff --git a/tests/contrib/django/test_django.py b/tests/contrib/django/test_django.py index 3100f80a279..2c044cae284 100644 --- a/tests/contrib/django/test_django.py +++ b/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 @@ -580,7 +582,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, ) @@ -1376,7 +1378,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", ] @@ -1391,8 +1395,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) diff --git a/tests/contrib/django/views.py b/tests/contrib/django/views.py index a393f856b60..9d796c25e38 100644 --- a/tests/contrib/django/views.py +++ b/tests/contrib/django/views.py @@ -126,7 +126,7 @@ class ComposedTemplateView(TemplateView, CustomDispatchMixin, AnotherCustomDispa def get_context_data(self, **kwargs): context = super(ComposedTemplateView, self).get_context_data(**kwargs) - context['dispatch_call_counter'] = self.dispatch_call_counter + context["dispatch_call_counter"] = self.dispatch_call_counter return context @@ -142,3 +142,18 @@ 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" diff --git a/tests/contrib/djangorestframework/test_djangorestframework.py b/tests/contrib/djangorestframework/test_djangorestframework.py index a45e094f574..1132465f6dd 100644 --- a/tests/contrib/djangorestframework/test_djangorestframework.py +++ b/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")