Add Django ASGI support #391
Conversation
c605154 to
5cf5b16
Compare
|
Marking this as ready for review. There are still a few things to investigate:
cc @lonewolf3739, because of the context provided in #334. |
|
@adamantike is this still WIP? If so, please mark it as draft. 8000 |
Oh, I see, please edit the PR description instead. |
...tation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py
Show resolved
Hide resolved
.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py
Outdated
Show resolved
Hide resolved
instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py
Outdated
Show resolved
Hide resolved
instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py
Outdated
Show resolved
Hide resolved
|
I have rebased, fixed some comments, and added support for traced request attributes! The only issue I still find is this one:
However, it could be related to how Pytest handles ContextVars and contexts related to attached tokens, based on pytest-dev/pytest-asyncio#127 |
d7956d5 to
f33b9cd
Compare
|
Please do not merge yet. I just found that the I will need to debug what's going on. I'm open to any insights about what could be generating this issue. |
|
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
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
|
Filed #533, which circumvents the |
|
Rebased now that #533 was merged. All the |
| @@ -45,6 +45,8 @@ install_requires = | |||
| opentelemetry-semantic-conventions == 0.23.dev0 | |||
|
|
|||
| [options.extras_require] | |||
| asgi = | |||
| opentelemetry-instrumentation-asgi == 0.23.dev0 | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Show resolved
Hide resolved
.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py
Show resolved
Hide resolved
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| if is_asgi_request: | ||
| attributes = collect_request_attributes(request.scope) | ||
| else: | ||
| attributes = collect_request_attributes(request_meta) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Good idea, applied!
| # 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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,
)| "{} {}".format(response.status_code, response.reason_phrase), | ||
| response, | ||
| ) | ||
| if is_asgi_request: |
There was a problem hiding this comment.
does add_response_attributes only set a status code? The name suggests it does more than that.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
- ASGI:
| response, | ||
| ) | ||
| if is_asgi_request: | ||
| set_status_code(span, response.status_code) 10000 |
There was a problem hiding this comment.
HTTP status codes need to be translated to OpenTelemetry status codes before setting the span status e.g,
There was a problem hiding this comment.
I also think this is already fixed in the current set_status_code implementation.
|
@adamantike Hi, thanks for taking this up. Are you still planning to finish this? |
|
@owais, I'll be tackling your comments this week, so we can move forward! Thanks for all the feedback |
6b30587 to
cae7e53
Compare
ef98549 to
44cd1dc
Compare
|
@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.
44cd1dc to
7b82acb
Compare
|
No problem! I have rebased and resolved existing conflicts. |
|
extra_requires sounds better to me if we are pulling in deps that non-asgi users won't need. |
|
I agree, though by that same approach, a future breaking change should be introduced for 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. |
|
Oh I've been waiting on pypa/setuptools#1139 for longer than I remember. It's such an obvious feature to have :) |
Description
This diff adds
asgias an extra, and uses its methods if the current request is anASGIRequest. 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
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 TestMiddlewareAsgiDoes This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.