-
Notifications
You must be signed in to change notification settings - Fork 633
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
Conversation
|
@Puneet140500 You need to fix the history committing only from accounts that covered by the CLA |
middleware_position, | ||
len(settings_middleware), | ||
) | ||
middleware_position = len(settings_middleware) |
There was a problem hiding this comment.
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?
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exception handling for int
?
@@ -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) |
There was a problem hiding this comment.
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?
@Puneet140500 , I've made some changes and raised a PR to your branch itself. If they make sense, you can go ahead and merge. I'm hoping for this PR to go live soon as we have a utility in my codebase for it :) |
Closing in favor of #2912 |
Fixes #1781