[Django] #25701: Add warning to an admin list_view if too many queries are being used
Django
-------------------------------+--------------------
Reporter: jacinda | Owner: nobody
Type: New feature | Status: new
Component: contrib.admin | Version:
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
If you use a related field in a callable, 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25701>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jacinda | Owner: jacinda
Type: New feature | Status: assigned
Component: contrib.admin | Version:
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jacinda):
* status: new => assigned
* cc: jacinda.shelly@… (added)
* needs_better_patch: => 0
* needs_tests: => 0
* owner: nobody => jacinda
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/25701#comment:1>
Django
Reporter: jacinda | Owner: jacinda
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Description changed by jacinda:
Old description:
> If you use a related field in a callable, 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.
New description:
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.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/25701#comment:2>
Django
Reporter: jacinda | Owner: jacinda
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
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 in
`list_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) since `connection.queries` is disabled otherwise.
--
Ticket URL: <https://code.djangoproject.com/ticket/25701#comment:3>
Django
Reporter: jacinda | Owner: jacinda
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jacinda):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25701#comment:4>
Django
Reporter: jacinda | Owner: jacinda
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
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:
1. educate newcomers to Django about the N + 1 queries problem in a
textbook example
2. 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25701#comment:5>
Django
Reporter: jacinda | Owner: jacinda
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* version: => master
* stage: Unreviewed => Accepted
Comment:
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()`).
--
Ticket URL: <https://code.djangoproject.com/ticket/25701#comment:6>
Django
Reporter: jacinda | Owner: jacinda
Type: New feature | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
Indeed, if the messages go to logging, there's no need for a mechanism to
opt-out of this feature.
--
Ticket URL: <https://code.djangoproject.com/ticket/25701#comment:7>
Django
Reporter: jacinda | Owner: jacinda
Type: New feature | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jacinda):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25701#comment:8>
Django
Reporter: Jacinda Shelly | Owner: (none)
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* owner: Jacinda Shelly => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/25701#comment:9>
Django
Reporter: Jacinda Shelly | Owner: (none)
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Changes (by Ülgen Sarıkavak):
* cc: Ülgen Sarıkavak (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/25701#comment:10>