diff --git a/CHANGELOG.md b/CHANGELOG.md index 9061fdeb1a..d7f63e8096 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,12 +12,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased ### Fixed - +- `opentelemetry-instrumentation-django`: fix trace failures when HTTP requests get cancelled in ASGI workers ([#3848](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3848) - `opentelemetry-instrumentation-botocore`: migrate off the deprecated events API to use the logs API ([#3624](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3624)) - `opentelemetry-instrumentation-dbapi`: fix crash retrieving libpq version when enabling commenter with psycopg ([#3796](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3796)) +### Breaking changes +- `opentelemetry-instrumentation-django`: remove support for Django < 2.0 ([#3848](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3848) ### Added - `opentelemetry-instrumentation-botocore`: Add support for AWS Secrets Manager semantic convention attribute diff --git a/instrumentation/opentelemetry-instrumentation-django/pyproject.toml b/instrumentation/opentelemetry-instrumentation-django/pyproject.toml index 5cd7f7fa4f..ccbf9cd03d 100644 --- a/instrumentation/opentelemetry-instrumentation-django/pyproject.toml +++ b/instrumentation/opentelemetry-instrumentation-django/pyproject.toml @@ -37,7 +37,7 @@ asgi = [ "opentelemetry-instrumentation-asgi == 0.59b0.dev", ] instruments = [ - "django >= 1.10", + "django >= 2.0", ] [project.entry-points.opentelemetry_instrumentor] 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 ebc3e08f4d..52ddd6712c 100644 --- a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/__init__.py @@ -270,21 +270,10 @@ def response_hook(span, request, response): from opentelemetry.trace import get_tracer from opentelemetry.util.http import get_excluded_urls, parse_excluded_urls -DJANGO_2_0 = django_version >= (2, 0) - _excluded_urls_from_env = get_excluded_urls("DJANGO") _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", None) is None: - return "MIDDLEWARE_CLASSES" - return "MIDDLEWARE" - - def _get_django_otel_middleware_position( middleware_length, default_middleware_position=0 ): @@ -387,24 +376,23 @@ 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 = _get_django_middleware_setting() settings_middleware = [] try: - settings_middleware = getattr(settings, _middleware_setting, []) + settings_middleware = getattr(settings, "MIDDLEWARE", []) except ImproperlyConfigured as exception: _logger.debug( "DJANGO_SETTINGS_MODULE environment variable not configured. Defaulting to empty settings: %s", exception, ) settings.configure() - settings_middleware = getattr(settings, _middleware_setting, []) + settings_middleware = getattr(settings, "MIDDLEWARE", []) except ModuleNotFoundError as exception: _logger.debug( "DJANGO_SETTINGS_MODULE points to a non-existent module. Defaulting to empty settings: %s", exception, ) settings.configure() - settings_middleware = getattr(settings, _middleware_setting, []) + settings_middleware = getattr(settings, "MIDDLEWARE", []) # 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 @@ -426,11 +414,10 @@ def _instrument(self, **kwargs): middleware_position, self._opentelemetry_middleware ) - setattr(settings, _middleware_setting, settings_middleware) + setattr(settings, "MIDDLEWARE", settings_middleware) def _uninstrument(self, **kwargs): - _middleware_setting = _get_django_middleware_setting() - settings_middleware = getattr(settings, _middleware_setting, None) + settings_middleware = getattr(settings, "MIDDLEWARE", None) # FIXME This is starting to smell like trouble. We have 2 mechanisms # that may make this condition be True, one implemented in @@ -443,4 +430,4 @@ def _uninstrument(self, **kwargs): return settings_middleware.remove(self._opentelemetry_middleware) - setattr(settings, _middleware_setting, settings_middleware) + setattr(settings, "MIDDLEWARE", settings_middleware) diff --git a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py index f607046959..af3004abb4 100644 --- a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py @@ -77,30 +77,8 @@ except ImportError: 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 - # middlewares can be used. - class MiddlewareMixin: - def __init__(self, get_response): - self.get_response = get_response - - def __call__(self, request): - self.process_request(request) - response = self.get_response(request) - return self.process_response(request, response) - -else: - # Django versions 1.x can use `settings.MIDDLEWARE_CLASSES` and expect - # old-style middlewares, which are created by inheriting from - # `deprecation.MiddlewareMixin` since its creation in Django 1.10 and 1.11, - # or from `object` for older versions. - try: - from django.utils.deprecation import MiddlewareMixin - except ImportError: - MiddlewareMixin = object if DJANGO_3_0: from django.core.handlers.asgi import ASGIRequest @@ -135,7 +113,7 @@ def _is_asgi_request(request: HttpRequest) -> bool: return ASGIRequest is not None and isinstance(request, ASGIRequest) -class _DjangoMiddleware(MiddlewareMixin): +class _DjangoMiddleware: """Django Middleware for OpenTelemetry""" _environ_activation_key = ( @@ -165,6 +143,62 @@ class _DjangoMiddleware(MiddlewareMixin): None ) + def __init__(self, get_response): + self.get_response = get_response + + def __call__(self, request): + excluded_url = self._excluded_urls.url_disabled(request.build_absolute_uri("?")) + is_asgi_request = _is_asgi_request(request) + if excluded_url or is_asgi_request and not _is_asgi_supported: + return self.get_response(request) + + # the request creates an activation and potentially a token + # to clean up + self.process_request(request) + activation = request.META[self._environ_activation_key] + try: + activation.__enter__() + self._run_request_hook(request) + response = self.get_response(request) + return self.process_response(request, response) + finally: + # record any exceptions raised while processing the request + exception = request.META.pop(self._environ_exception_key, None) + + if exception: + activation.__exit__( + type(exception), + exception, + getattr(exception, "__traceback__", None), + ) + else: + activation.__exit__(None, None, None) + + if request.META.get(self._environ_token, None) is not None: + detach(request.META.get(self._environ_token)) + request.META.pop(self._environ_token) + + def _run_request_hook(self, request): + span = request.META[self._environ_span_key] + if _DjangoMiddleware._otel_request_hook: + try: + _DjangoMiddleware._otel_request_hook( # pylint: disable=not-callable + span, request + ) + except Exception: # pylint: disable=broad-exception-caught + # Raising an exception here would leak the request span since process_response + # would not be called. Log the exception instead. + _logger.exception("Exception raised by request_hook") + + def _run_response_hook(self, span, request, response): + if _DjangoMiddleware._otel_response_hook: + try: + _DjangoMiddleware._otel_response_hook( # pylint: disable=not-callable + span, request, response + ) + except Exception: # pylint: disable=broad-exception-caught + _logger.exception("Exception raised by response_hook") + @staticmethod def _get_span_name(request): method = sanitize_method(request.method.strip()) @@ -194,12 +228,7 @@ def process_request(self, request): # Read more about request.META here: # https://docs.djangoproject.com/en/3.0/ref/request-response/#django.http.HttpRequest.META - if self._excluded_urls.url_disabled(request.build_absolute_uri("?")): - return - is_asgi_request = _is_asgi_request(request) - if not _is_asgi_supported and is_asgi_request: - return # pylint:disable=W0212 request._otel_start_time = time() @@ -283,7 +312,6 @@ def process_request(self, request): span.set_attribute(key, value) activation = use_span(span, end_on_exit=True) - activation.__enter__() # pylint: disable=E1101 request_start_time = default_timer() request.META[self._environ_timer_key] = request_start_time request.META[self._environ_activation_key] = activation @@ -291,16 +319,6 @@ def process_request(self, request): if token: request.META[self._environ_token] = token - if _DjangoMiddleware._otel_request_hook: - try: - _DjangoMiddleware._otel_request_hook( # pylint: disable=not-callable - span, request - ) - except Exception: # pylint: disable=broad-exception-caught - # Raising an exception here would leak the request span since process_response - # would not be called. Log the exception instead. - _logger.exception("Exception raised by request_hook") - # pylint: disable=unused-argument def process_view(self, request, view_func, *args, **kwargs): # Process view is executed before the view function, here we get the @@ -340,12 +358,7 @@ def process_exception(self, request, exception): # pylint: disable=too-many-locals # pylint: disable=too-many-statements def process_response(self, request, response): - if self._excluded_urls.url_disabled(request.build_absolute_uri("?")): - return response - is_asgi_request = _is_asgi_request(request) - if not _is_asgi_supported and is_asgi_request: - return response activation = request.META.pop(self._environ_activation_key, None) span = request.META.pop(self._environ_span_key, None) @@ -357,74 +370,56 @@ def process_response(self, request, response): ) request_start_time = request.META.pop(self._environ_timer_key, None) - if activation and span: - if is_asgi_request: - set_status_code( - span, - response.status_code, - metric_attributes=duration_attrs, - sem_conv_opt_in_mode=self._sem_conv_opt_in_mode, - ) + if is_asgi_request: + set_status_code( + span, + response.status_code, + metric_attributes=duration_attrs, + sem_conv_opt_in_mode=self._sem_conv_opt_in_mode, + ) - if span.is_recording() and span.kind == SpanKind.SERVER: - custom_headers = {} - for key, value in response.items(): - asgi_setter.set(custom_headers, key, value) + if span.is_recording() and span.kind == SpanKind.SERVER: + custom_headers = {} + for key, value in response.items(): + asgi_setter.set(custom_headers, key, value) - custom_res_attributes = asgi_collect_custom_headers_attributes( - custom_headers, - SanitizeValue( - get_custom_headers( - OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SANITIZE_FIELDS - ) - ), + custom_res_attributes = asgi_collect_custom_headers_attributes( + custom_headers, + SanitizeValue( get_custom_headers( - OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE - ), - normalise_response_header_name, - ) - for key, value in custom_res_attributes.items(): - span.set_attribute(key, value) - else: - add_response_attributes( - span, - f"{response.status_code} {response.reason_phrase}", - response.items(), - duration_attrs=duration_attrs, - sem_conv_opt_in_mode=self._sem_conv_opt_in_mode, - ) - if span.is_recording() and span.kind == SpanKind.SERVER: - custom_attributes = ( - wsgi_collect_custom_response_headers_attributes( - response.items() + OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SANITIZE_FIELDS ) + ), + get_custom_headers( + OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE + ), + normalise_response_header_name, + ) + for key, value in custom_res_attributes.items(): + span.set_attribute(key, value) + else: + add_response_attributes( + span, + f"{response.status_code} {response.reason_phrase}", + response.items(), + duration_attrs=duration_attrs, + sem_conv_opt_in_mode=self._sem_conv_opt_in_mode, + ) + if span.is_recording() and span.kind == SpanKind.SERVER: + custom_attributes = ( + wsgi_collect_custom_response_headers_attributes( + response.items() ) - if len(custom_attributes) > 0: - span.set_attributes(custom_attributes) + ) + if len(custom_attributes) > 0: + span.set_attributes(custom_attributes) - propagator = get_global_response_propagator() - if propagator: - propagator.inject(response) + propagator = get_global_response_propagator() + if propagator: + propagator.inject(response) - # record any exceptions raised while processing the request - exception = request.META.pop(self._environ_exception_key, None) + self._run_response_hook(span, request, response) - if _DjangoMiddleware._otel_response_hook: - try: - _DjangoMiddleware._otel_response_hook( # pylint: disable=not-callable - span, request, response - ) - except Exception: # pylint: disable=broad-exception-caught - _logger.exception("Exception raised by response_hook") - - if exception: - activation.__exit__( - type(exception), - exception, - getattr(exception, "__traceback__", None), - ) - else: - activation.__exit__(None, None, None) if request_start_time is not None: duration_s = default_timer() - request_start_time @@ -447,9 +442,6 @@ def process_response(self, request, response): max(duration_s, 0), duration_attrs_new ) self._active_request_counter.add(-1, active_requests_count_attrs) - if request.META.get(self._environ_token, None) is not None: - detach(request.META.get(self._environ_token)) - request.META.pop(self._environ_token) return response diff --git a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/package.py b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/package.py index 290061a36f..c828e8f0d6 100644 --- a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/package.py +++ b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/package.py @@ -13,5 +13,5 @@ # limitations under the License. -_instruments = ("django >= 1.10",) +_instruments = ("django >= 2.0",) _supports_metrics = True diff --git a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py index 960bf97bc4..3a4f07379b 100644 --- a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py @@ -73,17 +73,10 @@ 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 path, re_path -else: - from django.conf.urls import url as re_path - - def path(path_argument, *args, **kwargs): - return re_path(rf"^{path_argument}$", *args, **kwargs) +from django.urls import path, re_path urlpatterns = [ @@ -161,10 +154,7 @@ def tearDownClass(cls): def test_middleware_added_at_position(self): _django_instrumentor.uninstrument() - if DJANGO_2_0: - middleware = conf.settings.MIDDLEWARE - else: - middleware = conf.settings.MIDDLEWARE_CLASSES + middleware = conf.settings.MIDDLEWARE # adding two dummy middlewares temprory_middelware = "django.utils.deprecation.MiddlewareMixin" middleware.append(temprory_middelware) @@ -181,10 +171,7 @@ def test_middleware_added_at_position(self): def test_middleware_added_at_position_if_wrong_position(self): _django_instrumentor.uninstrument() - if DJANGO_2_0: - middleware = conf.settings.MIDDLEWARE - else: - middleware = conf.settings.MIDDLEWARE_CLASSES + middleware = conf.settings.MIDDLEWARE # adding middleware temprory_middelware = "django.utils.deprecation.MiddlewareMixin" middleware.append(temprory_middelware) diff --git a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py index a527f404b2..292aa70c6b 100644 --- a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py +++ b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py @@ -14,6 +14,8 @@ # pylint: disable=E0611 +from asyncio import CancelledError +from logging import raiseExceptions from sys import modules from unittest.mock import Mock, patch @@ -22,6 +24,7 @@ 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 import trace as trace_api from opentelemetry.instrumentation._semconv import ( @@ -83,14 +86,8 @@ async_with_custom_header, ) -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"^traced_custom_header/", async_with_custom_header), @@ -826,3 +823,32 @@ async def test_http_custom_response_headers_not_in_span_attributes(self): for key, _ in not_expected.items(): self.assertNotIn(key, span.attributes) self.memory_exporter.clear() + + async def test_cancelled_http_requests_clean_up_spans(self): + """ + An ASGI request handling task can be cancelled by Django + if it detects that a connection is closed. + + This manifests in an asyncio.CancelledError + """ + # HACK unfortunately asyncio.CancelledError bubbles up to the top of the AsyncClient, + # making a bit of a mess + # + # Django's ASGI runner will call task.cancel() in practice if it detects a closed + # connection + with self.assertRaises(CancelledError): + # simulate a cancellation during the response side + # of the middleware + with patch( + "opentelemetry.instrumentation.django.middleware.otel_middleware._DjangoMiddleware.process_response", + # + side_effect=CancelledError, + ): + await self.async_client.get("/traced-custom-error/") + + spans = self.exporter.get_finished_spans() + self.assertEqual(len(spans), 1) + span = spans[0] + self.assertEqual(span.kind, SpanKind.SERVER) + self.assertEqual(span.name, "GET") + self.memory_exporter.clear() diff --git a/instrumentation/opentelemetry-instrumentation-django/tests/test_sqlcommenter.py b/instrumentation/opentelemetry-instrumentation-django/tests/test_sqlcommenter.py index 63f7c5e883..7212be35ba 100644 --- a/instrumentation/opentelemetry-instrumentation-django/tests/test_sqlcommenter.py +++ b/instrumentation/opentelemetry-instrumentation-django/tests/test_sqlcommenter.py @@ -27,8 +27,6 @@ ) from opentelemetry.test.wsgitestutil import WsgiTestBase -DJANGO_2_0 = VERSION >= (2, 0) - _django_instrumentor = DjangoInstrumentor() @@ -62,10 +60,7 @@ def tearDownClass(cls): def test_middleware_added(self, sqlcommenter_middleware): instance = sqlcommenter_middleware.return_value instance.get_response = HttpResponse() - if DJANGO_2_0: - middleware = conf.settings.MIDDLEWARE - else: - middleware = conf.settings.MIDDLEWARE_CLASSES + middleware = conf.settings.MIDDLEWARE self.assertTrue( "opentelemetry.instrumentation.django.middleware.sqlcommenter_middleware.SqlCommenter" @@ -77,10 +72,7 @@ def test_middleware_added(self, sqlcommenter_middleware): ) def test_middleware_added_at_position(self, sqlcommenter_middleware): _django_instrumentor.uninstrument() - if DJANGO_2_0: - middleware = conf.settings.MIDDLEWARE - else: - middleware = conf.settings.MIDDLEWARE_CLASSES + middleware = conf.settings.MIDDLEWARE # adding two dummy middlewares temprory_middelware = "django.utils.deprecation.MiddlewareMixin" @@ -167,9 +159,6 @@ def test_query_wrapper_non_string_queries(self, trace_capture): "opentelemetry.instrumentation.django.middleware.sqlcommenter_middleware._QueryWrapper" ) def test_multiple_connection_support(self, query_wrapper): - if not DJANGO_2_0: - pytest.skip() - requests_mock = MagicMock() get_response = MagicMock()