Skip to content

Commit a0119a4

Browse files
fix(django): Fix multiple fn middleware sharing the same resource name (#2884) (#2886)
* fix(django): Fix multiple fn middleware sharing the same resource name Fixes #2873 * fix flake8 issues * fix other tests * scope test to >=2.0.0 (cherry picked from commit 83a775a) Co-authored-by: Brett Langdon <[email protected]>
1 parent 836486d commit a0119a4

20 files changed

+62
-30
lines changed

ddtrace/contrib/django/patch.py

Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -252,19 +252,28 @@ def traced_load_middleware(django, pin, func, instance, args, kwargs):
252252
base = ".".join(split[:-1])
253253
attr = split[-1]
254254

255-
# Function-based middleware is a factory which returns a handler function for requests.
256-
# So instead of tracing the factory, we want to trace its returned value.
257-
# We wrap the factory to return a traced version of the handler function.
258-
def wrapped_factory(func, instance, args, kwargs):
259-
# r is the middleware handler function returned from the factory
260-
r = func(*args, **kwargs)
261-
if r:
262-
return wrapt.FunctionWrapper(r, traced_func(django, "django.middleware", resource=mw_path))
263-
# If r is an empty middleware function (i.e. returns None), don't wrap since NoneType cannot be called
264-
else:
265-
return r
266-
267-
trace_utils.wrap(base, attr, wrapped_factory)
255+
# DEV: We need to have a closure over `mw_path` for the resource name or else
256+
# all function based middleware will share the same resource name
257+
def _wrapper(resource):
258+
# Function-based middleware is a factory which returns a handler function for requests.
259+
# So instead of tracing the factory, we want to trace its returned value.
260+
# We wrap the factory to return a traced version of the handler function.
261+
def wrapped_factory(func, instance, args, kwargs):
262+
# r is the middleware handler function returned from the factory
263+
r = func(*args, **kwargs)
264+
if r:
265+
return wrapt.FunctionWrapper(
266+
r,
267+
traced_func(django, "django.middleware", resource=resource),
268+
)
269+
# If r is an empty middleware function (i.e. returns None), don't wrap since
270+
# NoneType cannot be called
271+
else:
272+
return r
273+
274+
return wrapped_factory
275+
276+
trace_utils.wrap(base, attr, _wrapper(resource=mw_path))
268277

269278
# Instrument class-based middleware
270279
elif isclass(mw):
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
fixes:
3+
- |
4+
Fixes an issue where all Django function middleware will share the same resource name.

tests/contrib/django/middleware.py

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,16 @@ def mw(request):
4242
return mw
4343

4444

45+
def fn2_middleware(get_response):
46+
"""Function factory middleware."""
47+
48+
def mw(request):
49+
response = get_response(request)
50+
return response
51+
52+
return mw
53+
54+
4555
def empty_middleware(get_response):
4656
"""Empty function middleware for regression testing."""
4757

tests/contrib/django/test_django.py

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -278,7 +278,7 @@ def test_v2XX_middleware(client, test_spans):
278278
"django.middleware.security.SecurityMiddleware.process_request",
279279
"django.middleware.security.SecurityMiddleware.process_response",
280280
"tests.contrib.django.middleware.ClsMiddleware.__call__",
281-
"tests.contrib.django.middleware.EverythingMiddleware",
281+
"tests.contrib.django.middleware.fn_middleware",
282282
"tests.contrib.django.middleware.EverythingMiddleware.__call__",
283283
"tests.contrib.django.middleware.EverythingMiddleware.process_view",
284284
}
@@ -449,6 +449,15 @@ def test_empty_middleware_func_is_raised_in_django(client, test_spans):
449449
client.get("/")
450450

451451

452+
@pytest.mark.skipif(django.VERSION < (2, 0, 0), reason="")
453+
def test_multiple_fn_middleware_resource_names(client, test_spans):
454+
with modify_settings(MIDDLEWARE={"append": "tests.contrib.django.middleware.fn2_middleware"}):
455+
client.get("/")
456+
457+
assert test_spans.find_span(name="django.middleware", resource="tests.contrib.django.middleware.fn_middleware")
458+
assert test_spans.find_span(name="django.middleware", resource="tests.contrib.django.middleware.fn2_middleware")
459+
460+
452461
"""
453462
View tests
454463
"""

tests/snapshots/tests.contrib.django.test_django_snapshots.test_404_exceptions.snap

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@
150150
"duration" 1007000}
151151
{"name" "django.middleware"
152152
"service" "django"
153-
"resource" "tests.contrib.django.middleware.EverythingMiddleware"
153+
"resource" "tests.contrib.django.middleware.fn_middleware"
154154
"error" 0
155155
"span_id" 21
156156
"trace_id" 0

tests/snapshots/tests.contrib.django.test_django_snapshots.test_404_exceptions_21x.snap

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,7 @@
149149
"duration" 1023000}
150150
{"name" "django.middleware"
151151
"service" "django"
152-
"resource" "tests.contrib.django.middleware.EverythingMiddleware"
152+
"resource" "tests.contrib.django.middleware.fn_middleware"
153153
"error" 0
154154
"span_id" 21
155155
"trace_id" 0

tests/snapshots/tests.contrib.django.test_django_snapshots.test_asgi_200_30.snap

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@
150150
"duration" 239310}
151151
{"name" "django.middleware"
152152
"service" "django"
153-
"resource" "tests.contrib.django.middleware.EverythingMiddleware"
153+
"resource" "tests.contrib.django.middleware.fn_middleware"
154154
"error" 0
155155
"span_id" 21
156156
"trace_id" 0

tests/snapshots/tests.contrib.django.test_django_snapshots.test_asgi_200_31.snap

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@
152152
"duration" 1720934}
153153
{"name" "django.middleware"
154154
"service" "django"
155-
"resource" "tests.contrib.django.middleware.EverythingMiddleware"
155+
"resource" "tests.contrib.django.middleware.fn_middleware"
156156
"error" 0
157157
"span_id" 21
158158
"trace_id" 0

tests/snapshots/tests.contrib.django.test_django_snapshots.test_asgi_200_3x.snap

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@
152152
"duration" 2065900}
153153
{"name" "django.middleware"
154154
"service" "django"
155-
"resource" "tests.contrib.django.middleware.EverythingMiddleware"
155+
"resource" "tests.contrib.django.middleware.fn_middleware"
156156
"error" 0
157157
"span_id" 21
158158
"trace_id" 0

tests/snapshots/tests.contrib.django.test_django_snapshots.test_asgi_500_30.snap

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@
150150
"duration" 1747119558}
151151
{"name" "django.middleware"
152152
"service" "django"
153-
"resource" "tests.contrib.django.middleware.EverythingMiddleware"
153+
"resource" "tests.contrib.django.middleware.fn_middleware"
154154
"error" 0
155155
"span_id" 21
156156
"trace_id" 0

0 commit comments

Comments
 (0)