-
Notifications
You must be signed in to change notification settings - Fork 395
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
Django 2.0 compatibility: User.is_authenticated is now a property #415
Conversation
@sciyoshi thank you very much with this PR! Indeed we're looking on it so that we can review/merge for the next release! Will keep you updated! |
ddtrace/contrib/django/middleware.py
Outdated
@@ -134,7 +134,11 @@ def _set_auth_tags(span, request): | |||
return span | |||
|
|||
if hasattr(user, 'is_authenticated'): | |||
span.set_tag('django.user.is_authenticated', user.is_authenticated()) | |||
try: | |||
is_authenticated = user.is_authenticated() |
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.
Here I think we have a couple of options:
- use a kind of wrapper in a
compat.py
module insidecontrib/django
that retrieves properly the field:
def is_authenticated(user):
if callable(user.is_authenticated):
return user.is_authenticated()
return user.is_authenticated
- ask forgiveness and not the permission (that is idiomatic for Python and is what you're doing), but in this case we hit the exception most of the time (so it's not an exception anymore). Any newer version of Django (>=2.0) will have only the boolean and so we'll always hit the
except
block, introducing the overhead to handle the exception for any request.
Even if I really like your approach, would be great making user.is_authenticated
the default case and not the exception since it will be future-proof.
Does it make sense? What do you think?
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.
Yea, the exception probably isn't great for performance. I've updated the PR with a version check and a wrapper in compat.py.
40b4099
to
7721ae4
Compare
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.
Thanks @sciyoshi for the change! All good to me, I've just added Django 2.0 to our test matrix and double checked that without your change the test suite fails (and it does).
Let's wait for the CI and see if I need to tweak something else otherwise this will be part of the next release.
Thanks again!
Fixes #397 - tracing with Django 2.0 seems to work fine with this small change.