From d32848487ca07e58eca03b0c9998fb8a10e33509 Mon Sep 17 00:00:00 2001 From: Michael Manganiello Date: Wed, 31 Mar 2021 17:57:26 -0300 Subject: [PATCH 1/8] Add Django ASGI support 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 #165, #185, #280, #334. --- .../setup.cfg | 2 + .../instrumentation/django/middleware.py | 59 ++++++++++++++++--- tox.ini | 2 +- 3 files changed, 54 insertions(+), 9 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-django/setup.cfg b/instrumentation/opentelemetry-instrumentation-django/setup.cfg index 67eb7c52b5..d9ba8f7657 100644 --- a/instrumentation/opentelemetry-instrumentation-django/setup.cfg +++ b/instrumentation/opentelemetry-instrumentation-django/setup.cfg @@ -45,6 +45,8 @@ install_requires = opentelemetry-semantic-conventions == 0.24b0 [options.extras_require] +asgi = + opentelemetry-instrumentation-asgi == 0.22.dev0 test = opentelemetry-test == 0.24b0 diff --git a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py index f071ba6e56..806da09309 100644 --- a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py +++ b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py @@ -26,7 +26,7 @@ from opentelemetry.instrumentation.utils import extract_attributes_from_object from opentelemetry.instrumentation.wsgi import ( add_response_attributes, - collect_request_attributes, + collect_request_attributes as wsgi_collect_request_attributes, wsgi_getter, ) from opentelemetry.propagate import extract @@ -68,6 +68,25 @@ def __call__(self, request): MiddlewareMixin = object +try: + from django.core.handlers.asgi import ASGIRequest +except ImportError: + ASGIRequest = None + +try: + from opentelemetry.instrumentation.asgi import ( + asgi_getter, + collect_request_attributes as asgi_collect_request_attributes, + set_status_code, + ) + _is_asgi_supported = True +except ImportError: + asgi_getter = None + asgi_collect_request_attributes = None + set_status_code = None + _is_asgi_supported = False + + _logger = getLogger(__name__) _attributes_by_preference = [ [ @@ -132,6 +151,9 @@ def _get_span_name(request): except Resolver404: return f"HTTP {request.method}" + def _is_asgi_request(self, request): + return ASGIRequest and isinstance(request, ASGIRequest) + def process_request(self, request): # request.META is a dictionary containing all available HTTP headers # Read more about request.META here: @@ -140,12 +162,23 @@ def process_request(self, request): if self._excluded_urls.url_disabled(request.build_absolute_uri("?")): return + is_asgi_request = self._is_asgi_request(request) + if is_asgi_request and not _is_asgi_supported: + return + # pylint:disable=W0212 request._otel_start_time = time() request_meta = request.META - token = attach(extract(request_meta, getter=wsgi_getter)) + if is_asgi_request: + carrier_getter = asgi_getter + collect_request_attributes = asgi_collect_request_attributes + else: + carrier_getter = wsgi_getter + collect_request_attributes = wsgi_collect_request_attributes + + token = attach(extract(request_meta, getter=carrier_getter)) span = self._tracer.start_span( self._get_span_name(request), @@ -207,15 +240,22 @@ def process_response(self, request, response): if self._excluded_urls.url_disabled(request.build_absolute_uri("?")): return response + is_asgi_request = self._is_asgi_request(request) + if is_asgi_request and not _is_asgi_supported: + return + activation = request.META.pop(self._environ_activation_key, None) span = request.META.pop(self._environ_span_key, None) if activation and span: - add_response_attributes( - span, - f"{response.status_code} {response.reason_phrase}", - response, - ) + if is_asgi_request: + set_status_code(request.META[self._environ_span_key], response.status_code) + else: + add_response_attributes( + span, + f"{response.status_code} {response.reason_phrase}", + response, + ) propagator = get_global_response_propagator() if propagator: @@ -238,7 +278,10 @@ def process_response(self, request, response): activation.__exit__(None, None, None) if self._environ_token in request.META.keys(): - detach(request.environ.get(self._environ_token)) + if is_asgi_request: + detach(request.META.get(self._environ_token)) + else: + detach(request.environ.get(self._environ_token)) request.META.pop(self._environ_token) return response diff --git a/tox.ini b/tox.ini index d681345fb6..425c3b02f1 100644 --- a/tox.ini +++ b/tox.ini @@ -253,7 +253,7 @@ commands_pre = falcon{2,3},flask,django,pyramid,tornado,starlette,fastapi,aiohttp,asgi,requests,urllib,urllib3,wsgi: pip install {toxinidir}/util/opentelemetry-util-http[test] wsgi,falcon{2,3},flask,django,pyramid: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-wsgi[test] - asgi,starlette,fastapi: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-asgi[test] + asgi,django,starlette,fastapi: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-asgi[test] asyncpg: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-asyncpg[test] From 6a217dedbb5b3d3713a8f39ea8fb941745c4cb89 Mon Sep 17 00:00:00 2001 From: Michael Manganiello Date: Tue, 6 Apr 2021 12:53:45 -0300 Subject: [PATCH 2/8] Work on unit tests with AsyncClient --- .../instrumentation/django/middleware.py | 5 +- .../tests/test_middleware.py | 27 +- .../tests/test_middleware_asgi.py | 274 ++++++++++++++++++ .../tests/views.py | 30 ++ 4 files changed, 324 insertions(+), 12 deletions(-) create mode 100644 instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py diff --git a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py index 806da09309..5804aefbe8 100644 --- a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py +++ b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py @@ -188,7 +188,10 @@ def process_request(self, request): ), ) - attributes = collect_request_attributes(request_meta) + if is_asgi_request: + attributes = collect_request_attributes(request.scope) + else: + attributes = collect_request_attributes(request_meta) if span.is_recording(): attributes = extract_attributes_from_object( diff --git a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py index bec805ffd8..1ccdcd7102 100644 --- a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py @@ -15,12 +15,12 @@ from sys import modules from unittest.mock import Mock, patch -from django import VERSION -from django.conf import settings +from django import VERSION, conf from django.conf.urls import url from django.http import HttpRequest, HttpResponse -from django.test import Client +from django.test.client import Client from django.test.utils import setup_test_environment, teardown_test_environment +from django.urls import re_path from opentelemetry.instrumentation.django import ( DjangoInstrumentor, @@ -57,13 +57,13 @@ DJANGO_2_2 = VERSION >= (2, 2) urlpatterns = [ - url(r"^traced/", traced), - url(r"^route/(?P[0-9]{4})/template/$", traced_template), - url(r"^error/", error), - url(r"^excluded_arg/", excluded), - url(r"^excluded_noarg/", excluded_noarg), - url(r"^excluded_noarg2/", excluded_noarg2), - url(r"^span_name/([0-9]{4})/$", route_span_name), + re_path(r"^traced/", traced), + re_path(r"^route/(?P[0-9]{4})/template/$", traced_template), + re_path(r"^error/", error), + re_path(r"^excluded_arg/", excluded), + re_path(r"^excluded_noarg/", excluded_noarg), + re_path(r"^excluded_noarg2/", excluded_noarg2), + re_path(r"^span_name/([0-9]{4})/$", route_span_name), ] _django_instrumentor = DjangoInstrumentor() @@ -72,7 +72,7 @@ class TestMiddleware(TestBase, WsgiTestBase): @classmethod def setUpClass(cls): super().setUpClass() - settings.configure(ROOT_URLCONF=modules[__name__]) + conf.settings.configure(ROOT_URLCONF=modules[__name__]) def setUp(self): super().setUp() @@ -105,6 +105,11 @@ def tearDown(self): teardown_test_environment() _django_instrumentor.uninstrument() + @classmethod + def tearDownClass(cls): + super().tearDownClass() + conf.settings = conf.LazySettings() + def test_templated_route_get(self): Client().get("/route/2020/template/") diff --git a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py new file mode 100644 index 0000000000..8a8ce8bd9c --- /dev/null +++ b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py @@ -0,0 +1,274 @@ +# Copyright The OpenTelemetry Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from sys import modules +from unittest.mock import Mock, patch + +from django import VERSION, conf +from django.test import SimpleTestCase +from django.test.utils import setup_test_environment, teardown_test_environment +from django.urls import re_path +import pytest + +from opentelemetry.instrumentation.django import DjangoInstrumentor +from opentelemetry.test.test_base import TestBase +from opentelemetry.trace import SpanKind, StatusCode +from opentelemetry.util.http import get_excluded_urls, get_traced_request_attrs + +# pylint: disable=import-error +from .views import ( + async_error, + async_excluded, + async_excluded_noarg, + async_excluded_noarg2, + async_route_span_name, + async_traced, + async_traced_template, +) + +DJANGO_3_1 = VERSION >= (3, 1) + +if DJANGO_3_1: + from django.test.client import AsyncClient +else: + AsyncClient = None + +urlpatterns = [ + re_path(r"^traced/", async_traced), + re_path(r"^route/(?P[0-9]{4})/template/$", async_traced_template), + re_path(r"^error/", async_error), + re_path(r"^excluded_arg/", async_excluded), + re_path(r"^excluded_noarg/", async_excluded_noarg), + re_path(r"^excluded_noarg2/", async_excluded_noarg2), + re_path(r"^span_name/([0-9]{4})/$", async_route_span_name), +] +_django_instrumentor = DjangoInstrumentor() + + +@pytest.mark.skipif(not DJANGO_3_1, reason="AsyncClient implemented since Django 3.1") +class TestMiddlewareAsgi(SimpleTestCase, TestBase): + + @classmethod + def setUpClass(cls): + super().setUpClass() + conf.settings.configure(ROOT_URLCONF=modules[__name__]) + + def setUp(self): + super().setUp() + setup_test_environment() + _django_instrumentor.instrument() + self.env_patch = patch.dict( + "os.environ", + { + "OTEL_PYTHON_DJANGO_EXCLUDED_URLS": "http://testserver/excluded_arg/123,excluded_noarg", + "OTEL_PYTHON_DJANGO_TRACED_REQUEST_ATTRS": "path_info,content_type,non_existing_variable", + }, + ) + self.env_patch.start() + self.exclude_patch = patch( + "opentelemetry.instrumentation.django.middleware._DjangoMiddleware._excluded_urls", + get_excluded_urls("DJANGO"), + ) + self.traced_patch = patch( + "opentelemetry.instrumentation.django.middleware._DjangoMiddleware._traced_request_attrs", + get_traced_request_attrs("DJANGO"), + ) + self.exclude_patch.start() + self.traced_patch.start() + + def tearDown(self): + super().tearDown() + self.env_patch.stop() + self.exclude_patch.stop() + self.traced_patch.stop() + teardown_test_environment() + _django_instrumentor.uninstrument() + + @classmethod + def tearDownClass(cls): + super().tearDownClass() + conf.settings = conf.LazySettings() + + @classmethod + def _add_databases_failures(cls): + # Disable databases. + pass + + @classmethod + def _remove_databases_failures(cls): + # Disable databases. + pass + + @pytest.mark.skip(reason="TODO") + async def test_templated_route_get(self): + await self.async_client.get("/route/2020/template/") + + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 1) + + span = spans[0] + + self.assertEqual(span.name, "^route/(?P[0-9]{4})/template/$") + self.assertEqual(span.kind, SpanKind.SERVER) + self.assertEqual(span.status.status_code, StatusCode.UNSET) + self.assertEqual(span.attributes["http.method"], "GET") + self.assertEqual( + span.attributes["http.url"], + "http://testserver/route/2020/template/", + ) + self.assertEqual( + span.attributes["http.route"], + "^route/(?P[0-9]{4})/template/$", + ) + self.assertEqual(span.attributes["http.scheme"], "http") + self.assertEqual(span.attributes["http.status_code"], 200) + self.assertEqual(span.attributes["http.status_text"], "OK") + + @pytest.mark.skip(reason="TODO") + async def test_traced_get(self): + await self.async_client.get("/traced/") + + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 1) + + span = spans[0] + + self.assertEqual(span.name, "^traced/") + self.assertEqual(span.kind, SpanKind.SERVER) + self.assertEqual(span.status.status_code, StatusCode.UNSET) + self.assertEqual(span.attributes["http.method"], "GET") + self.assertEqual( + span.attributes["http.url"], "http://testserver/traced/" + ) + self.assertEqual(span.attributes["http.route"], "^traced/") + self.assertEqual(span.attributes["http.scheme"], "http") + self.assertEqual(span.attributes["http.status_code"], 200) + self.assertEqual(span.attributes["http.status_text"], "OK") + + async def test_not_recording(self): + mock_tracer = Mock() + mock_span = Mock() + mock_span.is_recording.return_value = False + mock_tracer.start_span.return_value = mock_span + with patch("opentelemetry.trace.get_tracer") as tracer: + tracer.return_value = mock_tracer + await self.async_client.get("/traced/") + self.assertFalse(mock_span.is_recording()) + self.assertTrue(mock_span.is_recording.called) + self.assertFalse(mock_span.set_attribute.called) + self.assertFalse(mock_span.set_status.called) + + @pytest.mark.skip(reason="TODO") + async def test_traced_post(self): + await self.async_client.post("/traced/") + + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 1) + + span = spans[0] + + self.assertEqual(span.name, "^traced/") + self.assertEqual(span.kind, SpanKind.SERVER) + self.assertEqual(span.status.status_code, StatusCode.UNSET) + self.assertEqual(span.attributes["http.method"], "POST") + self.assertEqual( + span.attributes["http.url"], "http://testserver/traced/" + ) + self.assertEqual(span.attributes["http.route"], "^traced/") + self.assertEqual(span.attributes["http.scheme"], "http") + self.assertEqual(span.attributes["http.status_code"], 200) + self.assertEqual(span.attributes["http.status_text"], "OK") + + @pytest.mark.skip(reason="TODO") + async def test_error(self): + with self.assertRaises(ValueError): + await self.async_client.get("/error/") + + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 1) + + span = spans[0] + + self.assertEqual(span.name, "^error/") + self.assertEqual(span.kind, SpanKind.SERVER) + self.assertEqual(span.status.status_code, StatusCode.ERROR) + self.assertEqual(span.attributes["http.method"], "GET") + self.assertEqual( + span.attributes["http.url"], "http://testserver/error/" + ) + self.assertEqual(span.attributes["http.route"], "^error/") + self.assertEqual(span.attributes["http.scheme"], "http") + self.assertEqual(span.attributes["http.status_code"], 500) + + self.assertEqual(len(span.events), 1) + event = span.events[0] + self.assertEqual(event.name, "exception") + self.assertEqual(event.attributes["exception.type"], "ValueError") + self.assertEqual(event.attributes["exception.message"], "error") + + async def test_exclude_lists(self): + await self.async_client.get("/excluded_arg/123") + span_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(span_list), 0) + + await self.async_client.get("/excluded_arg/125") + span_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(span_list), 1) + + await self.async_client.get("/excluded_noarg/") + span_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(span_list), 1) + + await self.async_client.get("/excluded_noarg2/") + span_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(span_list), 1) + + async def test_span_name(self): + # test no query_string + await self.async_client.get("/span_name/1234/") + span_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(span_list), 1) + + span = span_list[0] + self.assertEqual(span.name, "^span_name/([0-9]{4})/$") + + async def test_span_name_for_query_string(self): + """ + request not have query string + """ + await self.async_client.get("/span_name/1234/?query=test") + span_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(span_list), 1) + + span = span_list[0] + self.assertEqual(span.name, "^span_name/([0-9]{4})/$") + + async def test_span_name_404(self): + await self.async_client.get("/span_name/1234567890/") + span_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(span_list), 1) + + span = span_list[0] + self.assertEqual(span.name, "HTTP GET") + + @pytest.mark.skip(reason="TODO") + async def test_traced_request_attrs(self): + await self.async_client.get("/span_name/1234/", CONTENT_TYPE="test/ct") + span_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(span_list), 1) + + span = span_list[0] + self.assertEqual(span.attributes["path_info"], "/span_name/1234/") + self.assertEqual(span.attributes["content_type"], "test/ct") + self.assertNotIn("non_existing_variable", span.attributes) diff --git a/instrumentation/opentelemetry-instrumentation-django/tests/views.py b/instrumentation/opentelemetry-instrumentation-django/tests/views.py index 872222a842..82220717ce 100644 --- a/instrumentation/opentelemetry-instrumentation-django/tests/views.py +++ b/instrumentation/opentelemetry-instrumentation-django/tests/views.py @@ -29,3 +29,33 @@ def route_span_name( request, *args, **kwargs ): # pylint: disable=unused-argument return HttpResponse() + + +async def async_traced(request): # pylint: disable=unused-argument + return HttpResponse() + + +async def async_traced_template(request, year): # pylint: disable=unused-argument + return HttpResponse() + + +async def async_error(request): # pylint: disable=unused-argument + raise ValueError("error") + + +async def async_excluded(request): # pylint: disable=unused-argument + return HttpResponse() + + +async def async_excluded_noarg(request): # pylint: disable=unused-argument + return HttpResponse() + + +async def async_excluded_noarg2(request): # pylint: disable=unused-argument + return HttpResponse() + + +async def async_route_span_name( + request, *args, **kwargs +): # pylint: disable=unused-argument + return HttpResponse() From 6aa5fa41f2bf33ff99e06723aa46f7e2d8e795d1 Mon Sep 17 00:00:00 2001 From: Michael Manganiello Date: Tue, 6 Apr 2021 13:31:10 -0300 Subject: [PATCH 3/8] Fix some of the pending unit tests --- CHANGELOG.md | 2 ++ .../instrumentation/asgi/__init__.py | 2 +- .../instrumentation/django/middleware.py | 3 +- .../tests/test_middleware_asgi.py | 30 ++++++++++--------- .../tests/views.py | 4 ++- 5 files changed, 24 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f76e9b1b3..89fd273fd9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -126,6 +126,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - `opentelemetry-instrumentation-httpx` Add `httpx` instrumentation ([#461](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/461)) +- `opentelemetry-instrumentation-django` Add ASGI support + ([#391](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/391)) ## [0.22b0](https://github.com/open-telemetry/opentelemetry-python/releases/tag/v1.3.0-0.22b0) - 2021-06-01 diff --git a/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py b/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py index 83bfc42f11..7bbf23e407 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py @@ -121,7 +121,7 @@ def get_host_port_url_tuple(scope): """Returns (host, port, full_url) tuple.""" server = scope.get("server") or ["0.0.0.0", 80] port = server[1] - server_host = server[0] + (":" + str(port) if port != 80 else "") + server_host = server[0] + (":" + str(port) if str(port) != "80" else "") full_path = scope.get("root_path", "") + scope.get("path", "") http_url = scope.get("scheme", "http") + "://" + server_host + full_path return server_host, port, http_url diff --git a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py index 5804aefbe8..e70d4c7447 100644 --- a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py +++ b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py @@ -79,6 +79,7 @@ def __call__(self, request): collect_request_attributes as asgi_collect_request_attributes, set_status_code, ) + _is_asgi_supported = True except ImportError: asgi_getter = None @@ -252,7 +253,7 @@ def process_response(self, request, response): if activation and span: if is_asgi_request: - set_status_code(request.META[self._environ_span_key], response.status_code) + set_status_code(span, response.status_code) else: add_response_attributes( span, diff --git a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py index 8a8ce8bd9c..0ccf895cbd 100644 --- a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py +++ b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py @@ -56,9 +56,10 @@ _django_instrumentor = DjangoInstrumentor() -@pytest.mark.skipif(not DJANGO_3_1, reason="AsyncClient implemented since Django 3.1") +@pytest.mark.skipif( + not DJANGO_3_1, reason="AsyncClient implemented since Django 3.1" +) class TestMiddlewareAsgi(SimpleTestCase, TestBase): - @classmethod def setUpClass(cls): super().setUpClass() @@ -110,7 +111,6 @@ def _remove_databases_failures(cls): # Disable databases. pass - @pytest.mark.skip(reason="TODO") async def test_templated_route_get(self): await self.async_client.get("/route/2020/template/") @@ -125,7 +125,7 @@ async def test_templated_route_get(self): self.assertEqual(span.attributes["http.method"], "GET") self.assertEqual( span.attributes["http.url"], - "http://testserver/route/2020/template/", + "http://127.0.0.1/route/2020/template/", ) self.assertEqual( span.attributes["http.route"], @@ -133,9 +133,9 @@ async def test_templated_route_get(self): ) self.assertEqual(span.attributes["http.scheme"], "http") self.assertEqual(span.attributes["http.status_code"], 200) - self.assertEqual(span.attributes["http.status_text"], "OK") + # TODO: Add http.status_text to ASGI instrumentation. + # self.assertEqual(span.attributes["http.status_text"], "OK") - @pytest.mark.skip(reason="TODO") async def test_traced_get(self): await self.async_client.get("/traced/") @@ -149,12 +149,13 @@ async def test_traced_get(self): self.assertEqual(span.status.status_code, StatusCode.UNSET) self.assertEqual(span.attributes["http.method"], "GET") self.assertEqual( - span.attributes["http.url"], "http://testserver/traced/" + span.attributes["http.url"], "http://127.0.0.1/traced/" ) self.assertEqual(span.attributes["http.route"], "^traced/") self.assertEqual(span.attributes["http.scheme"], "http") self.assertEqual(span.attributes["http.status_code"], 200) - self.assertEqual(span.attributes["http.status_text"], "OK") + # TODO: Add http.status_text to ASGI instrumentation. + # self.assertEqual(span.attributes["http.status_text"], "OK") async def test_not_recording(self): mock_tracer = Mock() @@ -169,7 +170,6 @@ async def test_not_recording(self): self.assertFalse(mock_span.set_attribute.called) self.assertFalse(mock_span.set_status.called) - @pytest.mark.skip(reason="TODO") async def test_traced_post(self): await self.async_client.post("/traced/") @@ -183,14 +183,14 @@ async def test_traced_post(self): self.assertEqual(span.status.status_code, StatusCode.UNSET) self.assertEqual(span.attributes["http.method"], "POST") self.assertEqual( - span.attributes["http.url"], "http://testserver/traced/" + span.attributes["http.url"], "http://127.0.0.1/traced/" ) self.assertEqual(span.attributes["http.route"], "^traced/") self.assertEqual(span.attributes["http.scheme"], "http") self.assertEqual(span.attributes["http.status_code"], 200) - self.assertEqual(span.attributes["http.status_text"], "OK") + # TODO: Add http.status_text to ASGI instrumentation. + # self.assertEqual(span.attributes["http.status_text"], "OK") - @pytest.mark.skip(reason="TODO") async def test_error(self): with self.assertRaises(ValueError): await self.async_client.get("/error/") @@ -205,7 +205,7 @@ async def test_error(self): self.assertEqual(span.status.status_code, StatusCode.ERROR) self.assertEqual(span.attributes["http.method"], "GET") self.assertEqual( - span.attributes["http.url"], "http://testserver/error/" + span.attributes["http.url"], "http://127.0.0.1/error/" ) self.assertEqual(span.attributes["http.route"], "^error/") self.assertEqual(span.attributes["http.scheme"], "http") @@ -262,7 +262,9 @@ async def test_span_name_404(self): span = span_list[0] self.assertEqual(span.name, "HTTP GET") - @pytest.mark.skip(reason="TODO") + @pytest.mark.skip( + reason="TODO: Traced request attributes not supported yet" + ) async def test_traced_request_attrs(self): await self.async_client.get("/span_name/1234/", CONTENT_TYPE="test/ct") span_list = self.memory_exporter.get_finished_spans() diff --git a/instrumentation/opentelemetry-instrumentation-django/tests/views.py b/instrumentation/opentelemetry-instrumentation-django/tests/views.py index 82220717ce..0bcc7e95be 100644 --- a/instrumentation/opentelemetry-instrumentation-django/tests/views.py +++ b/instrumentation/opentelemetry-instrumentation-django/tests/views.py @@ -35,7 +35,9 @@ async def async_traced(request): # pylint: disable=unused-argument return HttpResponse() -async def async_traced_template(request, year): # pylint: disable=unused-argument +async def async_traced_template( + request, year +): # pylint: disable=unused-argument return HttpResponse() From c101ea99b6d49bf30d3e8714c5904fcf49a20f78 Mon Sep 17 00:00:00 2001 From: Michael Manganiello Date: Sun, 30 May 2021 12:57:06 -0300 Subject: [PATCH 4/8] Add support for traced attributes 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. --- .../instrumentation/django/middleware.py | 24 ++- .../tests/test_middleware.py | 2 +- .../tests/test_middleware_asgi.py | 194 ++++++++++++++---- 3 files changed, 172 insertions(+), 48 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py index e70d4c7447..8300098478 100644 --- a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py +++ b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py @@ -14,6 +14,7 @@ from logging import getLogger from time import time +import types from typing import Callable from django import VERSION as django_version @@ -195,9 +196,26 @@ def process_request(self, request): attributes = collect_request_attributes(request_meta) if span.is_recording(): - attributes = extract_attributes_from_object( - request, self._traced_request_attrs, attributes - ) + if is_asgi_request: + # 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. + attributes = extract_attributes_from_object( + types.SimpleNamespace(**{ + **request.__dict__, + **{ + name.decode('latin1'): value.decode('latin1') + for name, value in request.scope.get('headers', []) + }, + }), + self._traced_request_attrs, + attributes, + ) + else: + attributes = extract_attributes_from_object( + request, self._traced_request_attrs, attributes + ) for key, value in attributes.items(): span.set_attribute(key, value) diff --git a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py index 1ccdcd7102..17896d6086 100644 --- a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py @@ -71,8 +71,8 @@ class TestMiddleware(TestBase, WsgiTestBase): @classmethod def setUpClass(cls): - super().setUpClass() conf.settings.configure(ROOT_URLCONF=modules[__name__]) + super().setUpClass() def setUp(self): super().setUp() diff --git a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py index 0ccf895cbd..dc9857a52a 100644 --- a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py +++ b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py @@ -16,14 +16,31 @@ from unittest.mock import Mock, patch from django import VERSION, conf +from django.conf.urls import url +from django.http import HttpRequest, HttpResponse from django.test import SimpleTestCase from django.test.utils import setup_test_environment, teardown_test_environment from django.urls import re_path import pytest -from opentelemetry.instrumentation.django import DjangoInstrumentor +from opentelemetry.instrumentation.django import ( + DjangoInstrumentor, + _DjangoMiddleware, +) +from opentelemetry.instrumentation.propagators import ( + TraceResponsePropagator, + set_global_response_propagator, +) +from opentelemetry.sdk import resources +from opentelemetry.sdk.trace import Span +from opentelemetry.semconv.trace import SpanAttributes from opentelemetry.test.test_base import TestBase -from opentelemetry.trace import SpanKind, StatusCode +from opentelemetry.trace import ( + SpanKind, + StatusCode, + format_span_id, + format_trace_id, +) from opentelemetry.util.http import get_excluded_urls, get_traced_request_attrs # pylint: disable=import-error @@ -62,8 +79,8 @@ class TestMiddlewareAsgi(SimpleTestCase, TestBase): @classmethod def setUpClass(cls): - super().setUpClass() conf.settings.configure(ROOT_URLCONF=modules[__name__]) + super().setUpClass() def setUp(self): super().setUp() @@ -101,16 +118,6 @@ def tearDownClass(cls): super().tearDownClass() conf.settings = conf.LazySettings() - @classmethod - def _add_databases_failures(cls): - # Disable databases. - pass - - @classmethod - def _remove_databases_failures(cls): - # Disable databases. - pass - async def test_templated_route_get(self): await self.async_client.get("/route/2020/template/") @@ -122,19 +129,17 @@ async def test_templated_route_get(self): self.assertEqual(span.name, "^route/(?P[0-9]{4})/template/$") self.assertEqual(span.kind, SpanKind.SERVER) self.assertEqual(span.status.status_code, StatusCode.UNSET) - self.assertEqual(span.attributes["http.method"], "GET") + self.assertEqual(span.attributes[SpanAttributes.HTTP_METHOD], "GET") self.assertEqual( - span.attributes["http.url"], + span.attributes[SpanAttributes.HTTP_URL], "http://127.0.0.1/route/2020/template/", ) self.assertEqual( - span.attributes["http.route"], + span.attributes[SpanAttributes.HTTP_ROUTE], "^route/(?P[0-9]{4})/template/$", ) - self.assertEqual(span.attributes["http.scheme"], "http") - self.assertEqual(span.attributes["http.status_code"], 200) - # TODO: Add http.status_text to ASGI instrumentation. - # self.assertEqual(span.attributes["http.status_text"], "OK") + self.assertEqual(span.attributes[SpanAttributes.HTTP_SCHEME], "http") + self.assertEqual(span.attributes[SpanAttributes.HTTP_STATUS_CODE], 200) async def test_traced_get(self): await self.async_client.get("/traced/") @@ -147,15 +152,16 @@ async def test_traced_get(self): self.assertEqual(span.name, "^traced/") self.assertEqual(span.kind, SpanKind.SERVER) self.assertEqual(span.status.status_code, StatusCode.UNSET) - self.assertEqual(span.attributes["http.method"], "GET") + self.assertEqual(span.attributes[SpanAttributes.HTTP_METHOD], "GET") + self.assertEqual( + span.attributes[SpanAttributes.HTTP_URL], + "http://127.0.0.1/traced/", + ) self.assertEqual( - span.attributes["http.url"], "http://127.0.0.1/traced/" + span.attributes[SpanAttributes.HTTP_ROUTE], "^traced/" ) - self.assertEqual(span.attributes["http.route"], "^traced/") - self.assertEqual(span.attributes["http.scheme"], "http") - self.assertEqual(span.attributes["http.status_code"], 200) - # TODO: Add http.status_text to ASGI instrumentation. - # self.assertEqual(span.attributes["http.status_text"], "OK") + self.assertEqual(span.attributes[SpanAttributes.HTTP_SCHEME], "http") + self.assertEqual(span.attributes[SpanAttributes.HTTP_STATUS_CODE], 200) async def test_not_recording(self): mock_tracer = Mock() @@ -181,15 +187,16 @@ async def test_traced_post(self): self.assertEqual(span.name, "^traced/") self.assertEqual(span.kind, SpanKind.SERVER) self.assertEqual(span.status.status_code, StatusCode.UNSET) - self.assertEqual(span.attributes["http.method"], "POST") + self.assertEqual(span.attributes[SpanAttributes.HTTP_METHOD], "POST") + self.assertEqual( + span.attributes[SpanAttributes.HTTP_URL], + "http://127.0.0.1/traced/", + ) self.assertEqual( - span.attributes["http.url"], "http://127.0.0.1/traced/" + span.attributes[SpanAttributes.HTTP_ROUTE], "^traced/" ) - self.assertEqual(span.attributes["http.route"], "^traced/") - self.assertEqual(span.attributes["http.scheme"], "http") - self.assertEqual(span.attributes["http.status_code"], 200) - # TODO: Add http.status_text to ASGI instrumentation. - # self.assertEqual(span.attributes["http.status_text"], "OK") + self.assertEqual(span.attributes[SpanAttributes.HTTP_SCHEME], "http") + self.assertEqual(span.attributes[SpanAttributes.HTTP_STATUS_CODE], 200) async def test_error(self): with self.assertRaises(ValueError): @@ -203,19 +210,24 @@ async def test_error(self): self.assertEqual(span.name, "^error/") self.assertEqual(span.kind, SpanKind.SERVER) self.assertEqual(span.status.status_code, StatusCode.ERROR) - self.assertEqual(span.attributes["http.method"], "GET") + self.assertEqual(span.attributes[SpanAttributes.HTTP_METHOD], "GET") self.assertEqual( - span.attributes["http.url"], "http://127.0.0.1/error/" + span.attributes[SpanAttributes.HTTP_URL], + "http://127.0.0.1/error/", ) - self.assertEqual(span.attributes["http.route"], "^error/") - self.assertEqual(span.attributes["http.scheme"], "http") - self.assertEqual(span.attributes["http.status_code"], 500) + self.assertEqual(span.attributes[SpanAttributes.HTTP_ROUTE], "^error/") + self.assertEqual(span.attributes[SpanAttributes.HTTP_SCHEME], "http") + self.assertEqual(span.attributes[SpanAttributes.HTTP_STATUS_CODE], 500) self.assertEqual(len(span.events), 1) event = span.events[0] self.assertEqual(event.name, "exception") - self.assertEqual(event.attributes["exception.type"], "ValueError") - self.assertEqual(event.attributes["exception.message"], "error") + self.assertEqual( + event.attributes[SpanAttributes.EXCEPTION_TYPE], "ValueError" + ) + self.assertEqual( + event.attributes[SpanAttributes.EXCEPTION_MESSAGE], "error" + ) async def test_exclude_lists(self): await self.async_client.get("/excluded_arg/123") @@ -262,9 +274,6 @@ async def test_span_name_404(self): span = span_list[0] self.assertEqual(span.name, "HTTP GET") - @pytest.mark.skip( - reason="TODO: Traced request attributes not supported yet" - ) async def test_traced_request_attrs(self): await self.async_client.get("/span_name/1234/", CONTENT_TYPE="test/ct") span_list = self.memory_exporter.get_finished_spans() @@ -274,3 +283,100 @@ async def test_traced_request_attrs(self): self.assertEqual(span.attributes["path_info"], "/span_name/1234/") self.assertEqual(span.attributes["content_type"], "test/ct") self.assertNotIn("non_existing_variable", span.attributes) + + async def test_hooks(self): + request_hook_args = () + response_hook_args = () + + def request_hook(span, request): + nonlocal request_hook_args + request_hook_args = (span, request) + + def response_hook(span, request, response): + nonlocal response_hook_args + response_hook_args = (span, request, response) + response["hook-header"] = "set by hook" + + _DjangoMiddleware._otel_request_hook = request_hook + _DjangoMiddleware._otel_response_hook = response_hook + + response = await self.async_client.get("/span_name/1234/") + _DjangoMiddleware._otel_request_hook = ( + _DjangoMiddleware._otel_response_hook + ) = None + + self.assertEqual(response["hook-header"], "set by hook") + + span_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(span_list), 1) + span = span_list[0] + self.assertEqual(span.attributes["path_info"], "/span_name/1234/") + + self.assertEqual(len(request_hook_args), 2) + self.assertEqual(request_hook_args[0].name, span.name) + self.assertIsInstance(request_hook_args[0], Span) + self.assertIsInstance(request_hook_args[1], HttpRequest) + + self.assertEqual(len(response_hook_args), 3) + self.assertEqual(request_hook_args[0], response_hook_args[0]) + self.assertIsInstance(response_hook_args[1], HttpRequest) + self.assertIsInstance(response_hook_args[2], HttpResponse) + self.assertEqual(response_hook_args[2], response) + + async def test_trace_response_headers(self): + response = await self.async_client.get("/span_name/1234/") + + self.assertNotIn("Server-Timing", response.headers) + self.memory_exporter.clear() + + set_global_response_propagator(TraceResponsePropagator()) + + response = await self.async_client.get("/span_name/1234/") + span = self.memory_exporter.get_finished_spans()[0] + + self.assertIn("traceresponse", response.headers) + self.assertEqual( + response.headers["Access-Control-Expose-Headers"], "traceresponse", + ) + self.assertEqual( + response.headers["traceresponse"], + "00-{0}-{1}-01".format( + format_trace_id(span.get_span_context().trace_id), + format_span_id(span.get_span_context().span_id), + ), + ) + self.memory_exporter.clear() + + +class TestMiddlewareAsgiWithTracerProvider(SimpleTestCase, TestBase): + @classmethod + def setUpClass(cls): + super().setUpClass() + + def setUp(self): + super().setUp() + setup_test_environment() + resource = resources.Resource.create( + {"resource-key": "resource-value"} + ) + result = self.create_tracer_provider(resource=resource) + tracer_provider, exporter = result + self.exporter = exporter + _django_instrumentor.instrument(tracer_provider=tracer_provider) + + def tearDown(self): + super().tearDown() + teardown_test_environment() + _django_instrumentor.uninstrument() + + async def test_tracer_provider_traced(self): + await self.async_client.post("/traced/") + + spans = self.exporter.get_finished_spans() + self.assertEqual(len(spans), 1) + + span = spans[0] + + self.assertEqual( + span.resource.attributes["resource-key"], "resource-value" + ) From 5b523af4876971b569a3083b9c004212d8bbcd55 Mon Sep 17 00:00:00 2001 From: Michael Manganiello Date: Sun, 30 May 2021 13:33:57 -0300 Subject: [PATCH 5/8] Fix linting issues --- .../setup.cfg | 2 +- .../instrumentation/django/middleware.py | 39 +++++++++++-------- .../tests/test_middleware.py | 7 +++- .../tests/test_middleware_asgi.py | 14 +++---- 4 files changed, 36 insertions(+), 26 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-django/setup.cfg b/instrumentation/opentelemetry-instrumentation-django/setup.cfg index d9ba8f7657..ccec0e068e 100644 --- a/instrumentation/opentelemetry-instrumentation-django/setup.cfg +++ b/instrumentation/opentelemetry-instrumentation-django/setup.cfg @@ -46,7 +46,7 @@ install_requires = [options.extras_require] asgi = - opentelemetry-instrumentation-asgi == 0.22.dev0 + opentelemetry-instrumentation-asgi == 0.23.dev0 test = opentelemetry-test == 0.24b0 diff --git a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py index 8300098478..131e1de46e 100644 --- a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py +++ b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py @@ -12,9 +12,9 @@ # See the License for the specific language governing permissions and # limitations under the License. +import types from logging import getLogger from time import time -import types from typing import Callable from django import VERSION as django_version @@ -25,11 +25,11 @@ get_global_response_propagator, ) from opentelemetry.instrumentation.utils import extract_attributes_from_object +from opentelemetry.instrumentation.wsgi import add_response_attributes from opentelemetry.instrumentation.wsgi import ( - add_response_attributes, collect_request_attributes as wsgi_collect_request_attributes, - wsgi_getter, ) +from opentelemetry.instrumentation.wsgi import wsgi_getter from opentelemetry.propagate import extract from opentelemetry.semconv.trace import SpanAttributes from opentelemetry.trace import Span, SpanKind, use_span @@ -75,11 +75,11 @@ def __call__(self, request): ASGIRequest = None try: + from opentelemetry.instrumentation.asgi import asgi_getter from opentelemetry.instrumentation.asgi import ( - asgi_getter, collect_request_attributes as asgi_collect_request_attributes, - set_status_code, ) + from opentelemetry.instrumentation.asgi import set_status_code _is_asgi_supported = True except ImportError: @@ -112,6 +112,10 @@ def __call__(self, request): ] +def _is_asgi_request(request: HttpRequest) -> bool: + return bool(ASGIRequest and isinstance(request, ASGIRequest)) + + class _DjangoMiddleware(MiddlewareMixin): """Django Middleware for OpenTelemetry""" @@ -153,9 +157,6 @@ def _get_span_name(request): except Resolver404: return f"HTTP {request.method}" - def _is_asgi_request(self, request): - return ASGIRequest and isinstance(request, ASGIRequest) - def process_request(self, request): # request.META is a dictionary containing all available HTTP headers # Read more about request.META here: @@ -164,7 +165,7 @@ def process_request(self, request): if self._excluded_urls.url_disabled(request.build_absolute_uri("?")): return - is_asgi_request = self._is_asgi_request(request) + is_asgi_request = _is_asgi_request(request) if is_asgi_request and not _is_asgi_supported: return @@ -202,13 +203,17 @@ def process_request(self, request): # contents, for the extract_attributes_from_object function to be able to retrieve # attributes from it. attributes = extract_attributes_from_object( - types.SimpleNamespace(**{ - **request.__dict__, + types.SimpleNamespace( **{ - name.decode('latin1'): value.decode('latin1') - for name, value in request.scope.get('headers', []) - }, - }), + **request.__dict__, + **{ + name.decode("latin1"): value.decode("latin1") + for name, value in request.scope.get( + "headers", [] + ) + }, + } + ), self._traced_request_attrs, attributes, ) @@ -262,9 +267,9 @@ def process_response(self, request, response): if self._excluded_urls.url_disabled(request.build_absolute_uri("?")): return response - is_asgi_request = self._is_asgi_request(request) + is_asgi_request = _is_asgi_request(request) if is_asgi_request and not _is_asgi_supported: - return + return response activation = request.META.pop(self._environ_activation_key, None) span = request.META.pop(self._environ_span_key, None) diff --git a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py index 17896d6086..c9142472ce 100644 --- a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py @@ -16,7 +16,6 @@ from unittest.mock import Mock, patch from django import VERSION, conf -from django.conf.urls import url from django.http import HttpRequest, HttpResponse from django.test.client import Client from django.test.utils import setup_test_environment, teardown_test_environment @@ -362,6 +361,7 @@ def test_trace_response_headers(self): class TestMiddlewareWithTracerProvider(TestBase, WsgiTestBase): @classmethod def setUpClass(cls): + conf.settings.configure(ROOT_URLCONF=modules[__name__]) super().setUpClass() def setUp(self): @@ -380,6 +380,11 @@ def tearDown(self): teardown_test_environment() _django_instrumentor.uninstrument() + @classmethod + def tearDownClass(cls): + super().tearDownClass() + conf.settings = conf.LazySettings() + def test_tracer_provider_traced(self): Client().post("/traced/") diff --git a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py index dc9857a52a..36a45c5fc1 100644 --- a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py +++ b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py @@ -15,13 +15,12 @@ from sys import modules from unittest.mock import Mock, patch +import pytest from django import VERSION, conf -from django.conf.urls import url from django.http import HttpRequest, HttpResponse from django.test import SimpleTestCase from django.test.utils import setup_test_environment, teardown_test_environment from django.urls import re_path -import pytest from opentelemetry.instrumentation.django import ( DjangoInstrumentor, @@ -56,11 +55,6 @@ DJANGO_3_1 = VERSION >= (3, 1) -if DJANGO_3_1: - from django.test.client import AsyncClient -else: - AsyncClient = None - urlpatterns = [ re_path(r"^traced/", async_traced), re_path(r"^route/(?P[0-9]{4})/template/$", async_traced_template), @@ -351,6 +345,7 @@ async def test_trace_response_headers(self): class TestMiddlewareAsgiWithTracerProvider(SimpleTestCase, TestBase): @classmethod def setUpClass(cls): + conf.settings.configure(ROOT_URLCONF=modules[__name__]) super().setUpClass() def setUp(self): @@ -369,6 +364,11 @@ def tearDown(self): teardown_test_environment() _django_instrumentor.uninstrument() + @classmethod + def tearDownClass(cls): + super().tearDownClass() + conf.settings = conf.LazySettings() + async def test_tracer_provider_traced(self): await self.async_client.post("/traced/") From e5a2949414667ccdc7612b36d1e2f6b88d6a08a4 Mon Sep 17 00:00:00 2001 From: Michael Manganiello Date: Thu, 8 Jul 2021 23:08:49 -0300 Subject: [PATCH 6/8] Tackle PR comments --- .../opentelemetry/instrumentation/django/middleware.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py index 131e1de46e..8bb7223fb2 100644 --- a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py +++ b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py @@ -44,6 +44,7 @@ from django.urls import Resolver404, resolve DJANGO_2_0 = django_version >= (2, 0) +DJANGO_3_0 = django_version >= (3, 0) if DJANGO_2_0: # Since Django 2.0, only `settings.MIDDLEWARE` is supported, so new-style @@ -68,10 +69,9 @@ def __call__(self, request): except ImportError: MiddlewareMixin = object - -try: +if DJANGO_3_0: from django.core.handlers.asgi import ASGIRequest -except ImportError: +else: ASGIRequest = None try: @@ -113,7 +113,7 @@ def __call__(self, request): def _is_asgi_request(request: HttpRequest) -> bool: - return bool(ASGIRequest and isinstance(request, ASGIRequest)) + return ASGIRequest is not None and isinstance(request, ASGIRequest) class _DjangoMiddleware(MiddlewareMixin): From 20d19a8c5543afe38f2e50ad202752a8cf3bf773 Mon Sep 17 00:00:00 2001 From: Michael Manganiello Date: Tue, 21 Sep 2021 16:04:59 -0300 Subject: [PATCH 7/8] Rebase and apply @owais comments --- CHANGELOG.md | 10 +++--- .../setup.cfg | 2 +- .../instrumentation/django/middleware.py | 34 +++++++------------ 3 files changed, 19 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 89fd273fd9..abdc43bfbf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#706](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/706)) - `opentelemetry-instrumentation-requests` added exclude urls functionality ([#714](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/714)) +- `opentelemetry-instrumentation-django` Add ASGI support + ([#391](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/391)) ### Changed - `opentelemetry-instrumentation-botocore` Make common span attributes compliant with semantic conventions @@ -57,12 +59,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - `opentelemetry-sdk-extension-aws` Add AWS resource detectors to extension package ([#586](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/586)) -- `opentelemetry-instrumentation-asgi`, `opentelemetry-instrumentation-aiohttp-client`, `openetelemetry-instrumentation-fastapi`, - `opentelemetry-instrumentation-starlette`, `opentelemetry-instrumentation-urllib`, `opentelemetry-instrumentation-urllib3` Added `request_hook` and `response_hook` callbacks +- `opentelemetry-instrumentation-asgi`, `opentelemetry-instrumentation-aiohttp-client`, `openetelemetry-instrumentation-fastapi`, + `opentelemetry-instrumentation-starlette`, `opentelemetry-instrumentation-urllib`, `opentelemetry-instrumentation-urllib3` Added `request_hook` and `response_hook` callbacks ([#576](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/576)) - `opentelemetry-instrumentation-pika` added RabbitMQ's pika module instrumentation. ([#680](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/680)) - + ### Changed - `opentelemetry-instrumentation-fastapi` Allow instrumentation of newer FastAPI versions. @@ -126,8 +128,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - `opentelemetry-instrumentation-httpx` Add `httpx` instrumentation ([#461](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/461)) -- `opentelemetry-instrumentation-django` Add ASGI support - ([#391](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/391)) ## [0.22b0](https://github.com/open-telemetry/opentelemetry-python/releases/tag/v1.3.0-0.22b0) - 2021-06-01 diff --git a/instrumentation/opentelemetry-instrumentation-django/setup.cfg b/instrumentation/opentelemetry-instrumentation-django/setup.cfg index ccec0e068e..4ce8df1a33 100644 --- a/instrumentation/opentelemetry-instrumentation-django/setup.cfg +++ b/instrumentation/opentelemetry-instrumentation-django/setup.cfg @@ -46,7 +46,7 @@ install_requires = [options.extras_require] asgi = - opentelemetry-instrumentation-asgi == 0.23.dev0 + opentelemetry-instrumentation-asgi == 0.24b0 test = opentelemetry-test == 0.24b0 diff --git a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py index 8bb7223fb2..91304d873b 100644 --- a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py +++ b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py @@ -74,6 +74,7 @@ def __call__(self, request): else: ASGIRequest = None +# try/except block exclusive for optional ASGI imports. try: from opentelemetry.instrumentation.asgi import asgi_getter from opentelemetry.instrumentation.asgi import ( @@ -166,7 +167,7 @@ def process_request(self, request): return is_asgi_request = _is_asgi_request(request) - if is_asgi_request and not _is_asgi_supported: + if not _is_asgi_supported and is_asgi_request: return # pylint:disable=W0212 @@ -175,9 +176,11 @@ def process_request(self, request): request_meta = request.META if is_asgi_request: + carrier = request.scope carrier_getter = asgi_getter collect_request_attributes = asgi_collect_request_attributes else: + carrier = request_meta carrier_getter = wsgi_getter collect_request_attributes = wsgi_collect_request_attributes @@ -191,36 +194,25 @@ def process_request(self, request): ), ) - if is_asgi_request: - attributes = collect_request_attributes(request.scope) - else: - attributes = collect_request_attributes(request_meta) + attributes = collect_request_attributes(carrier) if span.is_recording(): + attributes = extract_attributes_from_object( + request, self._traced_request_attrs, attributes + ) if is_asgi_request: - # 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. + # ASGI requests include extra attributes in request.scope.headers. attributes = extract_attributes_from_object( types.SimpleNamespace( **{ - **request.__dict__, - **{ - name.decode("latin1"): value.decode("latin1") - for name, value in request.scope.get( - "headers", [] - ) - }, + name.decode("latin1"): value.decode("latin1") + for name, value in request.scope.get("headers", []) } ), self._traced_request_attrs, attributes, ) - else: - attributes = extract_attributes_from_object( - request, self._traced_request_attrs, attributes - ) + for key, value in attributes.items(): span.set_attribute(key, value) @@ -268,7 +260,7 @@ def process_response(self, request, response): return response is_asgi_request = _is_asgi_request(request) - if is_asgi_request and not _is_asgi_supported: + if not _is_asgi_supported and is_asgi_request: return response activation = request.META.pop(self._environ_activation_key, None) From 7b82acbfede43fd41f3ef818e4270c528a66ca5e Mon Sep 17 00:00:00 2001 From: Michael Manganiello Date: Mon, 27 Sep 2021 23:37:56 -0300 Subject: [PATCH 8/8] Always detach token from request.META --- .../src/opentelemetry/instrumentation/django/middleware.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py index 91304d873b..114f6f4a51 100644 --- a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py +++ b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py @@ -297,10 +297,7 @@ def process_response(self, request, response): activation.__exit__(None, None, None) if self._environ_token in request.META.keys(): - if is_asgi_request: - detach(request.META.get(self._environ_token)) - else: - detach(request.environ.get(self._environ_token)) + detach(request.META.get(self._environ_token)) request.META.pop(self._environ_token) return response