Skip to content

Django logging not working when djdt is enabled #1814

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

Closed
forrestkouakou opened this issue Jul 10, 2023 · 12 comments · Fixed by #1820
Closed

Django logging not working when djdt is enabled #1814

forrestkouakou opened this issue Jul 10, 2023 · 12 comments · Fixed by #1820
Labels

Comments

@forrestkouakou
Copy link

I recently noticed that django default logging is not working when I enable djdt.
django-debug-toolbar==4.1.0
Django==4.2.3

Is it possible to have SQL pannel and logging for sql queries work together?

Without djdt
Screenshot 2023-07-10 at 01 00 07

With djdt
Screenshot 2023-07-10 at 01 02 08

@tim-schilling
Copy link
Member

Hi @forrestkouakou, can you create a reproducible environment for this?

@dannosaur
Copy link

I've just dug deep into django's guts to figure out why our console logging isn't working either. It looks like it's caused by the DjDTCursorWrapper (debug_toolbar/panels/sql/tracking:wrap_cursor()) overriding the default CursorDebugWrapper that django installs based on settings.DEBUG or an option set somehow to force it. When wrap_cursor() is called by djdt, it's returning an instance of NormalCursorWrapper which eventually extends CursorWrapper (not CursorDebugWrapper where the logging functions are).

My guess is that the resolution to this is that DjDTCursorWrapper needs to be mindful of what cursor django has installed, and if it's installed CursorDebugWrapper instead of CursorWrapper then it should inherit that instead. Something can probably modify DjDTCursorWrapper.__bases__ or use a function in the class inheritance instead in order to dynamically inherit the correct cursor wrapper class.

@living180
Copy link
Contributor

@tim-schilling based on this would you support a revert of e7575e8?

@tim-schilling
Copy link
Member

@dannosaur thank you for that investigation! That's super helpful. Do you have a reproducible use case?

@living180 I think I'm on board for a revert, though I'd prefer to roll forward a fix.

@dannosaur
Copy link

@tim-schilling yes, I can reproduce the issue in my project.

@tim-schilling
Copy link
Member

@dannosaur Sorry, I meant can you create a way for me to reproduce the error so that I can investigate and attempt a fix?

@dannosaur
Copy link

dannosaur commented Jul 25, 2023

@tim-schilling lol, sorry! I'll try and put something together quickly tomorrow that reproduces the issue. I don't think it's a complex setup though - django 4.2, any version of psycopg (2 or 3), and djdt 4.1. Leave it with me, I'll post back tomorrow with a git repo.

Edit: it's probably the case with all the database drivers, but I can only test with postgres. I don't think the engine is relevant though.

@tim-schilling
Copy link
Member

No worries. I can set up postgres on my end. Having the ability to pull a repo, set up a database and reproduce the problem makes this much, much easier to investigate. Otherwise I'm attempting random things to try to reproduce the behavior. I appreciate your work on this so far, thank you!

@dannosaur
Copy link

dannosaur commented Jul 28, 2023

@tim-schilling here you go: https://github.com/dannosaur/djdttest

The django admin is installed with a user. Login dan/password. Observe the terminal running runserver - there should be some queries printed directed at auth_user table (there isn't though, demonstrating this issue). In settings.py I put a DISABLE_PANELS section with a commented out item. If you uncomment that line (158) and reload the admin user list page, you'll see the SQL query in the console.

LMK if you need anything else, happy to help!

@tim-schilling
Copy link
Member

Thank you @dannosaur, I was able to reproduce this.

@tim-schilling
Copy link
Member

Can folks test their project against #1820 to confirm that works?

@dannosaur
Copy link

@tim-schilling confirmed working now in my project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants