Async compatible SQLPanel
#1993
Conversation
|
@tim-schilling I think this is ready as per the discussion tests have been covered by #1835 |
debug_toolbar/middleware.py
Outdated
| if panel.panel_id == "SQLPanel": | ||
| await sync_to_async(panel.enable_instrumentation)() |
There was a problem hiding this comment.
Is it possible to do this within the SQLPanel.enable_instrumentation itself?
There was a problem hiding this comment.
It's a bit odd to see specific logic for a single panel in the middleware. It'll be easier to understand the middleware if we don't have specific panel logic within it. It's also easier to explain why we do that in the panel and have it be in the place a person is most likely to be looking for it.
There was a problem hiding this comment.
if think for future panel or update the "panel.panel_id == "SQLPanel"" is not great
i would put something like this
if hasttr(panel, "aenable_instrumentation"):
panel.aenable_instrumentation()
else:
panel.enable_instrumentation()
This is more generic and if there is a "afunction" you can put in the afunction comment why it's needed
There was a problem hiding this comment.
we only have one panel as of now that would utilise this pattern, though it seems really intuitive.
There was a problem hiding this comment.
yep but if a people create a third party panel he need to change the code of the core for make work. That's not a great thing.
Also you may finish with a thing like if panel.panel_id in [very long and boring array].... Not a beautiful code in my opinion.
debug_toolbar/panels/__init__.py
Outdated
| @@ -154,6 +154,9 @@ def enable_instrumentation(self): | |||
|
|
|||
| Unless the toolbar or this panel is disabled, this method will be | |||
| called early in ``DebugToolbarMiddleware``. It should be idempotent. | |||
|
|
|||
| Add aenable_instrumentation method to the panel class to support async | |||
| instrumentation logic. | |||
There was a problem hiding this comment.
maybe an "if needed" ? it's not mandatory for async to have a aenable ?
There was a problem hiding this comment.
yup, although async instrumentation logic itself means that await is required in that case aenable_instrumentation would be the only choice?
…-debug-toolbar into async-sql-panel
|
@tim-schilling Apparently |
Description
By instrumenting the
SQLPanelinsync_to_asyncwe can monkey patch all the connection in a dedicated thread and store those connections there meanwhile all cross compatible views and operations in async context will take place in that threaded thus they will use those patched db connections which will help us store all async and sync operations.more explanation of
sync_to_async: https://docs.djangoproject.com/en/4.2/topics/async/#sync-to-asyncFixes #1430
Fixes #932
Checklist:
docs/changes.rst.