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

Author: Kyle-Verhoog

ddtrace/compat.py

@@ -6,15 +6,15 @@
 from ddtrace.vendor import six
 
 __all__ = [
-    'httplib',
-    'iteritems',
-    'PY2',
-    'Queue',
-    'stringify',
-    'StringIO',
-    'urlencode',
-    'parse',
-    'reraise',
+    "httplib",
+    "iteritems",
+    "PY2",
+    "Queue",
+    "stringify",
+    "StringIO",
+    "urlencode",
+    "parse",
+    "reraise",
 ]
 
 PYTHON_VERSION_INFO = sys.version_info
@@ -42,7 +42,7 @@
 string_type = six.string_types[0]
 msgpack_type = six.binary_type
 # DEV: `six` doesn't have `float` in `integer_types`
-numeric_types = six.integer_types + (float, )
+numeric_types = six.integer_types + (float,)
 
 # Pattern class generated by `re.compile`
 if PYTHON_VERSION_INFO >= (3, 7):
@@ -76,7 +76,9 @@ def time_ns():
     # Execute from a string to get around syntax errors from `yield from`
     # DEV: The idea to do this was stolen from `six`
     #   https://github.com/benjaminp/six/blob/15e31431af97e5e64b80af0a3f598d382bcdd49a/six.py#L719-L737
-    six.exec_(textwrap.dedent("""
+    six.exec_(
+        textwrap.dedent(
+            """
     import functools
     import asyncio
 
@@ -100,7 +102,9 @@ def func_wrapper(*args, **kwargs):
                 return result
 
         return func_wrapper
-    """))
+    """
+        )
+    )
 
 else:
     # asyncio is missing so we can't have coroutines; these
@@ -112,100 +116,6 @@ def iscoroutinefunction(fn):
     def make_async_decorator(tracer, fn, *params, **kw_params):
         return fn
 
-# static version of getattr backported from Python 3.7
-try:
-    from inspect import getattr_static
-except ImportError:
-    import types
-
-    _sentinel = object()
-
-    def _static_getmro(klass):
-        return type.__dict__['__mro__'].__get__(klass)
-
-    def _check_instance(obj, attr):
-        instance_dict = {}
-        try:
-            instance_dict = object.__getattribute__(obj, "__dict__")
-        except AttributeError:
-            pass
-        return dict.get(instance_dict, attr, _sentinel)
-
-    def _check_class(klass, attr):
-        for entry in _static_getmro(klass):
-            if _shadowed_dict(type(entry)) is _sentinel:
-                try:
-                    return entry.__dict__[attr]
-                except KeyError:
-                    pass
-        return _sentinel
-
-    def _is_type(obj):
-        try:
-            _static_getmro(obj)
-        except TypeError:
-            return False
-        return True
-
-    def _shadowed_dict(klass):
-        dict_attr = type.__dict__["__dict__"]
-        for entry in _static_getmro(klass):
-            try:
-                class_dict = dict_attr.__get__(entry)["__dict__"]
-            except KeyError:
-                pass
-            else:
-                if not (type(class_dict) is types.GetSetDescriptorType and # noqa: E721,E261,W504
-                        class_dict.__name__ == "__dict__" and # noqa: E261,W504
-                        class_dict.__objclass__ is entry):
-                    return class_dict
-        return _sentinel
-
-    def getattr_static(obj, attr, default=_sentinel):
-        """Retrieve attributes without triggering dynamic lookup via the
-        descriptor protocol,  __getattr__ or __getattribute__.
-
-        Note: this function may not be able to retrieve all attributes
-        that getattr can fetch (like dynamically created attributes)
-        and may find attributes that getattr can't (like descriptors
-        that raise AttributeError). It can also return descriptor objects
-        instead of instance members in some cases. See the
-        documentation for details.
-        """
-        instance_result = _sentinel
-        if not _is_type(obj):
-            klass = type(obj)
-            dict_attr = _shadowed_dict(klass)
-            if (dict_attr is _sentinel or # noqa: E261,E721,W504
-                type(dict_attr) is types.MemberDescriptorType):
-                instance_result = _check_instance(obj, attr)
-        else:
-            klass = obj
-
-        klass_result = _check_class(klass, attr)
-
-        if instance_result is not _sentinel and klass_result is not _sentinel:
-            if (_check_class(type(klass_result), '__get__') is not _sentinel and # noqa: W504,E261,E721
-                _check_class(type(klass_result), '__set__') is not _sentinel):
-                return klass_result
-
-        if instance_result is not _sentinel:
-            return instance_result
-        if klass_result is not _sentinel:
-            return klass_result
-
-        if obj is klass:
-            # for types we check the metaclass too
-            for entry in _static_getmro(type(klass)):
-                if _shadowed_dict(type(entry)) is _sentinel:
-                    try:
-                        return entry.__dict__[attr]
-                    except KeyError:
-                        pass
-        if default is not _sentinel:
-            return default
-        raise AttributeError(attr)
-
 
 # DEV: There is `six.u()` which does something similar, but doesn't have the guard around `hasattr(s, 'decode')`
 def to_unicode(s):
