From f29172cfc84da4d9ea378e47cf393e6674a592d4 Mon Sep 17 00:00:00 2001 From: Michael Manganiello Date: Mon, 25 Oct 2021 21:15:16 -0300 Subject: [PATCH 1/6] `django`: Fix instrumentation and tests for all Django major versions * Fix support for Django `1.10`/`1.11`, by allowing `settings.MIDDLEWARE`. * Add Tox environments for every Django major version, including 4.0 beta. * Fix tests for new Tox environments to pass. Relevant context: * In Django 1.x, usage of `settings.MIDDLEWARE_CLASSES` is detected by running `settings.MIDDLEWARE is None`: https://github.com/django/django/blob/58f02c498b659a906e9c30d946bd89bedc4717e5/django/core/handlers/base.py#L48 * HTTP route in `ResolverMatch` object is only available since Django 2.2: https://github.com/django/django/pull/10657 * Django class `HttpResponseBase` works as a mapping object, so avoiding accessing its internal headers object simplifies support accross Django versions. --- .../instrumentation/django/__init__.py | 16 +++++-- .../tests/test_middleware.py | 42 ++++++++++++------- .../tests/test_middleware_asgi.py | 15 ++++--- tox.ini | 18 ++++---- 4 files changed, 61 insertions(+), 30 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/__init__.py b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/__init__.py index 344fbb261c..a33197bae6 100644 --- a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/__init__.py @@ -83,6 +83,7 @@ def response_hook(span, request, response): from os import environ from typing import Collection +from django import VERSION as django_version from django.conf import settings from opentelemetry.instrumentation.django.environment_variables import ( @@ -94,6 +95,8 @@ def response_hook(span, request, response): from opentelemetry.instrumentation.instrumentor import BaseInstrumentor from opentelemetry.trace import get_tracer +DJANGO_2_0 = django_version >= (2, 0) + _logger = getLogger(__name__) @@ -136,16 +139,23 @@ def _instrument(self, **kwargs): # https://docs.djangoproject.com/en/3.0/ref/middleware/#middleware-ordering settings_middleware = getattr(settings, "MIDDLEWARE", []) + self._middleware_setting = "MIDDLEWARE" + # In Django versions 1.x, setting MIDDLEWARE_CLASSES can be used as a legacy + # alternative to MIDDLEWARE. + if settings_middleware is None and not DJANGO_2_0: + settings_middleware = getattr(settings, "MIDDLEWARE_CLASSES", []) + self._middleware_setting = "MIDDLEWARE_CLASSES" + # Django allows to specify middlewares as a tuple, so we convert this tuple to a # list, otherwise we wouldn't be able to call append/remove if isinstance(settings_middleware, tuple): settings_middleware = list(settings_middleware) settings_middleware.insert(0, self._opentelemetry_middleware) - setattr(settings, "MIDDLEWARE", settings_middleware) + setattr(settings, self._middleware_setting, settings_middleware) def _uninstrument(self, **kwargs): - settings_middleware = getattr(settings, "MIDDLEWARE", None) + settings_middleware = getattr(settings, self._middleware_setting, None) # FIXME This is starting to smell like trouble. We have 2 mechanisms # that may make this condition be True, one implemented in @@ -158,4 +168,4 @@ def _uninstrument(self, **kwargs): return settings_middleware.remove(self._opentelemetry_middleware) - setattr(settings, "MIDDLEWARE", settings_middleware) + setattr(settings, self._middleware_setting, settings_middleware) diff --git a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py index 0b90a9bb7c..b361d78b44 100644 --- a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py @@ -19,7 +19,6 @@ from django.http import HttpRequest, HttpResponse 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, @@ -54,7 +53,14 @@ traced_template, ) +DJANGO_2_0 = VERSION >= (2, 0) DJANGO_2_2 = VERSION >= (2, 2) +DJANGO_3_0 = VERSION >= (3, 0) + +if DJANGO_2_0: + from django.urls import re_path +else: + from django.conf.urls import url as re_path urlpatterns = [ re_path(r"^traced/", traced), @@ -122,7 +128,7 @@ def test_templated_route_get(self): span.name, "^route/(?P[0-9]{4})/template/$" if DJANGO_2_2 - else "tests.views.traced", + else "tests.views.traced_template", ) self.assertEqual(span.kind, SpanKind.SERVER) self.assertEqual(span.status.status_code, StatusCode.UNSET) @@ -131,10 +137,11 @@ def test_templated_route_get(self): span.attributes[SpanAttributes.HTTP_URL], "http://testserver/route/2020/template/", ) - self.assertEqual( - span.attributes[SpanAttributes.HTTP_ROUTE], - "^route/(?P[0-9]{4})/template/$", - ) + if DJANGO_2_2: + self.assertEqual( + span.attributes[SpanAttributes.HTTP_ROUTE], + "^route/(?P[0-9]{4})/template/$", + ) self.assertEqual(span.attributes[SpanAttributes.HTTP_SCHEME], "http") self.assertEqual(span.attributes[SpanAttributes.HTTP_STATUS_CODE], 200) @@ -156,9 +163,10 @@ def test_traced_get(self): span.attributes[SpanAttributes.HTTP_URL], "http://testserver/traced/", ) - self.assertEqual( - span.attributes[SpanAttributes.HTTP_ROUTE], "^traced/" - ) + if DJANGO_2_2: + self.assertEqual( + span.attributes[SpanAttributes.HTTP_ROUTE], "^traced/" + ) self.assertEqual(span.attributes[SpanAttributes.HTTP_SCHEME], "http") self.assertEqual(span.attributes[SpanAttributes.HTTP_STATUS_CODE], 200) @@ -193,9 +201,10 @@ def test_traced_post(self): span.attributes[SpanAttributes.HTTP_URL], "http://testserver/traced/", ) - self.assertEqual( - span.attributes[SpanAttributes.HTTP_ROUTE], "^traced/" - ) + if DJANGO_2_2: + self.assertEqual( + span.attributes[SpanAttributes.HTTP_ROUTE], "^traced/" + ) self.assertEqual(span.attributes[SpanAttributes.HTTP_SCHEME], "http") self.assertEqual(span.attributes[SpanAttributes.HTTP_STATUS_CODE], 200) @@ -218,7 +227,10 @@ def test_error(self): span.attributes[SpanAttributes.HTTP_URL], "http://testserver/error/", ) - self.assertEqual(span.attributes[SpanAttributes.HTTP_ROUTE], "^error/") + if DJANGO_2_2: + 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) @@ -363,14 +375,14 @@ async def test_trace_parent(self): def test_trace_response_headers(self): response = Client().get("/span_name/1234/") - self.assertNotIn("Server-Timing", response.headers) + self.assertFalse(response.has_header("Server-Timing")) self.memory_exporter.clear() set_global_response_propagator(TraceResponsePropagator()) response = Client().get("/span_name/1234/") self.assertTraceResponseHeaderMatchesSpan( - response.headers, self.memory_exporter.get_finished_spans()[0] + response, self.memory_exporter.get_finished_spans()[0], ) self.memory_exporter.clear() diff --git a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py index 0bac1c7243..a82d167a5a 100644 --- a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py +++ b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py @@ -20,7 +20,6 @@ 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 from opentelemetry.instrumentation.django import ( DjangoInstrumentor, @@ -54,8 +53,14 @@ async_traced_template, ) +DJANGO_2_0 = VERSION >= (2, 0) DJANGO_3_1 = VERSION >= (3, 1) +if DJANGO_2_0: + from django.urls import re_path +else: + from django.conf.urls import url as re_path + urlpatterns = [ re_path(r"^traced/", async_traced), re_path(r"^route/(?P[0-9]{4})/template/$", async_traced_template), @@ -344,7 +349,7 @@ async def test_trace_parent(self): async def test_trace_response_headers(self): response = await self.async_client.get("/span_name/1234/") - self.assertNotIn("Server-Timing", response.headers) + self.assertFalse(response.has_header("Server-Timing")) self.memory_exporter.clear() set_global_response_propagator(TraceResponsePropagator()) @@ -352,12 +357,12 @@ async def test_trace_response_headers(self): response = await self.async_client.get("/span_name/1234/") span = self.memory_exporter.get_finished_spans()[0] - self.assertIn("traceresponse", response.headers) + self.assertTrue(response.has_header("traceresponse")) self.assertEqual( - response.headers["Access-Control-Expose-Headers"], "traceresponse", + response["Access-Control-Expose-Headers"], "traceresponse", ) self.assertEqual( - response.headers["traceresponse"], + response["traceresponse"], "00-{}-{}-01".format( format_trace_id(span.get_span_context().trace_id), format_span_id(span.get_span_context().span_id), diff --git a/tox.ini b/tox.ini index aa169d1f7c..22d828d58f 100644 --- a/tox.ini +++ b/tox.ini @@ -30,8 +30,8 @@ envlist = pypy3-test-instrumentation-botocore ; opentelemetry-instrumentation-django - py3{6,7,8,9,10}-test-instrumentation-django - pypy3-test-instrumentation-django + py3{6,7,8,9,10}-test-instrumentation-django{1,2,3,4} + pypy3-test-instrumentation-django{1,2,3,4} ; opentelemetry-instrumentation-dbapi py3{6,7,8,9,10}-test-instrumentation-dbapi @@ -186,6 +186,10 @@ deps = test: pytest-benchmark coverage: pytest coverage: pytest-cov + django1: django~=1.0 + django2: django~=2.0 + django3: django~=3.0 + django4: django>=4.0b1,<5.0 elasticsearch2: elasticsearch-dsl>=2.0,<3.0 elasticsearch2: elasticsearch>=2.0,<3.0 elasticsearch5: elasticsearch-dsl>=5.0,<6.0 @@ -221,7 +225,7 @@ changedir = test-instrumentation-botocore: instrumentation/opentelemetry-instrumentation-botocore/tests test-instrumentation-celery: instrumentation/opentelemetry-instrumentation-celery/tests test-instrumentation-dbapi: instrumentation/opentelemetry-instrumentation-dbapi/tests - test-instrumentation-django: instrumentation/opentelemetry-instrumentation-django/tests + test-instrumentation-django{1,2,3,4}: instrumentation/opentelemetry-instrumentation-django/tests test-instrumentation-elasticsearch{2,5,6}: instrumentation/opentelemetry-instrumentation-elasticsearch/tests test-instrumentation-falcon{2,3}: instrumentation/opentelemetry-instrumentation-falcon/tests test-instrumentation-fastapi: instrumentation/opentelemetry-instrumentation-fastapi/tests @@ -272,9 +276,9 @@ commands_pre = grpc: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-grpc[test] - 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,django,starlette,fastapi: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-asgi[test] + falcon{2,3},flask,django{1,2,3,4},pyramid,tornado,starlette,fastapi,aiohttp,asgi,requests,urllib,urllib3,wsgi: pip install {toxinidir}/util/opentelemetry-util-http[test] + wsgi,falcon{2,3},flask,django{1,2,3,4},pyramid: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-wsgi[test] + asgi,django{3,4},starlette,fastapi: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-asgi[test] asyncpg: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-asyncpg[test] @@ -293,7 +297,7 @@ commands_pre = dbapi: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-dbapi[test] - django: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-django[test] + django{1,2,3,4}: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-django[test] fastapi: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-fastapi[test] From bb2b2b28583a1dfd4bad4b59f441c12179ba33fa Mon Sep 17 00:00:00 2001 From: Michael Manganiello Date: Mon, 25 Oct 2021 21:33:09 -0300 Subject: [PATCH 2/6] Add changelog entry --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d7fa3863b7..ef66ca03a2 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 ([#767](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/767)) - Don't set Span Status on 4xx http status code for SpanKind.SERVER spans ([#776](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/776)) +- `opentelemetry-instrumentation-django` Fixed instrumentation and tests for all Django major versions. + ([#780](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/780)) ## [1.6.2-0.25b2](https://github.com/open-telemetry/opentelemetry-python/releases/tag/v1.6.2-0.25b2) - 2021-10-19 @@ -41,7 +43,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `opentelemetry-distro` uses the correct entrypoint name which was updated in the core release of 1.6.0 but the distro was not updated with it ([#755](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/755)) - + ### Added - `opentelemetry-instrumentation-pika` Add `publish_hook` and `consume_hook` callbacks passed as arguments to the instrument method ([#763](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/763)) From 9532d8d155286635843b28107c46668f94571c0f Mon Sep 17 00:00:00 2001 From: Michael Manganiello Date: Tue, 26 Oct 2021 01:11:41 -0300 Subject: [PATCH 3/6] Only test Django major releases on officially supported Python versions --- tox.ini | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tox.ini b/tox.ini index 22d828d58f..b6b1b9c6c5 100644 --- a/tox.ini +++ b/tox.ini @@ -30,8 +30,11 @@ envlist = pypy3-test-instrumentation-botocore ; opentelemetry-instrumentation-django - py3{6,7,8,9,10}-test-instrumentation-django{1,2,3,4} - pypy3-test-instrumentation-django{1,2,3,4} + py3{6,7}-test-instrumentation-django1 + py3{6,7,8,9}-test-instrumentation-django2 + py3{6,7,8,9,10}-test-instrumentation-django3 + py3{8,9,10}-test-instrumentation-django4 + pypy3-test-instrumentation-django{1,2,3} ; opentelemetry-instrumentation-dbapi py3{6,7,8,9,10}-test-instrumentation-dbapi From 60ee7c316cb83e1c21aa76b8e1cc847232777b01 Mon Sep 17 00:00:00 2001 From: Michael Manganiello Date: Tue, 26 Oct 2021 01:36:40 -0300 Subject: [PATCH 4/6] Reduce test environments --- .../instrumentation/django/__init__.py | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/__init__.py b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/__init__.py index a33197bae6..ae41dda840 100644 --- a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/__init__.py @@ -138,13 +138,8 @@ def _instrument(self, **kwargs): # https://docs.djangoproject.com/en/3.0/topics/http/middleware/#activating-middleware # https://docs.djangoproject.com/en/3.0/ref/middleware/#middleware-ordering - settings_middleware = getattr(settings, "MIDDLEWARE", []) - self._middleware_setting = "MIDDLEWARE" - # In Django versions 1.x, setting MIDDLEWARE_CLASSES can be used as a legacy - # alternative to MIDDLEWARE. - if settings_middleware is None and not DJANGO_2_0: - settings_middleware = getattr(settings, "MIDDLEWARE_CLASSES", []) - self._middleware_setting = "MIDDLEWARE_CLASSES" + _middleware_setting = self._get_middleware_setting() + settings_middleware = getattr(settings, _middleware_setting, []) # Django allows to specify middlewares as a tuple, so we convert this tuple to a # list, otherwise we wouldn't be able to call append/remove @@ -152,10 +147,11 @@ def _instrument(self, **kwargs): settings_middleware = list(settings_middleware) settings_middleware.insert(0, self._opentelemetry_middleware) - setattr(settings, self._middleware_setting, settings_middleware) + setattr(settings, _middleware_setting, settings_middleware) def _uninstrument(self, **kwargs): - settings_middleware = getattr(settings, self._middleware_setting, None) + _middleware_setting = self._get_middleware_setting() + settings_middleware = getattr(settings, _middleware_setting, None) # FIXME This is starting to smell like trouble. We have 2 mechanisms # that may make this condition be True, one implemented in @@ -168,4 +164,11 @@ def _uninstrument(self, **kwargs): return settings_middleware.remove(self._opentelemetry_middleware) - setattr(settings, self._middleware_setting, settings_middleware) + setattr(settings, _middleware_setting, settings_middleware) + + def _get_middleware_setting(self) -> str: + # In Django versions 1.x, setting MIDDLEWARE_CLASSES can be used as a legacy + # alternative to MIDDLEWARE. + if not DJANGO_2_0 and getattr(settings, "MIDDLEWARE", []) is None: + return "MIDDLEWARE_CLASSES" + return "MIDDLEWARE" From 6302077d7d42d3cad4250afa36de06d98bcf8a52 Mon Sep 17 00:00:00 2001 From: Michael Manganiello Date: Tue, 26 Oct 2021 11:13:15 -0300 Subject: [PATCH 5/6] Move middleware setting logic to separate function --- .../instrumentation/django/__init__.py | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/__init__.py b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/__init__.py index ae41dda840..8e6e3913a8 100644 --- a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/__init__.py @@ -100,6 +100,15 @@ def response_hook(span, request, response): _logger = getLogger(__name__) +def _get_django_middleware_setting() -> str: + # In Django versions 1.x, setting MIDDLEWARE_CLASSES can be used as a legacy + # alternative to MIDDLEWARE. This is the case when `settings.MIDDLEWARE` has + # its default value (`None`). + if not DJANGO_2_0 and getattr(settings, "MIDDLEWARE", []) is None: + return "MIDDLEWARE_CLASSES" + return "MIDDLEWARE" + + class DjangoInstrumentor(BaseInstrumentor): """An instrumentor for Django @@ -138,7 +147,7 @@ def _instrument(self, **kwargs): # https://docs.djangoproject.com/en/3.0/topics/http/middleware/#activating-middleware # https://docs.djangoproject.com/en/3.0/ref/middleware/#middleware-ordering - _middleware_setting = self._get_middleware_setting() + _middleware_setting = _get_django_middleware_setting() settings_middleware = getattr(settings, _middleware_setting, []) # Django allows to specify middlewares as a tuple, so we convert this tuple to a @@ -150,7 +159,7 @@ def _instrument(self, **kwargs): setattr(settings, _middleware_setting, settings_middleware) def _uninstrument(self, **kwargs): - _middleware_setting = self._get_middleware_setting() + _middleware_setting = _get_django_middleware_setting() settings_middleware = getattr(settings, _middleware_setting, None) # FIXME This is starting to smell like trouble. We have 2 mechanisms @@ -165,10 +174,3 @@ def _uninstrument(self, **kwargs): settings_middleware.remove(self._opentelemetry_middleware) setattr(settings, _middleware_setting, settings_middleware) - - def _get_middleware_setting(self) -> str: - # In Django versions 1.x, setting MIDDLEWARE_CLASSES can be used as a legacy - # alternative to MIDDLEWARE. - if not DJANGO_2_0 and getattr(settings, "MIDDLEWARE", []) is None: - return "MIDDLEWARE_CLASSES" - return "MIDDLEWARE" From a0ed547c2c4326bfe1c3714460781c46b0cf9d5c Mon Sep 17 00:00:00 2001 From: Michael Manganiello Date: Thu, 28 Oct 2021 10:40:15 -0300 Subject: [PATCH 6/6] Add note for Tox environments for Django --- tox.ini | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tox.ini b/tox.ini index b6b1b9c6c5..3139443ea9 100644 --- a/tox.ini +++ b/tox.ini @@ -30,6 +30,9 @@ envlist = pypy3-test-instrumentation-botocore ; opentelemetry-instrumentation-django + ; Only officially supported Python versions are tested for each Django + ; major release. Updated list can be found at: + ; https://docs.djangoproject.com/en/dev/faq/install/#what-python-version-can-i-use-with-django py3{6,7}-test-instrumentation-django1 py3{6,7,8,9}-test-instrumentation-django2 py3{6,7,8,9,10}-test-instrumentation-django3