Add Django ASGI support #391

adamantike · 2021-03-31T21:06:30Z

Description

This diff adds asgi as an extra, and uses its methods if the current request is an ASGIRequest. Also adds support for traced request attributes, by retrieving them from request scope headers.

Fixes #165, fixes #185, fixes #280, fixes #334.

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

tox -e test-instrumentation-django -- -k TestMiddlewareAsgi

Does This PR Require a Core Repo Change?

No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

Followed the style guidelines of this project
Changelogs have been updated
Unit tests have been added
Documentation has been updated

adamantike · 2021-04-06T16:52:36Z

Marking this as ready for review. There are still a few things to investigate:

Tests are passing, but triggering log error: opentelemetry.context:__init__.py:146 Failed to detach context.
~~ASGI intrumentation's set_status_code method does not populate the status_text field. I think this should be tackled in a separate PR.~~ Not needed as of 370952f
Traced request attributes are not supported in this PR yet.

cc @lonewolf3739, because of the context provided in #334.

ocelotl · 2021-05-27T18:25:31Z

@adamantike is this still WIP? If so, please mark it as draft.

ocelotl · 2021-05-27T18:26:11Z

@adamantike is this still WIP? If so, please mark it as draft.

Oh, I see, please edit the PR description instead.

...tation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py

.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py

instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py

adamantike · 2021-05-30T16:09:56Z

I have rebased, fixed some comments, and added support for traced request attributes! The only issue I still find is this one:

Tests are passing, but triggering log error: opentelemetry.context:__init__.py:146 Failed to detach context.

However, it could be related to how Pytest handles ContextVars and contexts related to attached tokens, based on pytest-dev/pytest-asyncio#127

adamantike · 2021-06-03T04:13:34Z

Please do not merge yet. I just found that the Failed to detach context log messages also appear in a real Django project, when manually installing this branch, for a project using Gunicorn with Uvicorn workers.

I will need to debug what's going on. I'm open to any insights about what could be generating this issue.

adamantike · 2021-06-03T19:36:17Z

I have filed https://code.djangoproject.com/ticket/32815, to determine if the bug is in Django's codebase instead.

New-style middlewares were [introduced][django_1_10_changelog] in Django `1.10`, and also `settings.MIDDLEWARE_CLASSES` was [removed][django_2_0_changelog] in Django 2.0. This change migrates the Django middleware to conform with the new style. This is useful because it will help us solve the pending issue in open-telemetry#391. By having a single entrypoint to the middleware, `__call__`, which is [wrapped with `sync_to_async` just once][call_wrapped] for async requests, we avoid the [issue][asgiref_issue] where a `ContextVar` cannot be reset from a different context. With the current deprecated `MiddlewareMixin` way, both `process_request` and `process_response` were being [wrapped separately with `sync_to_async`][mixin_wrapped], which was the source of the mentioned issue. [django_1_10_changelog]: https://docs.djangoproject.com/en/3.2/releases/1.10/#new-style-middleware [django_2_0_changelog]: https://docs.djangoproject.com/en/3.2/releases/2.0/#features-removed-in-2-0 [call_wrapped]: https://github.com/django/django/blob/213850b4b9641bdcb714172999725ec9aa9c9e84/django/core/handlers/base.py#L54-L57 [mixin_wrapped]: https://github.com/django/django/blob/213850b4b9641bdcb714172999725ec9aa9c9e84/django/utils/deprecation.py#L137-L147 [asgiref_issue]: django/asgiref#267

adamantike · 2021-06-05T00:26:09Z

Filed #533, which circumvents the Failed to detach context error by simplifying how the Django middleware is wrapped with sync_to_async. With that change applied, the issue doesn't occur anymore!

adamantike · 2021-07-06T20:09:12Z

Rebased now that #533 was merged. All the Failed to detach context errors have been fixed, and this is now ready for another review!

owais · 2021-07-11T20:29:22Z

instrumentation/opentelemetry-instrumentation-django/setup.cfg

@@ -45,6 +45,8 @@ install_requires =
    opentelemetry-semantic-conventions == 0.23.dev0

 [options.extras_require]
+asgi =
+    opentelemetry-instrumentation-asgi == 0.23.dev0


This should be documented so people using ASGI with Django realize they need to install these extra deps to make it work.

How do ASGI deps work with Django? Do regular Django projects need to install additional packages to make ASGI work or does Django now ship with (or depend on) bundled ASGI module. If some level of ASGI support is always present in Django now, may be we can add this as a main dependency instead?

This is a good discussion topic. Django doesn't require any additional/extra dependencies for ASGI to work (aside from running it using Uvicorn/Daphne), and bundles asgiref since the async introduction in Django 3.0: django/django@a415ce7#diff-60f61ab7a8d1910d86d9fda2261620314edcae5894d5aaa236b821c7256badd7

