Opened 9 years ago
Last modified 8 months ago
#25701 new New feature
Add warning to an admin list_view if too many queries are being used
Reported by: | Jacinda Shelly | Owned by: | |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | jacinda.shelly@…, Ülgen Sarıkavak | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
If you use a related field in a callable on a Model or ModelAdmin, and then use that callable in list_display, Django doesn't currently have a way to automatically detect that select_related should be used and performs a query for every row in the list.
While it might be possible to do this automatically, a quick way to help with this problem would be to warn a developer with something like django.contrib.messages (if enabled) that a list_view is performing O(n) queries and that they should investigate this as a potential performance issue.
Change History (10)
comment:1 by , 9 years ago
Cc: | added |
---|---|
Owner: | changed from | to
Status: | new → assigned |
comment:2 by , 9 years ago
Description: | modified (diff) |
---|
comment:3 by , 9 years ago
comment:4 by , 9 years ago
I don't think this would be a replacement for django-debug-toolbar. I think it's actually most useful if the warning suggests to the user that they use django-debug-toolbar or a similar tool to diagnose the cause of the high number of queries. This just lets them know that there's something that should be investigated.
I think the checks framework suggestion is good, but wouldn’t help people detect if they had a poorly constructed callable in list_display since it would be the callable name in list_select_related and not a field name.
Yep, I was assuming this feature would only work in DEBUG mode. I don't think this is something most people would want to show up in production.
comment:5 by , 9 years ago
We discussed this at Django under the Hood. There aren't that many ways to detect and report the problem at runtime. I think a check when DEBUG = True
is a good idea.
I've had this issue on sites where I use the admin and where I have the debug toolbar installed in dev. I only discovered and fixed in when I wondered "doesn't this page feel a bit slow?" I wasn't obvious — just the difference between 50 and 250 ms.
In my opinion this feature would achieve two results:
- educate newcomers to Django about the N + 1 queries problem in a textbook example
- quickly alert more experienced developers when the accidentally create such a problem
It would need a way to turn it off on with a ModelAdmin option.
I agree that it feels weird to use django.contrib.messages
to relay messages to the developer at runtime. Furthermore, I just realized we have an ordering problem: the admin templates renders messages before iterating on the queryset to render the changelist. I can't see how we'd implement that.
I suppose the more traditional alternative to relay that information to the developer is through logging.
comment:6 by , 9 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Version: | → master |
Using the console log handler seems reasonable to me (maybe at the INFO level?). Aymeric, could you elaborate on why you want a ModelAdmin
option to disable it? I think it would be easy enough to ignore the message if you don't care about it, but maybe there's some other reason you had in mind? If we do add such an option, I wonder if it's better suited to ChangeList
(which we'd likely need to start documenting as a public API).
Related the last point, Ola pointed out that the ModelAdmin.get_search_results()
method really belongs on ChangeList
, but we put it on ModelAdmin
because the latter class isn't documented and requires a bit more work to override (using ModelAdmin.get_changelist()
).
comment:7 by , 9 years ago
Indeed, if the messages go to logging, there's no need for a mechanism to opt-out of this feature.
comment:8 by , 9 years ago
I'll try to get an example setup with messages going to logging instead of django.contrib.messages
. Maybe it would be more visible if the messages were logged at a WARNING level by default? We could add an option to set this level or disable the logging entirely if a developer didn't want it.
comment:9 by , 3 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:10 by , 8 months ago
Cc: | added |
---|
After some thought, I'm a bit hesitant to duplicate functionality (to some extent) provided by django-debug-toolbar. I think there's consensus to recommend that package in the docs (perhaps in the tutorial). While this will require the developer to understand things at a slightly deeper level, it seems like a more teachable solution in the long run. What do you think?
One partial solution would be to use the checks framework (at the INFO level, perhaps) to detect relations in
list_display
that aren't inlist_select_related
. Of course this won't cover O(n) queries in callables, but I'm not yet convinced a more sophisticated solution is worthwhile. I guess your proposal would only work in DEBUG mode (which makes sense to me since we probably wouldn't want end users to see the warning) sinceconnection.queries
is disabled otherwise.