Auto-instrument Django with WSGI middleware #1812

Kyle-Verhoog · 2020-11-18T02:43:35Z

Description

Auto-instrument Django's get_wsgi_application with DDWSGIMiddleware. This feature also enables the tracing of StreamingHttpResponses which was not previously possible by just patching Django.

Dependent on #1808.

The app in DataDog/trace-examples#106 is also used to test this feature.

Checklist

Ensure distributed tracing is correct when using the WSGI middleware with web framework integrations.
Use span_modifier to set resource, service
Entry added to release notes using reno
Tests provided; and/or
Description of manual testing performed and explanation is included in the code and/or PR.
~~Library documentation is updated.~~
~~Corp site documentation is updated (link to the PR).~~

brettlangdon

How does this help/hurt existing issues of bubbling metadata to the trace root span (including but not limited to, stack traces)?

Is there any place in django where we explicitly grab and root span and modify it?

Kyle-Verhoog · 2020-11-18T18:37:34Z

@brettlangdon the django integration doesn't reach for the root span at all so there shouldn't be issues there. However something that is going to get messy is how to deal with distributed tracing. One approach might be to strip the tracing headers out of the request in the WSGI/ASGI middlewares so that they aren't consumed downstream by Django.

brettlangdon · 2020-11-18T19:18:27Z

Does this WSGI middleware provide any additional visibility or benefit to the Django integration?

Should we just use this WSGI middleware as a reusable helper across all web apps?

Kyle-Verhoog · 2020-11-18T20:11:50Z

Does this WSGI middleware provide any additional visibility or benefit to the Django integration?

From the PR description:

This feature also enables the tracing of StreamingHttpResponses which was not previously possible by just patching Django.

Should we just use this WSGI middleware as a reusable helper across all web apps?

Yes that's the plan

brettlangdon · 2020-11-18T20:18:43Z

Sorry, not sure how I glossed over that in the description....

this will change the service root operation name though, meaning people's links/dashboards/monitors might break.

We probably need to figure out how to make this change more carefully, e.g. probably need to introduce _dd.measured: 1 for the django.request spans to ensure we continue to compute metrics from that span.

Can we make it so the wsgi.request is actually just the django.request span?

If we did that, probably means we need a way to bubble up django specific metadata into that span, but might be the safer way to handle this change, changing the structure of the trace (at least who the root span is) can cause issues... might be better off avoiding if we can.

Kyle-Verhoog · 2020-11-18T20:29:55Z

We probably need to figure out how to make this change more carefully, e.g. probably need to introduce _dd.measured: 1 for the django.request spans to ensure we continue to compute metrics from that span.

We actually don't do this currently. So for anyone to actually have their Django spans measured now have to add a retention filter in the app. Which should continue to work no matter the trace shape I believe?

Can we make it so the wsgi.request is actually just the django.request span?
A couple issues with this:

It's not true. The WSGI handling is done by the WSGI server, not django, as seen with StreamingHttpResponses
It would still mess up metrics/monitors because now the request is going to encompass a larger time span from before.

It might just have to be a breaking change that we disclaim in the release notes.

Co-authored-by: Tahir H. Butt <[email protected]> Co-authored-by: Julien Danjou <[email protected]>

…h wsgi, added django resource and service names to top level wsgi span

…o wsgi-django

…cific

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* Add arch64 wheel build support (DataDog#1917) Co-authored-by: Tahir H. Butt <[email protected]> Co-authored-by: Julien Danjou <[email protected]> * Added status_msg as a param in set_http_meta, added it as a key in http.py * Added test clause in test case for set_http_meta to include status_msg * Changed status_msg assertion to include six module text_type Co-authored-by: odidev <[email protected]> Co-authored-by: Tahir H. Butt <[email protected]> Co-authored-by: Julien Danjou <[email protected]> Co-authored-by: Kyle Verhoog <[email protected]>

…cific

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

…ataDog#1967) * chore(profiling): decrease max CPU usage for stack profiling to 1% * Update releasenotes/notes/profiling-lower-max-cpu-usage-default-59458d90e0d6d33d.yaml Co-authored-by: Kyle Verhoog <[email protected]>

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* First version of the cherrypy middleware for datadog, tests will come later * Added tests based on Flask Middleware and updated to match expected outputs. * Added release notes * Flake8 fixes * Added cherrypy to CircleCI job * Updates following PR feedback. * Updated following second review * Refactored service_name to service * Updated documentation. * Fix bad naming causing build docs failure. Co-authored-by: Kyle Verhoog <[email protected]> Co-authored-by: Alex Cowan <[email protected]> Co-authored-by: Kyle Verhoog <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

This gets it out of the context which will help with refactoring.

These tests depended on implementation details of the context which are not described in our public API. In my opinion it's safe to remove the dependence so long as the functionality is still asserted. Co-authored-by: Julien Danjou <[email protected]>

These tests depended on the implementation details of the context management for gevent. There exist very similar tests that assert the correct context management behaviour which are less brittle. Co-authored-by: Julien Danjou <[email protected]>

I find it handy to be able to notice at a glance PR that are in conflict with a label, so we know there's, e.g., no point in reviewing them now. This also notifies the author they need to update the PR to get it working.

) Since the vendoring contextvars (thread-local by default) there is no longer a need for ContextManagers.