This dependency doesn't include other heavy dependencies that aren't already required by opentelemetry-instrumentation-wsgi. However, we also have to consider that ASGI is only present since Django 3.x, while this project tries to maintain support for older Django versions.

I think the only missing change on this PR, if we go with adding this to install_requires, is to make _is_asgi_supported also depend on DJANGO_3_0 being True, as we won't receive any ImportError anymore when importing ASGI dependencies.

.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py

owais · 2021-07-11T20:45:07Z

.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py

@@ -141,12 +166,23 @@ def process_request(self, request):
        if self._excluded_urls.url_disabled(request.build_absolute_uri("?")):
            return

+        is_asgi_request = _is_asgi_request(request)
+        if is_asgi_request and not _is_asgi_supported:
+            return


Can a django service/middleware handle both WSGI and ASGI requests at the same time? If it can only handle one kind, may be this can can run in middleware init and result can be stored in an instance variable.

At least is_asgi_supported can be called before _is_asgi_request and that way we can avoid running the type check for non-ASGI requests.

Can a django service/middleware handle both WSGI and ASGI requests at the same time?

Yes, it can handle both, so we need to check which kind of request we're receiving on each call.

At least is_asgi_supported can be called before _is_asgi_request and that way we can avoid running the type check for non-ASGI requests.

As we're using is_asgi_request multiple times on this method, I don't think we can avoid the type-check call.

If ASGI is not supported, we need to know if the current request is ASGI.

If ASGI is supported, we need to know it nevertheless, to decide on which carrier, carrier_getter, etc. to use.

owais · 2021-07-11T20:47:18Z

.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py

+        if is_asgi_request:
+            attributes = collect_request_attributes(request.scope)
+        else:
+            attributes = collect_request_attributes(request_meta)


We could also set a variable named carrier to request.scope or request_meta in the similar is_asgi_request check above and here just call the function on carrier

Good idea, applied!

owais · 2021-07-11T20:50:06Z

.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py

+                # ASGI requests include extra attributes in request.scope.headers. For this reason,
+                # we need to build an object with the union of `request` and `request.scope.headers`
+                # contents, for the extract_attributes_from_object function to be able to retrieve
+                # attributes from it.


we don't have to use extract_attribute_from_object. I think it'd be simpler to just write a dedicated extract function for Django ASGIRequest objects instead of doing all this just to be able to use the existing func.

Would it make more sense, to keep using the extract_attributes_from_object util, to go with a composition approach?

attributes = extract_attributes_from_object( request, self._traced_request_attrs, attributes ) if is_asgi_request: # ASGI requests include extra attributes in request.scope.headers. attributes = extract_attributes_from_object( types.SimpleNamespace(**{ name.decode("latin1"): value.decode("latin1") for name, value in request.scope.get("headers", []) }), self._traced_request_attrs, attributes, )

owais · 2021-07-11T20:52:49Z

.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py

-                "{} {}".format(response.status_code, response.reason_phrase),
-                response,
-            )
+            if is_asgi_request:


does add_response_attributes only set a status code? The name suggests it does more than that.

It does actually. It adds HTTP status code attribute in addition to setting status on the span. Plus is also has some additional logic which we miss out on here. Any reason we cannot just use that function for ASGI as well?

opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py

Lines 188 to 208 in e129174

def add_response_attributes(

span, start_response_status, response_headers

): # pylint: disable=unused-argument

"""Adds HTTP response attributes to span using the arguments

passed to a PEP3333-conforming start_response callable."""

if not span.is_recording():

return

status_code, _ = start_response_status.split(" ", 1)

try:

status_code = int(status_code)

except ValueError:

span.set_status(

Status(

StatusCode.ERROR,

"Non-integer HTTP status: " + repr(status_code),

)

)

else:

span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code)

span.set_status(Status(http_status_to_status_code(status_code)))

As the functions are right now, I don't see any needed changes for both WSGI and ASGI to make the same changes to the current span:

WSGI:

opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py

Lines 188 to 208 in e129174

def add_response_attributes(

span, start_response_status, response_headers

): # pylint: disable=unused-argument

"""Adds HTTP response attributes to span using the arguments

passed to a PEP3333-conforming start_response callable."""

if not span.is_recording():

return

status_code, _ = start_response_status.split(" ", 1)

try:

status_code = int(status_code)

except ValueError:

span.set_status(

Status(

StatusCode.ERROR,

"Non-integer HTTP status: " + repr(status_code),

)

)

else:

span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code)

span.set_status(Status(http_status_to_status_code(status_code)))

ASGI:

opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py

Lines 125 to 140 in e129174

def set_status_code(span, status_code):

