Opened 5 years ago

Last modified 5 years ago

#25701 assigned New feature

Add warning to an admin list_view if too many queries are being used

Reported by: Jacinda Shelly Owned by: Jacinda Shelly
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: jacinda.shelly@… 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 Jacinda Shelly)

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 (8)

comment:1 Changed 5 years ago by Jacinda Shelly

Cc: jacinda.shelly@… added
Owner: changed from nobody to Jacinda Shelly
Status: newassigned

comment:2 Changed 5 years ago by Jacinda Shelly

Description: modified (diff)

comment:3 Changed 5 years ago by Tim Graham

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.

comment:4 Changed 5 years ago by Jacinda Shelly

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 Changed 5 years ago by Aymeric Augustin

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.

comment:6 Changed 5 years ago by Tim Graham

Triage Stage: UnreviewedAccepted
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 Changed 5 years ago by Aymeric Augustin

Indeed, if the messages go to logging, there's no need for a mechanism to opt-out of this feature.

comment:8 Changed 5 years ago by Jacinda Shelly

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.

Note: See TracTickets for help on using tickets.
Back to Top