Skip to content

Allow overriding _DjangoMiddleware via Django settings #3653

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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__
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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)
Original file line number Diff line number Diff line change
Expand Up @@ -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 = (
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand All @@ -33,7 +37,7 @@
)
from opentelemetry.instrumentation.django import (
DjangoInstrumentor,
_DjangoMiddleware,
DjangoMiddleware,
)
from opentelemetry.instrumentation.propagators import (
TraceResponsePropagator,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand All @@ -166,17 +182,93 @@ 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(
middleware_position=middleware_position
)
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):
Expand All @@ -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
)
Expand All @@ -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):
Expand Down Expand Up @@ -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")
Expand All @@ -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()
Expand All @@ -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()
Expand Down
Loading