Skip to content

Add support for enabling/disabling SQLPanel tracking of DDT model queries #2211

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Add support for enabling/disabling SQLPanel tracking of DDT model queries #2211

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c63ee81
refactor: add contextvar to enable/disable tracking of DDT models
theShinigami Sep 25, 2025
efbff9a
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Sep 25, 2025
9f125bd
chore: ruff B012
theShinigami Sep 25, 2025
115f3a0
refactor: add `TRACK_DDT_MODELS` to override tracking DDT models
theShinigami Oct 3, 2025
2f61273
refactor: add `TRACK_DDT_MODELS` to override tracking DDT models
theShinigami Oct 3, 2025
1fe70b6
chore: remove leftover code from the previous change
theShinigami Oct 3, 2025
cb77283
refactor: small improvements
theShinigami Oct 3, 2025
52d9f14
refactor: add doc string to helper function and tests
theShinigami Oct 3, 2025
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
18 changes: 18 additions & 0 deletions debug_toolbar/panels/sql/tracking.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from time import perf_counter

import django.test.testcases
from django.apps import apps
from django.utils.encoding import force_str

from debug_toolbar.utils import get_stack_trace, get_template_info
Expand All @@ -27,6 +28,17 @@
# additional queries.
allow_sql = contextvars.ContextVar("debug-toolbar-allow-sql", default=True)

# Prevents tracking of DDT models
allow_ddt_models_tracking = contextvars.ContextVar(
"debug-toolbar-allow-ddt-models", default=False
)
Copy link
Member

Choose a reason for hiding this comment

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

@theShinigami I don't think I've communicated this idea well. The purpose here is to have a value that enables and disables whether we're using the sql panel. So by default it should track all SQL. However, when we get to the toolbar's own processing, it should be disabled. That way the toolbar isn't monitoring itself. Does that make sense?

Copy link
Author

Choose a reason for hiding this comment

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

@tim-schilling I'm not quite getting the idea, could you elaborate a little more? 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Given this context var:

is_recording = contextvars.ContextVar(
    "debug-toolbar-is-recording", default=True
)

Think of the is_recording as a global variable that controls whether a panel actually records the activity of the django project. When it's True, the SQL panel should record SQL queries. When it's False, the SQL panel shouldn't record SQL queries.

This comes into play for this issue because we want to disable the SQL panel when the toolbar is processing the request, but not when everything else is.

So we need to identify when to set and when to unset it (somewhere in the middleware most likely, we may run into some weirdness with the async flow), then lastly update the SQL panel to avoid tracking when it's not supposed to.

The places that you'll likely want to update are:

This relies on the idea that record_stats (what uses the HistoryEntry model) will only ever be called from generate_stats. I think that will work...

Copy link
Member

@tim-schilling tim-schilling Sep 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I feel like I may be missing something here around enable_instrumentation and disable_instrumentation. This global variable shouldn't be necessary. Sorry, my thoughts aren't fully baked yet


DDT_MODELS = {
m._meta.db_table
for m in apps.get_app_config("debug_toolbar").get_models()
if m._meta.app_label == "debug_toolbar"
}


class SQLQueryTriggered(Exception):
"""Thrown when template panel triggers a query"""
Expand Down Expand Up @@ -224,6 +236,12 @@ def _record(self, method, sql, params):
}
)

# Skip recording if query includes DDT models.
if not allow_ddt_models_tracking.get() and any(
table in sql for table in DDT_MODELS
):
return
Copy link
Member

Choose a reason for hiding this comment

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

We definitely shouldn't be returning inside a finally block.

Starting in python 3.14 the compiler emits a SyntaxWarning when seeing this construct:
https://docs.python.org/3.14/reference/compound_stmts.html#finally

Copy link
Author

@theShinigami theShinigami Sep 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I've noticed it when ruff complained 😆, I'm making changes. Thank you for the reference. 🙂


# We keep `sql` to maintain backwards compatibility
self.logger.record(**kwargs)

Expand Down
73 changes: 73 additions & 0 deletions tests/panels/test_sql.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from django.test.utils import override_settings

import debug_toolbar.panels.sql.tracking as sql_tracking
from debug_toolbar.models import HistoryEntry
from debug_toolbar.panels.sql import SQLPanel

try:
Expand All @@ -33,6 +34,20 @@ def sql_call(*, use_iterator=False):
return list(qs)


def sql_call_ddt(*, use_iterator=False):
Copy link
Member

Choose a reason for hiding this comment

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

Could we add a doc string here to explain what this function is for please? I think that'll help us in the future when reviewing code.

Copy link
Member

Choose a reason for hiding this comment

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

Ack, posted too soon. I think a doc string on these two helper functions and the four tests would be fantastic.

qs = HistoryEntry.objects.all()
if use_iterator:
qs = qs.iterator()
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this isn't used, so we should be able to remove it and the one from async_sql_call_ddt

return list(qs)


async def async_sql_call_ddt(*, use_iterator=False):
qs = HistoryEntry.objects.all()
if use_iterator:
qs = qs.iterator()
return await sync_to_async(list)(qs)


async def async_sql_call(*, use_iterator=False):
qs = User.objects.all()
if use_iterator:
Expand Down Expand Up @@ -104,6 +119,64 @@ async def test_recording_concurrent_async(self):
# ensure the stacktrace is populated
self.assertTrue(len(query["stacktrace"]) > 0)

def test_ddt_models_tracking(self):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def test_ddt_models_tracking(self):
def test_ddt_models_tracked(self):

nit: Small naming improvement (should be used on the other similar test too)

self.assertEqual(len(self.panel._queries), 0)

# enable ddt tracking
sql_tracking.allow_ddt_models_tracking.set(True)

sql_call_ddt()

# ensure query was logged
self.assertEqual(len(self.panel._queries), 1)
query = self.panel._queries[0]
self.assertEqual(query["alias"], "default")
self.assertTrue("sql" in query)
self.assertTrue("duration" in query)
self.assertTrue("stacktrace" in query)
Copy link
Member

Choose a reason for hiding this comment

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

I think we may be better off replacing this with a check on the models' table in the sql query rather than these assertions.


# ensure the stacktrace is populated
self.assertTrue(len(self.panel._queries), 0)

async def test_ddt_models_tracking_async(self):
self.assertEqual(len(self.panel._queries), 0)

# enable ddt tracking
sql_tracking.allow_ddt_models_tracking.set(True)

await async_sql_call_ddt()

# ensure query was logged
self.assertEqual(len(self.panel._queries), 1)
query = self.panel._queries[0]
self.assertEqual(query["alias"], "default")
self.assertTrue("sql" in query)
self.assertTrue("duration" in query)
self.assertTrue("stacktrace" in query)

# ensure the stacktrace is populated
self.assertTrue(len(self.panel._queries), 0)

def test_ddt_models_untracking(self):
Copy link
Member

@tim-schilling tim-schilling Oct 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
def test_ddt_models_untracking(self):
def test_ddt_models_not_tracked(self):

nit: Small naming improvement (should be used on the other similar test too)

self.assertEqual(len(self.panel._queries), 0)

# disable ddt tracking
sql_tracking.allow_ddt_models_tracking.set(False)

sql_call_ddt()

self.assertEqual(len(self.panel._queries), 0)

async def test_ddt_models_untracking_async(self):
self.assertEqual(len(self.panel._queries), 0)

# enable ddt tracking
sql_tracking.allow_ddt_models_tracking.set(False)

await async_sql_call_ddt()

self.assertEqual(len(self.panel._queries), 0)

@unittest.skipUnless(
connection.vendor == "postgresql", "Test valid only on PostgreSQL"
)
Expand Down