Skip to content

Commit 4ff0d4e

Browse files
fix(django): populate span.resource on forced exit [backport 1.16] (#6695)
Backport 94c0415 from #6674 to 1.16. This change fixes a behavior where span.resource would not be correctly filled out when django.core.base.handler.BaseHandler.get_response was terminated. In particular, this issue arose with gunicorn terminating the active worker after a request took more than the worker timeout period (default 10 seconds). Before change: ``` span.resource = "GET" ``` After Change: ``` span.resource = "GET <ENDPOINT>" ``` This change also adds a .gitignore entry for an auto-generated file. ## Checklist - [x] Change(s) are motivated and described in the PR description. - [x] Testing strategy is described if automated tests are not included in the PR. - [x] Risk is outlined (performance impact, potential for breakage, maintainability, etc). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed. If no release note is required, add label `changelog/no-changelog`. - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)). - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Title is accurate. - [x] No unnecessary changes are introduced. - [x] Description motivates each change. - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes unless absolutely necessary. - [x] Testing strategy adequately addresses listed risk(s). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] Release note makes sense to a user of the library. - [x] Reviewer has explicitly acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment. - [x] 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: Teague Bick <[email protected]>
1 parent 7e26f59 commit 4ff0d4e

File tree

4 files changed

+29
-4
lines changed

4 files changed

+29
-4
lines changed

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ ddtrace/profiling/collector/stack.c
1414
ddtrace/profiling/exporter/pprof.c
1515
ddtrace/profiling/_build.c
1616
ddtrace/internal/datadog/profiling/ddup.cpp
17+
ddtrace/internal/datadog/profiling/_ddup.cpp
1718
ddtrace/internal/_encoding.c
1819
ddtrace/internal/_rand.c
1920
ddtrace/internal/_tagset.c

ddtrace/contrib/django/utils.py

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -329,6 +329,9 @@ def _after_request_tags(pin, span, request, response):
329329
except Exception:
330330
log.debug("Error retrieving authentication information for user", exc_info=True)
331331

332+
# DEV: Resolve the view and resource name at the end of the request in case
333+
# urlconf changes at any point during the request
334+
_set_resolver_tags(pin, span, request)
332335
if response:
333336
status = response.status_code
334337
span.set_tag_str("django.response.class", func_name(response))
@@ -361,10 +364,6 @@ def _after_request_tags(pin, span, request, response):
361364

362365
url = get_request_uri(request)
363366

364-
# DEV: Resolve the view and resource name at the end of the request in case
365-
# urlconf changes at any point during the request
366-
_set_resolver_tags(pin, span, request)
367-
368367
request_headers = None
369368
if config._appsec_enabled:
370369
from ddtrace.appsec import _asm_request_context
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
fixes:
2+
- |
3+
django: This fix resolves an issue where 'span.resource' would not include the endpoint when a Handler was interrupted, such as in the case
4+
of gunicorn worker timeouts.

tests/contrib/django/test_django.py

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,9 @@
2323
from ddtrace.constants import ERROR_TYPE
2424
from ddtrace.constants import SAMPLING_PRIORITY_KEY
2525
from ddtrace.constants import USER_KEEP
26+
from ddtrace.contrib import trace_utils
2627
from ddtrace.contrib.django.patch import instrument_view
28+
from ddtrace.contrib.django.patch import traced_get_response
2729
from ddtrace.contrib.django.utils import get_request_uri
2830
from ddtrace.ext import http
2931
from ddtrace.ext import user
@@ -2394,3 +2396,22 @@ def test_django_get_user(client, test_spans):
23942396
assert root.get_tag(user.NAME) == "usr.name"
23952397
assert root.get_tag(user.ROLE) == "usr.role"
23962398
assert root.get_tag(user.SCOPE) == "usr.scope"
2399+
2400+
2401+
@pytest.mark.skipif(django.VERSION < (2, 0, 0), reason="")
2402+
def test_django_base_handler_failure(client, test_spans):
2403+
"""
2404+
This tests the failure mode seen with Gunicorn during timeouts, where the Django
2405+
Handler is terminated after <x> seconds and no response is returned by
2406+
django.core.handler.base.BaseHandler.get_response. We expect to populate the resource
2407+
with "GET <resource>" instead of what we did before this test "GET"
2408+
"""
2409+
trace_utils.unwrap(client.handler, "get_response")
2410+
with mock.patch.object(client.handler, "get_response", side_effect=Exception("test")):
2411+
trace_utils.wrap(client.handler, "get_response", traced_get_response(django))
2412+
try:
2413+
client.get("/")
2414+
except Exception:
2415+
pass # We expect an error
2416+
root = test_spans.get_root_span()
2417+
assert root.resource == "GET ^$"

0 commit comments

Comments
 (0)