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

fix(django): Do not patch resolver_match #842

Merged
merged 3 commits into from Sep 29, 2020

Conversation

untitaker
Copy link
Member

Fix #837

@rhcarvalho
Copy link
Contributor

How does this change affect the timings of views?

from sentry_sdk.integrations.django import DjangoIntegration

old_resolve = URLResolver.resolve
old_make_view_atomic = BaseHandler.make_view_atomic
Copy link
Contributor

Choose a reason for hiding this comment

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

Is make_view_atomic available in every version of Django we support?

Had quick look at
https://github.com/django/django/blob/c1442e1192057a3bf14aecbaa1b713eee139eaff/django/core/handlers/base.py#L320

It is not so obvious that this is called for every view once.

Copy link
Member Author

Choose a reason for hiding this comment

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

make_view_atomic is called in get_response_async and get_response which is where all requests go through. It is the only thing called directly before accessing the resolver match, so it should be pretty close with regard to how timings are done.

For example called here: https://github.com/django/django/blob/c1442e1192057a3bf14aecbaa1b713eee139eaff/django/core/handlers/base.py#L174

The impact on timing I could see is that the function wrapper is created at a different point in time, so it could shift instrumentation overhead to a different point in time and therefore move a small gap in teh trace somewhere else.

Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

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

It looks like it could possibly work 😄

Good job @untitaker! As a side-effect we got rid of the csrf_exempt exceptional handling, which is a nice bonus.

@untitaker untitaker merged commit 5d89fa7 into master Sep 29, 2020
@untitaker untitaker deleted the fix/django-no-patch-resolver branch September 29, 2020 15:02
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.

Django view functions replaced by wrapper
2 participants