"""Adds HTTP response attributes to span using the status_code argument."""

if not span.is_recording():

return

try:

status_code = int(status_code)

except ValueError:

span.set_status(

Status(

StatusCode.ERROR,

"Non-integer HTTP status: " + repr(status_code),

)

)

else:

span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code)

span.set_status(Status(http_status_to_status_code(status_code)))

owais · 2021-07-11T20:55:42Z

.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py

-                response,
-            )
+            if is_asgi_request:
+                set_status_code(span, response.status_code)


HTTP status codes need to be translated to OpenTelemetry status codes before setting the span status e.g,

opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py

Line 208 in e129174

span.set_status(Status(http_status_to_status_code(status_code)))

I also think this is already fixed in the current set_status_code implementation.

owais · 2021-08-19T16:02:06Z

@adamantike Hi, thanks for taking this up. Are you still planning to finish this?

adamantike · 2021-08-23T14:08:09Z

@owais, I'll be tackling your comments this week, so we can move forward! Thanks for all the feedback

adamantike · 2021-09-24T14:38:49Z

@owais @ocelotl, sorry for the delay. This is now ready for another review!

owais · 2021-10-11T17:29:26Z

@adamantike sorry for the delay. can you please resolve the conflicts and rebase/merge with main? We can try to include it in the release this week.