* span: make tests context independent Also make the semi-integration tests real integration tests. * update docstring, remove tracer usage

* asyncio: clean-up tests - Remove dependence on context implementation - Skip tests that aren't necessary * add back assertions, add docs Co-authored-by: Julien Danjou <[email protected]>

Since the integration is disabled by default, it makes sense that if a user opts into using it that distributed tracing should be enabled out of the box. Also updates the docs to match our new format.

* tests(profiling): disable coverage for subprocesses Profiling tests use subprocesses in several cases which might mess with coverage. Let's disable coverage in Python subprocesses. * feat(profiling): support most uwsgi modes This adds support for many of the uWSGI configuration mode (master, lazy-apps, multi process, etc) by introspecting uWSGI options and leveraging its hooks. This makes sure the profiler is correctly started in the children processes even when started from the master/parent process.

This in general is fine but if there is a parent span of the same service (eg. WSGI) then metrics won't be calculated for django spans. With this change metrics will be generated regardless. Co-authored-by: Brett Langdon <[email protected]>

…o wsgi-django

Kyle-Verhoog · 2021-02-01T22:04:50Z

ddtrace/contrib/wsgi/__init__.py

+Enabling
+~~~~~~~~
+
+The WSGI instrumentation will be automatically added to a Django or Flask


Suggested change

The WSGI instrumentation will be automatically added to a Django or Flask

The WSGI instrumentation will be automatically added to Django

Kyle-Verhoog · 2021-02-01T22:04:58Z

ddtrace/contrib/wsgi/__init__.py

+~~~~~~~~
+
+The WSGI instrumentation will be automatically added to a Django or Flask
+application when using :ref:`ddtrace-run<ddtracerun>` or


Suggested change

application when using :ref:`ddtrace-run<ddtracerun>` or

applications when using :ref:`ddtrace-run<ddtracerun>` or

Kyle-Verhoog · 2021-05-14T20:45:23Z

Going to close this for now as there isn't a non-breaking way to introduce instrumenting Django by default with the WSGI integration.

Users are free to use the WSGI integration to wrap their Django app and gain the additional visibility if desired.

Kyle-Verhoog force-pushed the wsgi-django branch from 200aa8d to 387042e Compare November 18, 2020 03:42

brettlangdon reviewed Nov 18, 2020

View reviewed changes

odidev and others added 5 commits January 15, 2021 17:32

