Skip to content

Commit 209bdba

Browse files
brettlangdonP403n1x87christophe-papazian
authored
chore(django): migrate template render wrappers to WrappingContext (#14069)
Depends on #14098 We will work to update the Django integration to use new wrapping patterns. Overall this should have a performance impact, but only doing template render is not having enough of an impact to call this a performance improvement for Django. The only change to tests needed were the ones asserting that `Template.render` was wrapped, otherwise all existing tests are passing. ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) --------- Co-authored-by: Gabriele N. Tornetta <[email protected]> Co-authored-by: Christophe Papazian <[email protected]>
1 parent 71c9cde commit 209bdba

File tree

4 files changed

+174
-37
lines changed

4 files changed

+174
-37
lines changed

ddtrace/_trace/trace_handlers.py

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
11
import functools
22
import sys
3+
from types import TracebackType
34
from typing import Any
45
from typing import Callable
56
from typing import Dict
67
from typing import List
78
from typing import Optional
9+
from typing import Tuple
810
from urllib import parse
911

1012
import wrapt
@@ -144,6 +146,24 @@ def _start_span(ctx: core.ExecutionContext, call_trace: bool = True, **kwargs) -
144146
return span
145147

146148

149+
def _finish_span(
150+
ctx: core.ExecutionContext,
151+
exc_info: Tuple[Optional[type], Optional[BaseException], Optional[TracebackType]],
152+
):
153+
"""
154+
Finish the span in the context.
155+
If no span is present, do nothing.
156+
"""
157+
span = ctx.span
158+
if not span:
159+
return
160+
161+
exc_type, exc_value, exc_traceback = exc_info
162+
if exc_type and exc_value and exc_traceback:
163+
span.set_exc_info(exc_type, exc_value, exc_traceback)
164+
span.finish()
165+
166+
147167
def _set_web_frameworks_tags(ctx, span, int_config):
148168
span.set_tag_str(COMPONENT, int_config.integration_name)
149169
span.set_tag_str(SPAN_KIND, SpanKind.SERVER)
@@ -965,5 +985,8 @@ def listen():
965985
):
966986
core.on(f"context.started.{context_name}", _start_span)
967987

988+
for name in ("django.template.render",):
989+
core.on(f"context.ended.{name}", _finish_span)
990+
968991

969992
listen()

ddtrace/contrib/internal/django/patch.py

Lines changed: 8 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@
3535
from ddtrace.ext import sql as sqlx
3636
from ddtrace.internal import core
3737
from ddtrace.internal._exceptions import BlockingException
38-
from ddtrace.internal.compat import maybe_stringify
3938
from ddtrace.internal.constants import COMPONENT
4039
from ddtrace.internal.core.event_hub import ResultType
4140
from ddtrace.internal.logger import get_logger
@@ -573,36 +572,6 @@ def blocked_response():
573572
return response # noqa: B012
574573

575574

