diff --git a/CHANGELOG.md b/CHANGELOG.md index 05d3b39006..60605c8135 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +- `opentelemetry-instrumentation-django`: Add settings to allow overriding `_DjangoMiddleware` ([#3653](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3653)) + ### Fixed - `opentelemetry-instrumentation`: Fix dependency conflict detection when instrumented packages are not installed by moving check back to before instrumentors are loaded. Add "instruments-any" feature for instrumentations that target multiple packages. 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 7d92aff145..4fde36e8cd 100644 --- a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/__init__.py @@ -244,6 +244,7 @@ def response_hook(span, request, response): from django import VERSION as django_version from django.conf import settings from django.core.exceptions import ImproperlyConfigured +from django.utils.module_loading import import_string from opentelemetry.instrumentation._semconv import ( HTTP_DURATION_HISTOGRAM_BUCKETS_NEW, @@ -257,7 +258,7 @@ def response_hook(span, request, response): OTEL_PYTHON_DJANGO_INSTRUMENT, ) from opentelemetry.instrumentation.django.middleware.otel_middleware import ( - _DjangoMiddleware, + DjangoMiddleware, ) from opentelemetry.instrumentation.django.package import _instruments from opentelemetry.instrumentation.django.version import __version__ @@ -319,7 +320,7 @@ class DjangoInstrumentor(BaseInstrumentor): """ _opentelemetry_middleware = ".".join( - [_DjangoMiddleware.__module__, _DjangoMiddleware.__qualname__] + [DjangoMiddleware.__module__, DjangoMiddleware.__qualname__] ) _sql_commenter_middleware = "opentelemetry.instrumentation.django.middleware.sqlcommenter_middleware.SqlCommenter" @@ -354,34 +355,34 @@ def _instrument(self, **kwargs): meter_provider=meter_provider, schema_url=_get_schema_url(sem_conv_opt_in_mode), ) - _DjangoMiddleware._sem_conv_opt_in_mode = sem_conv_opt_in_mode - _DjangoMiddleware._tracer = tracer - _DjangoMiddleware._meter = meter - _DjangoMiddleware._excluded_urls = ( + DjangoMiddleware._sem_conv_opt_in_mode = sem_conv_opt_in_mode + DjangoMiddleware._tracer = tracer + DjangoMiddleware._meter = meter + DjangoMiddleware._excluded_urls = ( _excluded_urls_from_env if _excluded_urls is None else parse_excluded_urls(_excluded_urls) ) - _DjangoMiddleware._otel_request_hook = kwargs.pop("request_hook", None) - _DjangoMiddleware._otel_response_hook = kwargs.pop( + DjangoMiddleware._otel_request_hook = kwargs.pop("request_hook", None) + DjangoMiddleware._otel_response_hook = kwargs.pop( "response_hook", None ) - _DjangoMiddleware._duration_histogram_old = None + DjangoMiddleware._duration_histogram_old = None if _report_old(sem_conv_opt_in_mode): - _DjangoMiddleware._duration_histogram_old = meter.create_histogram( + DjangoMiddleware._duration_histogram_old = meter.create_histogram( name=MetricInstruments.HTTP_SERVER_DURATION, unit="ms", description="Measures the duration of inbound HTTP requests.", ) - _DjangoMiddleware._duration_histogram_new = None + DjangoMiddleware._duration_histogram_new = None if _report_new(sem_conv_opt_in_mode): - _DjangoMiddleware._duration_histogram_new = meter.create_histogram( + DjangoMiddleware._duration_histogram_new = meter.create_histogram( name=HTTP_SERVER_REQUEST_DURATION, description="Duration of HTTP server requests.", unit="s", explicit_bucket_boundaries_advisory=HTTP_DURATION_HISTOGRAM_BUCKETS_NEW, ) - _DjangoMiddleware._active_request_counter = ( + DjangoMiddleware._active_request_counter = ( create_http_server_active_requests(meter) ) # This can not be solved, but is an inherent problem of this approach: @@ -425,25 +426,52 @@ def _instrument(self, **kwargs): middleware_position, self._sql_commenter_middleware ) - settings_middleware.insert( - middleware_position, self._opentelemetry_middleware + otel_middleware = getattr( + settings, "OTEL_DJANGO_MIDDLEWARE", self._opentelemetry_middleware ) + if not self._middleware_is_valid(otel_middleware): + _logger.debug( + 'Middleware specified at settings.OTEL_DJANGO_MIDDLEWARE is not an instance of "%s". Switching to "%s".', + self._opentelemetry_middleware, + self._opentelemetry_middleware, + ) + otel_middleware = self._opentelemetry_middleware + + settings_middleware.insert(middleware_position, otel_middleware) setattr(settings, _middleware_setting, settings_middleware) + def _middleware_is_valid(self, middleware_path: str) -> bool: + if middleware_path == self._opentelemetry_middleware: + return True + try: + middleware_cls = import_string(middleware_path) + except ModuleNotFoundError: + return False + + # Require the custom middleware to inherit from `DjangoMiddleware` because we do some + # patching to that class that the custom one needs to inherit. + return isinstance(middleware_cls, type) and issubclass( + middleware_cls, DjangoMiddleware + ) + def _uninstrument(self, **kwargs): _middleware_setting = _get_django_middleware_setting() settings_middleware = getattr(settings, _middleware_setting, None) + otel_middleware = getattr( + settings, "OTEL_DJANGO_MIDDLEWARE", self._opentelemetry_middleware + ) # FIXME This is starting to smell like trouble. We have 2 mechanisms # that may make this condition be True, one implemented in # BaseInstrumentor and another one implemented in _instrument. Both # stop _instrument from running and thus, settings_middleware not being # set. - if settings_middleware is None or ( - self._opentelemetry_middleware not in settings_middleware + if ( + settings_middleware is None + or otel_middleware not in settings_middleware ): return - settings_middleware.remove(self._opentelemetry_middleware) + settings_middleware.remove(otel_middleware) setattr(settings, _middleware_setting, 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..f56236c6b4 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 @@ -135,7 +135,7 @@ def _is_asgi_request(request: HttpRequest) -> bool: return ASGIRequest is not None and isinstance(request, ASGIRequest) -class _DjangoMiddleware(MiddlewareMixin): +class DjangoMiddleware(MiddlewareMixin): """Django Middleware for OpenTelemetry""" _environ_activation_key = ( @@ -291,9 +291,9 @@ def process_request(self, request): if token: request.META[self._environ_token] = token - if _DjangoMiddleware._otel_request_hook: + if DjangoMiddleware._otel_request_hook: try: - _DjangoMiddleware._otel_request_hook( # pylint: disable=not-callable + DjangoMiddleware._otel_request_hook( # pylint: disable=not-callable span, request ) except Exception: # pylint: disable=broad-exception-caught @@ -409,9 +409,9 @@ def process_response(self, request, response): # record any exceptions raised while processing the request exception = request.META.pop(self._environ_exception_key, None) - if _DjangoMiddleware._otel_response_hook: + if DjangoMiddleware._otel_response_hook: try: - _DjangoMiddleware._otel_response_hook( # pylint: disable=not-callable + DjangoMiddleware._otel_response_hook( # pylint: disable=not-callable span, request, response ) except Exception: # pylint: disable=broad-exception-caught diff --git a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py index 960bf97bc4..7c75047c76 100644 --- a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py @@ -22,7 +22,11 @@ from django import VERSION, conf 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.test.utils import ( + override_settings, + setup_test_environment, + teardown_test_environment, +) from opentelemetry import trace from opentelemetry.instrumentation._semconv import ( @@ -33,7 +37,7 @@ ) from opentelemetry.instrumentation.django import ( DjangoInstrumentor, - _DjangoMiddleware, + DjangoMiddleware, ) from opentelemetry.instrumentation.propagators import ( TraceResponsePropagator, @@ -100,6 +104,18 @@ def path(path_argument, *args, **kwargs): _django_instrumentor = DjangoInstrumentor() +class CustomMiddleware(DjangoMiddleware): + pass + + +class RandomMiddleware: + def __init__(self, get_response): + self.get_response = get_response + + def __call__(self, request): + return self.get_response(request) + + # pylint: disable=too-many-public-methods class TestMiddleware(WsgiTestBase): @classmethod @@ -136,11 +152,11 @@ def setUp(self): self.env_patch.start() _django_instrumentor.instrument() self.exclude_patch = patch( - "opentelemetry.instrumentation.django.middleware.otel_middleware._DjangoMiddleware._excluded_urls", + "opentelemetry.instrumentation.django.middleware.otel_middleware.DjangoMiddleware._excluded_urls", get_excluded_urls("DJANGO"), ) self.traced_patch = patch( - "opentelemetry.instrumentation.django.middleware.otel_middleware._DjangoMiddleware._traced_request_attrs", + "opentelemetry.instrumentation.django.middleware.otel_middleware.DjangoMiddleware._traced_request_attrs", get_traced_request_attrs("DJANGO"), ) self.exclude_patch.start() @@ -166,9 +182,9 @@ def test_middleware_added_at_position(self): else: middleware = conf.settings.MIDDLEWARE_CLASSES # adding two dummy middlewares - temprory_middelware = "django.utils.deprecation.MiddlewareMixin" - middleware.append(temprory_middelware) - middleware.append(temprory_middelware) + temporary_middleware = "django.utils.deprecation.MiddlewareMixin" + middleware.append(temporary_middleware) + middleware.append(temporary_middleware) middleware_position = 1 _django_instrumentor.instrument( @@ -176,7 +192,83 @@ def test_middleware_added_at_position(self): ) self.assertEqual( middleware[middleware_position], - "opentelemetry.instrumentation.django.middleware.otel_middleware._DjangoMiddleware", + "opentelemetry.instrumentation.django.middleware.otel_middleware.DjangoMiddleware", + ) + + def test_custom_middleware_added_at_position(self): + _django_instrumentor.uninstrument() + if DJANGO_2_0: + middleware = conf.settings.MIDDLEWARE + else: + middleware = conf.settings.MIDDLEWARE_CLASSES + # adding two dummy middlewares + temporary_middleware = "django.utils.deprecation.MiddlewareMixin" + middleware.append(temporary_middleware) + middleware.append(temporary_middleware) + custom_middleware = ( + CustomMiddleware.__module__ + "." + CustomMiddleware.__qualname__ + ) + middleware_position = 1 + + with override_settings(OTEL_DJANGO_MIDDLEWARE=custom_middleware): + _django_instrumentor.instrument( + middleware_position=middleware_position + ) + + self.assertEqual( + middleware[middleware_position], custom_middleware + ) + + _django_instrumentor.uninstrument() # Uninstrument with the settings overridden + + def test_random_middleware_refused_in_favor_of_django_middleware(self): + _django_instrumentor.uninstrument() + if DJANGO_2_0: + middleware = conf.settings.MIDDLEWARE + else: + middleware = conf.settings.MIDDLEWARE_CLASSES + # adding two dummy middlewares + temporary_middleware = "django.utils.deprecation.MiddlewareMixin" + middleware.append(temporary_middleware) + middleware.append(temporary_middleware) + random_middleware = ( + RandomMiddleware.__module__ + "." + RandomMiddleware.__qualname__ + ) + middleware_position = 1 + + with override_settings(OTEL_DJANGO_MIDDLEWARE=random_middleware): + _django_instrumentor.instrument( + middleware_position=middleware_position + ) + + self.assertEqual( + middleware[middleware_position], + "opentelemetry.instrumentation.django.middleware.otel_middleware.DjangoMiddleware", + ) + + def test_custom_middleware_does_not_exist_default_to_django_middleware( + self, + ): + _django_instrumentor.uninstrument() + if DJANGO_2_0: + middleware = conf.settings.MIDDLEWARE + else: + middleware = conf.settings.MIDDLEWARE_CLASSES + # adding two dummy middlewares + temporary_middleware = "django.utils.deprecation.MiddlewareMixin" + middleware.append(temporary_middleware) + middleware.append(temporary_middleware) + non_existing_middleware = "path.to.a.non.existing.Middleware" + middleware_position = 1 + + with override_settings(OTEL_DJANGO_MIDDLEWARE=non_existing_middleware): + _django_instrumentor.instrument( + middleware_position=middleware_position + ) + + self.assertEqual( + middleware[middleware_position], + "opentelemetry.instrumentation.django.middleware.otel_middleware.DjangoMiddleware", ) def test_middleware_added_at_position_if_wrong_position(self): @@ -186,8 +278,8 @@ def test_middleware_added_at_position_if_wrong_position(self): else: middleware = conf.settings.MIDDLEWARE_CLASSES # adding middleware - temprory_middelware = "django.utils.deprecation.MiddlewareMixin" - middleware.append(temprory_middelware) + temporary_middleware = "django.utils.deprecation.MiddlewareMixin" + middleware.append(temporary_middleware) middleware_position = ( 756 # wrong position out of bound of middleware length ) @@ -196,7 +288,7 @@ def test_middleware_added_at_position_if_wrong_position(self): ) self.assertEqual( middleware[0], - "opentelemetry.instrumentation.django.middleware.otel_middleware._DjangoMiddleware", + "opentelemetry.instrumentation.django.middleware.otel_middleware.DjangoMiddleware", ) def test_templated_route_get(self): @@ -605,12 +697,12 @@ def response_hook(span, request, response): response_hook_args = (span, request, response) response["hook-header"] = "set by hook" - _DjangoMiddleware._otel_request_hook = request_hook - _DjangoMiddleware._otel_response_hook = response_hook + DjangoMiddleware._otel_request_hook = request_hook + DjangoMiddleware._otel_response_hook = response_hook response = Client().get("/span_name/1234/") - _DjangoMiddleware._otel_request_hook = ( - _DjangoMiddleware._otel_response_hook + DjangoMiddleware._otel_request_hook = ( + DjangoMiddleware._otel_response_hook ) = None self.assertEqual(response["hook-header"], "set by hook") @@ -636,9 +728,9 @@ def request_hook(span, request): # pylint: disable=broad-exception-raised raise Exception("request hook exception") - _DjangoMiddleware._otel_request_hook = request_hook + DjangoMiddleware._otel_request_hook = request_hook Client().get("/span_name/1234/") - _DjangoMiddleware._otel_request_hook = None + DjangoMiddleware._otel_request_hook = None # ensure that span ended finished_spans = self.memory_exporter.get_finished_spans() @@ -649,9 +741,9 @@ def response_hook(span, request, response): # pylint: disable=broad-exception-raised raise Exception("response hook exception") - _DjangoMiddleware._otel_response_hook = response_hook + DjangoMiddleware._otel_response_hook = response_hook Client().get("/span_name/1234/") - _DjangoMiddleware._otel_response_hook = None + DjangoMiddleware._otel_response_hook = None # ensure that span ended finished_spans = self.memory_exporter.get_finished_spans() diff --git a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py index d06c9c635c..72cc2a9575 100644 --- a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py +++ b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware_asgi.py @@ -30,7 +30,7 @@ ) from opentelemetry.instrumentation.django import ( DjangoInstrumentor, - _DjangoMiddleware, + DjangoMiddleware, ) from opentelemetry.instrumentation.propagators import ( TraceResponsePropagator, @@ -137,11 +137,11 @@ def setUp(self): self.env_patch.start() _django_instrumentor.instrument() self.exclude_patch = patch( - "opentelemetry.instrumentation.django.middleware.otel_middleware._DjangoMiddleware._excluded_urls", + "opentelemetry.instrumentation.django.middleware.otel_middleware.DjangoMiddleware._excluded_urls", get_excluded_urls("DJANGO"), ) self.traced_patch = patch( - "opentelemetry.instrumentation.django.middleware.otel_middleware._DjangoMiddleware._traced_request_attrs", + "opentelemetry.instrumentation.django.middleware.otel_middleware.DjangoMiddleware._traced_request_attrs", get_traced_request_attrs("DJANGO"), ) self.exclude_patch.start() @@ -573,12 +573,12 @@ def response_hook(span, request, response): response_hook_args = (span, request, response) response["hook-header"] = "set by hook" - _DjangoMiddleware._otel_request_hook = request_hook - _DjangoMiddleware._otel_response_hook = response_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 + DjangoMiddleware._otel_request_hook = ( + DjangoMiddleware._otel_response_hook ) = None self.assertEqual(response["hook-header"], "set by hook") diff --git a/instrumentation/opentelemetry-instrumentation-django/tests/test_sqlcommenter.py b/instrumentation/opentelemetry-instrumentation-django/tests/test_sqlcommenter.py index 63f7c5e883..d491c4d741 100644 --- a/instrumentation/opentelemetry-instrumentation-django/tests/test_sqlcommenter.py +++ b/instrumentation/opentelemetry-instrumentation-django/tests/test_sqlcommenter.py @@ -96,7 +96,7 @@ def test_middleware_added_at_position(self, sqlcommenter_middleware): instance.get_response = HttpResponse() self.assertEqual( middleware[middleware_position], - "opentelemetry.instrumentation.django.middleware.otel_middleware._DjangoMiddleware", + "opentelemetry.instrumentation.django.middleware.otel_middleware.DjangoMiddleware", ) self.assertEqual( middleware[middleware_position + 1],