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

Conversation

Puneet140500
Copy link

@Puneet140500 Puneet140500 commented Aug 28, 2024

Fixes #1781

@Puneet140500 Puneet140500 requested a review from a team August 28, 2024 08:59
Copy link

linux-foundation-easycla bot commented Aug 28, 2024

CLA Missing ID CLA Not Signed

@Puneet140500 Puneet140500 changed the title Adding Environment Variable for Auto Instrumentation Adding Custom Middleware Position for Django Instrumentation Aug 28, 2024
@xrmx
Copy link
Contributor

xrmx commented Aug 30, 2024

@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)
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?

@@ -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?

@@ -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?

@saichander17
Copy link
Contributor

@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 :)
https://github.com/Puneet140500/opentelemetry-python-contrib/pull/1/files

@xrmx
Copy link
Contributor

xrmx commented Oct 17, 2024

Closing in favor of #2912

@xrmx xrmx closed this Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DisallowedHost exception for Django instrumentation
6 participants