576-
@trace_utils.with_traced_module
577-
def traced_template_render(django, pin, wrapped, instance, args, kwargs):
578-
# DEV: Check here in case this setting is configured after a template has been instrumented
579-
if not config_django.instrument_templates:
580-
return wrapped(*args, **kwargs)
581-
582-
template_name = maybe_stringify(getattr(instance, "name", None))
583-
if template_name:
584-
resource = template_name
585-
else:
586-
resource = "{0}.{1}".format(func_name(instance), wrapped.__name__)
587-
588-
tags = {COMPONENT: config_django.integration_name}
589-
if template_name:
590-
tags["django.template.name"] = template_name
591-
engine = getattr(instance, "engine", None)
592-
if engine:
593-
tags["django.template.engine.class"] = func_name(engine)
594-
595-
with core.context_with_data(
596-
"django.template.render",
597-
span_name="django.template.render",
598-
resource=resource,
599-
span_type=http.TEMPLATE,
600-
tags=tags,
601-
pin=pin,
602-
) as ctx, ctx.span:
603-
return wrapped(*args, **kwargs)
604-
605-
606575
def instrument_view(django, view, path=None):
607576
"""
608577
Helper to wrap Django views.
@@ -985,9 +954,9 @@ def _(m):
985954
)
986955

987956
if config_django.instrument_templates:
988-
when_imported("django.template.base")(
989-
lambda m: trace_utils.wrap(m, "Template.render", traced_template_render(django))
990-
)
957+
from .templates import DjangoTemplateWrappingContext
958+
959+
when_imported("django.template.base")(DjangoTemplateWrappingContext.instrument_module)
991960

992961
if django.VERSION < (4, 0, 0):
993962
when_imported("django.conf.urls")(lambda m: trace_utils.wrap(m, "url", traced_urls_path(django)))
@@ -1060,6 +1029,11 @@ def _unpatch(django):
10601029
trace_utils.unwrap(conn, "cursor")
10611030
trace_utils.unwrap(django.db.utils.ConnectionHandler, "__getitem__")
10621031

1032+
if config.django.instrument_templates:
1033+
from .templates import DjangoTemplateWrappingContext
1034+
1035+
DjangoTemplateWrappingContext.uninstrument_module(django.template.base)
1036+
10631037

10641038
def unpatch():
10651039
import django
Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
from types import FunctionType
2+
from types import ModuleType
3+
from types import TracebackType
4+
import typing
5+
from typing import Optional
6+
from typing import Type
7+
from typing import TypeVar
8+
9+
import ddtrace
10+
from ddtrace import config
11+
from ddtrace._trace.pin import Pin
12+
from ddtrace.ext import http
13+
from ddtrace.internal import core
14+
from ddtrace.internal.compat import maybe_stringify
15+
from ddtrace.internal.constants import COMPONENT
16+
from ddtrace.internal.logger import get_logger
17+
from ddtrace.internal.utils.importlib import func_name
18+
from ddtrace.internal.wrapping.context import WrappingContext
19+
from ddtrace.settings.integration import IntegrationConfig
20+
21+
22+
T = TypeVar("T")
23+
24+
log = get_logger(__name__)
25+
26+
27+
# PERF: cache the getattr lookup for the Django config
28+
config_django: IntegrationConfig = typing.cast(IntegrationConfig, config.django)
29+
30+
31+
class DjangoTemplateWrappingContext(WrappingContext):
32+
"""
33+
A context for wrapping django.template.base:Template.render method.
34+
"""
35+
36+
def __init__(self, f: FunctionType, django: ModuleType) -> None:
37+
super().__init__(f)
38+
self._django = django
39+
40+
@classmethod
41+
def instrument_module(cls, django_template_base: ModuleType) -> None:
42+
"""
43+
Instrument the django template base module to wrap the render method.
44+
"""
45+
if not config_django.instrument_templates:
46+
return
47+
48+
# Wrap the render method of the Template class
49+
Template = getattr(django_template_base, "Template", None)
50+
if not Template or not hasattr(Template, "render"):
51+
return
52+
53+
import django
54+
55+
cls(typing.cast(FunctionType, Template.render), django).wrap()
56+
57+
@classmethod
58+
def uninstrument_module(cls, django_template_base: ModuleType) -> None:
59+
# Unwrap the render method of the Template class
60+
Template = getattr(django_template_base, "Template", None)
61+
if not Template or not hasattr(Template, "render"):
62+
return
63+
64+
if cls.is_wrapped(Template.render):
65+
ctx = cls.extract(Template.render)
66+
ctx.unwrap()
67+
68+
def __enter__(self) -> "DjangoTemplateWrappingContext":
69+
super().__enter__()
70+
71+
if not config_django.instrument_templates:
72+
return self
73+
74+
# Get the template instance (self parameter of the render method)
75+
# Note: instance is a django.template.base.Template
76+
instance = self.get_local("self")
77+
78+
# Extract template name
79+
template_name = maybe_stringify(getattr(instance, "name", None))
80+
if template_name:
81+
resource = template_name
82+
else:
83+
resource = "{0}.{1}".format(func_name(instance), self.__wrapped__.__name__)
84+
85+
# Build tags
86+
tags = {COMPONENT: config_django.integration_name}
87+
if template_name:
88+
tags["django.template.name"] = template_name
89+
90+
engine = getattr(instance, "engine", None)
91+
if engine:
92+
tags["django.template.engine.class"] = func_name(engine)
93+
94+
# Create the span context
95+
pin = Pin.get_from(self._django)
96+
tracer = ddtrace.tracer
97+
if pin:
98+
tracer = pin.tracer or tracer
99+
ctx = core.context_with_data(
100+
"django.template.render",
101+
span_name="django.template.render",
102+
resource=resource,
103+
span_type=http.TEMPLATE,
104+
tags=tags,
105+
tracer=tracer,
106+
)
107+
108+
# Enter the context and store it
109+
ctx.__enter__()
110+
self.set("ctx", ctx)
111+
112+
return self
113+
114+
def _close_ctx(
115+
self, exc_type: Optional[Type[BaseException]], exc_val: Optional[BaseException], exc_tb: Optional[TracebackType]
116+
) -> None:
117+
try:
118+
# Close the context and any open span
119+
ctx = self.get("ctx")
120+
ctx.__exit__(exc_type, exc_val, exc_tb)
121+
except Exception:
122+
log.exception("Failed to close Django template render wrapping context")
123+
124+
def __return__(self, value: T) -> T:
125+
if config_django.instrument_templates:
126+
self._close_ctx(None, None, None)
127+
return super().__return__(value)
128+
129+
def __exit__(
130+
self, exc_type: Optional[Type[BaseException]], exc_val: Optional[BaseException], exc_tb: Optional[TracebackType]
131+
) -> None:
132+
if config_django.instrument_templates:
133+
self._close_ctx(exc_type, exc_val, exc_tb)
134+
return super().__exit__(exc_type, exc_val, exc_tb)

tests/contrib/django/test_django_patch.py

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
from ddtrace.contrib.internal.django.patch import get_version
22
from ddtrace.contrib.internal.django.patch import patch
3+
from ddtrace.contrib.internal.django.templates import DjangoTemplateWrappingContext
34
from tests.contrib.patch import PatchTestCase
45

56

@@ -22,7 +23,7 @@ def assert_module_patched(self, django):
2223

2324
import django.template.base
2425

25-
self.assert_wrapped(django.template.base.Template.render)
26+
assert DjangoTemplateWrappingContext.is_wrapped(django.template.base.Template.render)
2627
if django.VERSION >= (2, 0, 0):
2728
self.assert_wrapped(django.urls.path)
2829
self.assert_wrapped(django.urls.re_path)
@@ -34,7 +35,7 @@ def assert_not_module_patched(self, django):
3435

3536
self.assert_not_wrapped(django.core.handlers.base.BaseHandler.load_middleware)
3637
self.assert_not_wrapped(django.core.handlers.base.BaseHandler.get_response)
37-
self.assert_not_wrapped(django.template.base.Template.render)
38+
assert not DjangoTemplateWrappingContext.is_wrapped(django.template.base.Template.render)
3839
if django.VERSION >= (2, 0, 0):
3940
self.assert_not_wrapped(django.urls.path)
4041
self.assert_not_wrapped(django.urls.re_path)
@@ -46,7 +47,12 @@ def assert_not_module_double_patched(self, django):
4647
self.assert_not_double_wrapped(django.apps.registry.Apps.populate)
4748
self.assert_not_double_wrapped(django.core.handlers.base.BaseHandler.load_middleware)
4849
self.assert_not_double_wrapped(django.core.handlers.base.BaseHandler.get_response)
49-
self.assert_not_double_wrapped(django.template.base.Template.render)
50+
51+
assert DjangoTemplateWrappingContext.is_wrapped(django.template.base.Template.render)
52+
ctx = DjangoTemplateWrappingContext.extract(django.template.base.Template.render)
53+
ctx.unwrap()
54+
assert not DjangoTemplateWrappingContext.is_wrapped(django.template.base.Template.render)
55+
5056
if django.VERSION >= (2, 0, 0):
5157
self.assert_not_double_wrapped(django.urls.path)
5258
self.assert_not_double_wrapped(django.urls.re_path)

0 commit comments

Comments
 (0)