@@ -219,8 +129,8 @@ def to_unicode(s):
 
     # If the object has a `decode` method, then decode into `utf-8`
     #   e.g. Python 2 `str`, Python 2/3 `bytearray`, etc
-    if hasattr(s, 'decode'):
-        return s.decode('utf-8')
+    if hasattr(s, "decode"):
+        return s.decode("utf-8")
 
     # Always try to coerce the object into the `six.text_type` object we expect
     #   e.g. `to_unicode(1)`, `to_unicode(dict(key='value'))`

ddtrace/contrib/django/patch.py

@@ -9,11 +9,10 @@
 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
-from ddtrace.compat import getattr_static
 from ddtrace.constants import ANALYTICS_SAMPLE_RATE_KEY
 from ddtrace.contrib import func_name, dbapi
 from ddtrace.ext import http, sql as sqlx, SpanTypes
@@ -433,28 +432,38 @@ 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_static(view, name, None)
+            func = getattr(view, name, None)
             if not func or isinstance(func, wrapt.ObjectProxy):
                 continue
 
             resource = "{0}.{1}".format(func_name(view), name)
             op_name = "django.view.{0}".format(name)
-
-            # Set attribute here rather than using wrapt.wrappers.wrap_function_wrapper
-            # since it will not resolve attribute to staticmethods
-            wrapper = wrapt.FunctionWrapper(func, traced_func(django, name=op_name, resource=resource))
-            setattr(view, name, wrapper)
+            wrap(view, name, traced_func(django, name=op_name, resource=resource))
         except Exception:
             log.debug("Failed to instrument Django view %r function %s", view, name, exc_info=True)
 
@@ -475,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,4 +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,4 +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/templates/custom_dispatch.html

@@ -0,0 +1 @@
+custom dispatch {{ dispatch_call_counter }}

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,
     )
 
 
@@ -1226,3 +1228,70 @@ def test_urlpatterns_repath(client, test_spans):
 
     # Ensure the view was traced
     assert len(list(test_spans.filter_spans(name="django.view"))) == 1
+
+
+def test_custom_dispatch_template_view(client, test_spans):
+    """
+    Test that a template view with a custom dispatch method inherited from a
+    mixin is called.
+    """
+    resp = client.get("/composed-template-view/")
+    assert resp.status_code == 200
+    assert resp.content.strip() == b"custom dispatch 2"
+
+    spans = test_spans.get_spans()
+    assert [s.resource for s in spans if s.resource.endswith("dispatch")] == [
+        "tests.contrib.django.views.CustomDispatchMixin.dispatch",
+        "tests.contrib.django.views.AnotherCustomDispatchMixin.dispatch",
+        "django.views.generic.base.View.dispatch",
+    ]
+
+
+def test_custom_dispatch_get_view(client, test_spans):
+    """
+    Test that a get method on a view with a custom dispatch method inherited
+    from a mixin is called.
+    """
+    resp = client.get("/composed-get-view/")
+    assert resp.status_code == 200
+    assert resp.content.strip() == b"custom get"
+
+    spans = test_spans.get_spans()
+    assert [s.resource for s in spans if s.resource.endswith("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)