Add arch64 wheel build support (DataDog#1917)

97251c0

Co-authored-by: Tahir H. Butt <[email protected]> Co-authored-by: Julien Danjou <[email protected]>

Merge branch 'master' of github.com:DataDog/dd-trace-py

843f87f

Merge branch 'master' of github.com:DataDog/dd-trace-py

1912b09

Merge branch 'master' of github.com:DataDog/dd-trace-py

7f8a036

Merge branch 'master' of github.com:DataDog/dd-trace-py

77560ea

Kyle-Verhoog mentioned this pull request Jan 27, 2021

Add WSGI middleware #1808

Merged

27 tasks

auto-instrument django wsgi applications

17fff49

Kyle-Verhoog force-pushed the wsgi-django branch from 387042e to 17fff49 Compare January 28, 2021 17:53

Yun-Kim and others added 15 commits February 1, 2021 16:47

Added distributed tracing default false when instrumenting django wit…

98fdf54

…h wsgi, added django resource and service names to top level wsgi span

Merge branch 'master' of github.com:DataDog/dd-trace-py into wsgi-django

da4ff4f

Merge branch 'master' of github.com:DataDog/dd-trace-py

c77a236

Modified snapshot tests for django 1.11 and 2.1

56d141d

auto-instrument django wsgi applications

07961ba

Added distributed tracing default false when instrumenting django wit…

a294e42

…h wsgi, added django resource and service names to top level wsgi span

Modified snapshot tests for django 1.11 and 2.1

2313ce0

Merge branch 'wsgi-django' of github.com:Kyle-Verhoog/dd-trace-py int…

9ce11a5

…o wsgi-django

Merge branch 'master' of github.com:DataDog/dd-trace-py into wsgi-django

a97a72e

Changed root span service/resource renaming to be wsgi-middleware spe…

3eba509

…cific

Fixed snapshot test for django 2.1

70e8eab

Merge branch 'master' into wsgi-django

dc4d139

Merge branch 'master' of github.com:DataDog/dd-trace-py into wsgi-django

a6dc762

auto-instrument django wsgi applications

110d7c5

requests: remove context dependence from tests (DataDog#1973)

32ebce0

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

Kyle-Verhoog and others added 21 commits February 10, 2021 13:44

aiohttp: remove context dependence (DataDog#1977)

6389d40

Changed root span service/resource renaming to be wsgi-middleware spe…

935b59d

…cific

Update integration.md (DataDog#1979)

dbb8e04

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

format futures (DataDog#1976)

24e6a9b

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

tracer: move hostname tagging to start_span (DataDog#1984)

bef81d1

This gets it out of the context which will help with refactoring.

chore(mergify): warn on PR conflict (DataDog#1988)

30c5394

I find it handy to be able to notice at a glance PR that are in conflict with a label, so we know there's, e.g., no point in reviewing them now. This also notifies the author they need to update the PR to get it working.

context: _dd_origin -> dd_origin (DataDog#1982)

b6e4d1a

refactor(core): roll context manager into context provider (DataDog#1983

5a2de94

) Since the vendoring contextvars (thread-local by default) there is no longer a need for ContextManagers.

span: make tests context independent (DataDog#1981)

f4ad7cc

* span: make tests context independent Also make the semi-integration tests real integration tests. * update docstring, remove tracer usage

asyncio: clean-up tests (DataDog#1985)

1a22719

* asyncio: clean-up tests - Remove dependence on context implementation - Skip tests that aren't necessary * add back assertions, add docs Co-authored-by: Julien Danjou <[email protected]>

httplib: enable distributed tracing by default (DataDog#1991)

0457e5e

Since the integration is disabled by default, it makes sense that if a user opts into using it that distributed tracing should be enabled out of the box. Also updates the docs to match our new format.

ci: disable output buffering (DataDog#1994)

c02e844

Merge branch 'wsgi-django' of github.com:Kyle-Verhoog/dd-trace-py int…

92c5e4e

…o wsgi-django

Merge branch 'master' into wsgi-django

b1e4f91

Kyle-Verhoog commented May 14, 2021

View reviewed changes

Kyle-Verhoog closed this May 14, 2021

Kyle-Verhoog mentioned this pull request Jul 16, 2021

Add ASGI support to Django #2656

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Auto-instrument Django with WSGI middleware #1812

Auto-instrument Django with WSGI middleware #1812

Uh oh!

Kyle-Verhoog commented Nov 18, 2020 •

edited

Loading

Uh oh!

brettlangdon left a comment

Uh oh!

Kyle-Verhoog commented Nov 18, 2020

Uh oh!

brettlangdon commented Nov 18, 2020

Uh oh!

Kyle-Verhoog commented Nov 18, 2020

Uh oh!

brettlangdon commented Nov 18, 2020

Uh oh!

Kyle-Verhoog commented Nov 18, 2020

Uh oh!

Kyle-Verhoog Feb 1, 2021

Uh oh!

Kyle-Verhoog Feb 1, 2021

Uh oh!

Kyle-Verhoog commented May 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

	The WSGI instrumentation will be automatically added to a Django or Flask
	The WSGI instrumentation will be automatically added to Django

	application when using :ref:`ddtrace-run<ddtracerun>` or
	applications when using :ref:`ddtrace-run<ddtracerun>` or

Auto-instrument Django with WSGI middleware #1812

Auto-instrument Django with WSGI middleware #1812

Uh oh!

Conversation

Kyle-Verhoog commented Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

brettlangdon left a comment

Choose a reason for hiding this comment

Uh oh!

Kyle-Verhoog commented Nov 18, 2020

Uh oh!

brettlangdon commented Nov 18, 2020

Uh oh!

Kyle-Verhoog commented Nov 18, 2020

Uh oh!

brettlangdon commented Nov 18, 2020

Uh oh!

Kyle-Verhoog commented Nov 18, 2020

Uh oh!

Kyle-Verhoog Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

Kyle-Verhoog Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

Kyle-Verhoog commented May 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Kyle-Verhoog commented Nov 18, 2020 •

edited

Loading