Skip to content
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

Adding Custom Middleware Position for Django Instrumentation #2834

Closed
wants to merge 10 commits into from
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
([#1879](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1879))
- Add optional distro and configurator selection for auto-instrumentation
([#1823](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1823))
- Add option to add Opentelemetry middleware at specific position in middleware chain
([#1903]https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1903)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be updated with the new PR number?


### Added
- `opentelemetry-instrumentation-kafka-python` Add instrumentation to `consume` method
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -388,10 +388,25 @@ def _instrument(self, **kwargs):

is_sql_commentor_enabled = kwargs.pop("is_sql_commentor_enabled", None)

otel_position = environ.get("OTEL_PYTHON_DJANGO_MIDDLEWARE_POSITION")
middleware_position = int(otel_position) if otel_position is not None else kwargs.pop("middleware_position", 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exception handling for int?


if len(settings_middleware) < middleware_position:
_logger.debug(
"The middleware_position you provided (%d) is greater than the current number of middlewares (%d). \
Since the number of middlewares is less than the total number of middlewares, the Otel middleware will be appended at the end of the middleware chain.",
middleware_position,
len(settings_middleware),
)
middleware_position = len(settings_middleware)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the default be 0 as it was before?

if is_sql_commentor_enabled:
settings_middleware.insert(0, self._sql_commenter_middleware)
settings_middleware.insert(
middleware_position, self._sql_commenter_middleware
)

settings_middleware.insert(0, self._opentelemetry_middleware)
settings_middleware.insert(
middleware_position, self._opentelemetry_middleware
)

setattr(settings, _middleware_setting, settings_middleware)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,46 @@ def tearDownClass(cls):
super().tearDownClass()
conf.settings = conf.LazySettings()

def test_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
temprory_middelware = "django.utils.deprecation.MiddlewareMixin"
middleware.append(temprory_middelware)
middleware.append(temprory_middelware)

middleware_position = 1
_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):
_django_instrumentor.uninstrument()
if DJANGO_2_0:
middleware = conf.settings.MIDDLEWARE
else:
middleware = conf.settings.MIDDLEWARE_CLASSES
# adding middleware
temprory_middelware = "django.utils.deprecation.MiddlewareMixin"
middleware.append(temprory_middelware)
middleware_position = (
756 # wrong position out of bound of middleware length
)
_django_instrumentor.instrument(
middleware_position=middleware_position
)
self.assertEqual(
middleware[len(middleware) - 1],
"opentelemetry.instrumentation.django.middleware.otel_middleware._DjangoMiddleware",
)

def test_templated_route_get(self):
Client().get("/route/2020/template/")

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,37 @@ def test_middleware_added(self, sqlcommenter_middleware):
in middleware
)

@patch(
"opentelemetry.instrumentation.django.middleware.sqlcommenter_middleware.SqlCommenter"
)
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

# adding two dummy middlewares
temprory_middelware = "django.utils.deprecation.MiddlewareMixin"
middleware.append(temprory_middelware)
middleware.append(temprory_middelware)

middleware_position = 1
_django_instrumentor.instrument(
is_sql_commentor_enabled=True,
middleware_position=middleware_position,
)
instance = sqlcommenter_middleware.return_value
instance.get_response = HttpResponse()
self.assertEqual(
middleware[middleware_position],
"opentelemetry.instrumentation.django.middleware.otel_middleware._DjangoMiddleware",
)
self.assertEqual(
middleware[middleware_position + 1],
"opentelemetry.instrumentation.django.middleware.sqlcommenter_middleware.SqlCommenter",
)

@patch(
"opentelemetry.instrumentation.django.middleware.sqlcommenter_middleware._get_opentelemetry_values"
)
Expand Down
Loading