Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Async compatible SQLPanel #1993

Merged
merged 22 commits into from
Sep 2, 2024
Merged

Async compatible SQLPanel #1993

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
6cb6396
ASGI check approach with added test and docs
salty-ivy Aug 20, 2024
830c964
revert changes
salty-ivy Aug 20, 2024
1c39baa
Async compatible
salty-ivy Aug 20, 2024
7e1993f
asynchornise instrumentaion internally in panel logic rather than mid…
salty-ivy Aug 27, 2024
a5970f5
fix docstring
salty-ivy Aug 27, 2024
284c369
add period in the docstring comment
salty-ivy Aug 27, 2024
263e856
mark aenable_instrumentation as mehtod
salty-ivy Aug 27, 2024
7bd5314
add asgi and aenable_instrumentation in wordlist
salty-ivy Aug 27, 2024
9ecd36d
fix word spelling
salty-ivy Aug 27, 2024
d9dbb92
Spelling changes?
tim-schilling Aug 27, 2024
4cd962c
merged with main
salty-ivy Aug 31, 2024
b926820
Merge branch 'async-sql-panel' of https://github.com/salty-ivy/django…
salty-ivy Aug 31, 2024
a6f6b49
remove docstring from base enable_instrumentation
salty-ivy Aug 31, 2024
6ba6a6d
add the revised docstring back again
salty-ivy Aug 31, 2024
07f175a
aenable for testing lint failures
salty-ivy Aug 31, 2024
e44b305
remove instrumentation keyword from docs
salty-ivy Aug 31, 2024
d954bda
add instrumentation in spelling list
salty-ivy Aug 31, 2024
252ae07
remove underscore
salty-ivy Aug 31, 2024
bde0bed
remove instrumentation
salty-ivy Aug 31, 2024
bae89e6
add instrumentation word in docstring
salty-ivy Aug 31, 2024
e406cf8
add instrumentation word in docstring
salty-ivy Aug 31, 2024
0532e7a
Update language for enable_instrumentation docstring.
tim-schilling Sep 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions debug_toolbar/middleware.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import socket
from functools import lru_cache

from asgiref.sync import iscoroutinefunction, markcoroutinefunction
from asgiref.sync import iscoroutinefunction, markcoroutinefunction, sync_to_async
from django.conf import settings
from django.utils.module_loading import import_string

Expand Down Expand Up @@ -112,7 +112,10 @@ async def __acall__(self, request):

# Activate instrumentation ie. monkey-patch.
for panel in toolbar.enabled_panels:
panel.enable_instrumentation()
if panel.panel_id == "SQLPanel":
await sync_to_async(panel.enable_instrumentation)()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to do this within the SQLPanel.enable_instrumentation itself?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@elineda elineda Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I like that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we only have one panel as of now that would utilise this pattern, though it seems really intuitive.

Copy link
Contributor

@elineda elineda Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else:
panel.enable_instrumentation()
try:
# Run panels like Django middleware.
response = await toolbar.process_request(request)
Expand Down
2 changes: 1 addition & 1 deletion debug_toolbar/panels/sql/panel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ class SQLPanel(Panel):
the request.
"""

is_async = False
is_async = True

def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
Expand Down
4 changes: 2 additions & 2 deletions docs/architecture.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@ Problematic Parts
the main benefit of the toolbar
- Support for async and multi-threading: ``debug_toolbar.middleware.DebugToolbarMiddleware``
is now async compatible and can process async requests. However certain
panels such as ``SQLPanel``, ``TimerPanel``,
``RequestPanel``, ``HistoryPanel`` and ``ProfilingPanel`` aren't fully
panels such as ``TimerPanel``, ``RequestPanel``,
``HistoryPanel`` and ``ProfilingPanel`` aren't fully
compatible and currently being worked on. For now, these panels
are disabled by default when running in async environment.
follow the progress of this issue in `Async compatible toolbar project <https://github.com/orgs/jazzband/projects/9>`_.