This diff adds `asgi` as an extra, and uses its methods if the current request is an `ASGIRequest`. I still need to dig deeper in the current test suite, to find a way to duplicate the tests in `instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py`, but using an [`AsyncClient`](https://docs.djangoproject.com/en/3.1/topics/testing/tools/#testing-asynchronous-code). Fixes open-telemetry#165, open-telemetry#185, open-telemetry#280, open-telemetry#334.

Add support for the `extract_attributes_from_object` function to retrieve traced attributes that are added to `request.scope.headers`. This is added based on the code found in [Django's `AsyncRequestFactory`]( https://github.com/django/django/blob/a948d9df394aafded78d72b1daa785a0abfeab48/django/test/client.py#L550-L553), which indicates that any extra argument sent to the `AsyncClient` is set in the `request.scope.headers` list. Also: * Stop inheriting from `SimpleTestCase` in async Django tests, to simplify test inheritance. * Correctly configure Django settings in tests, before calling parent's `setUpClass` method. * Rebase and port the latest changes to Django WSGI tests, to be supported by ASGI ones.

adamantike · 2021-10-11T19:41:18Z

No problem! I have rebased and resolved existing conflicts.
I think the pending discussion here is regarding keeping opentelemetry-instrumentation-asgi in extras_require (and where/how to document it), or move it to install_requires.

owais · 2021-10-11T19:51:49Z

extra_requires sounds better to me if we are pulling in deps that non-asgi users won't need.

adamantike · 2021-10-12T01:23:59Z

I agree, though by that same approach, a future breaking change should be introduced for wsgi to also become an extra, so users exclusively having ASGI don't get WSGI dependencies.

This is where pypa/setuptools#1139 would come useful if implemented. If no extras are specified, you get both WSGI and ASGI. If you want to cut down on dependencies, you need to specify which flow your app is going to use.

owais · 2021-10-12T12:27:43Z

Oh I've been waiting on pypa/setuptools#1139 for longer than I remember. It's such an obvious feature to have :)

adamantike force-pushed the django-asgi branch 3 times, most recently from c605154 to 5cf5b16 Compare April 6, 2021 16:47

adamantike marked this pull request as ready for review April 6, 2021 16:48

adamantike requested review from a team, aabmass and ocelotl and removed request for a team April 6, 2021 16:48

ocelotl suggested changes May 27, 2021

View reviewed changes

adamantike force-pushed the django-asgi branch from 5cf5b16 to 1750103 Compare May 30, 2021 16:02

adamantike force-pushed the django-asgi branch 4 times, most recently from d7956d5 to f33b9cd Compare May 30, 2021 19:19

ocelotl approved these changes Jun 2, 2021

View reviewed changes

adamantike force-pushed the django-asgi branch from f33b9cd to a01105c Compare June 3, 2021 03:19

adamantike marked this pull request as draft June 3, 2021 04:11

adamantike mentioned this pull request Jun 4, 2021

How to reset ContextVar in separate sync_to_async calls django/asgiref#267

Open

adamantike mentioned this pull request Jun 5, 2021

Migrate Django middleware to new-style #533

Merged

7 tasks

adamantike force-pushed the django-asgi branch from 3440e70 to 646d41f Compare July 6, 2021 20:08

owais reviewed Jul 11, 2021

View reviewed changes

owais requested changes Jul 11, 2021

View reviewed changes

owais self-assigned this Aug 19, 2021

adamantike force-pushed the django-asgi branch from 6b30587 to cae7e53 Compare September 21, 2021 19:06

lzchen approved these changes Sep 27, 2021

View reviewed changes

adamantike force-pushed the django-asgi branch from ef98549 to 44cd1dc Compare September 28, 2021 02:38

owais approved these changes Oct 11, 2021

View reviewed changes

adamantike added 8 commits October 11, 2021 16:35

Work on unit tests with AsyncClient

6a217de

Fix some of the pending unit tests

6aa5fa4

Fix linting issues

5b523af

Tackle PR comments

e5a2949

Rebase and apply @owais comments

20d19a8

Always detach token from request.META

7b82acb

adamantike force-pushed the django-asgi branch from 44cd1dc to 7b82acb Compare October 11, 2021 19:38

Merge branch 'main' into django-asgi

332fa25

lzchen merged commit 5105820 into open-telemetry:main Oct 12, 2021

adamantike deleted the django-asgi branch October 12, 2021 17:47

This was referenced Oct 15, 2021

Error in opentelemetry/instrumentation/django/middleware.py related to "wsgi.url_scheme" #280

Closed

Django ASGI not supported #165

Closed

owais mentioned this pull request Apr 12, 2022

Document how to use Django with ASGI #1051

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Django ASGI support #391

Add Django ASGI support #391

adamantike commented Mar 31, 2021 •

edited

Loading

adamantike commented Apr 6, 2021 •

edited

Loading

ocelotl commented May 27, 2021

ocelotl commented May 27, 2021

adamantike commented May 30, 2021 •

edited

Loading

adamantike commented Jun 3, 2021 •

edited

Loading

adamantike commented Jun 3, 2021

adamantike commented Jun 5, 2021

adamantike commented Jul 6, 2021

owais Jul 11, 2021

adamantike Sep 21, 2021

owais Jul 11, 2021

owais Jul 11, 2021

adamantike Sep 21, 2021 •

edited

Loading

owais Jul 11, 2021

adamantike Sep 21, 2021

owais Jul 11, 2021

adamantike Sep 21, 2021

owais Jul 11, 2021

owais Jul 11, 2021

adamantike Sep 21, 2021

owais Jul 11, 2021

adamantike Sep 21, 2021

owais commented Aug 19, 2021

adamantike commented Aug 23, 2021

adamantike commented Sep 24, 2021

owais commented Oct 11, 2021

adamantike commented Oct 11, 2021

owais commented Oct 11, 2021

adamantike commented Oct 12, 2021

owais commented Oct 12, 2021

	def add_response_attributes(
	span, start_response_status, response_headers
	): # pylint: disable=unused-argument
	"""Adds HTTP response attributes to span using the arguments
	passed to a PEP3333-conforming start_response callable."""
	if not span.is_recording():
	return
	status_code, _ = start_response_status.split(" ", 1)

	try:
	status_code = int(status_code)
	except ValueError:
	span.set_status(
	Status(
	StatusCode.ERROR,
	"Non-integer HTTP status: " + repr(status_code),
	)
	)
	else:
	span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code)
	span.set_status(Status(http_status_to_status_code(status_code)))

	def set_status_code(span, status_code):
	"""Adds HTTP response attributes to span using the status_code argument."""
	if not span.is_recording():
	return
	try:
	status_code = int(status_code)
	except ValueError:
	span.set_status(
	Status(
	StatusCode.ERROR,
	"Non-integer HTTP status: " + repr(status_code),
	)
	)
	else:
	span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code)
	span.set_status(Status(http_status_to_status_code(status_code)))

Add Django ASGI support #391

Add Django ASGI support #391

Conversation

adamantike commented Mar 31, 2021 • edited Loading

Description

Type of change

How Has This Been Tested?

Does This PR Require a Core Repo Change?

Checklist:

adamantike commented Apr 6, 2021 • edited Loading

ocelotl commented May 27, 2021

ocelotl commented May 27, 2021

adamantike commented May 30, 2021 • edited Loading

adamantike commented Jun 3, 2021 • edited Loading

adamantike commented Jun 3, 2021

adamantike commented Jun 5, 2021

adamantike commented Jul 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamantike Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owais commented Aug 19, 2021

adamantike commented Aug 23, 2021

adamantike commented Sep 24, 2021

owais commented Oct 11, 2021

adamantike commented Oct 11, 2021

owais commented Oct 11, 2021

adamantike commented Oct 12, 2021

owais commented Oct 12, 2021

adamantike commented Mar 31, 2021 •

edited

Loading

adamantike commented Apr 6, 2021 •

edited

Loading

adamantike commented May 30, 2021 •

edited

Loading

adamantike commented Jun 3, 2021 •

edited

Loading

adamantike Sep 21, 2021 •

edited

Loading