Add support for enabling/disabling SQLPanel tracking of DDT model queries #2211
Conversation
debug_toolbar/panels/sql/tracking.py
Outdated
| if not allow_ddt_models_tracking.get() and any( | ||
| table in sql for table in DDT_MODELS | ||
| ): | ||
| return |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I've noticed it when ruff complained 😆, I'm making changes. Thank you for the reference. 🙂
theShinigami
changed the title
debug_toolbar/panels/sql/tracking.py
Outdated
| allow_ddt_models_tracking = contextvars.ContextVar( | ||
| "debug-toolbar-allow-ddt-models", default=False | ||
| ) |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@tim-schilling I'm not quite getting the idea, could you elaborate a little more? 🙂
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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
tests/panels/test_sql.py
Outdated
| if use_iterator: | ||
| qs = qs.iterator() |
There was a problem hiding this comment.
Seems like this isn't used, so we should be able to remove it and the one from async_sql_call_ddt
tests/panels/test_sql.py
Outdated
| return list(qs) | ||
|
|
||
|
|
||
| def sql_call_ddt(*, use_iterator=False): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ack, posted too soon. I think a doc string on these two helper functions and the four tests would be fantastic.
tests/panels/test_sql.py
Outdated
| 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) |
There was a problem hiding this comment.
I think we may be better off replacing this with a check on the models' table in the sql query rather than these assertions.
tests/panels/test_sql.py
Outdated
| # ensure the stacktrace is populated | ||
| self.assertTrue(len(query["stacktrace"]) > 0) | ||
|
|
||
| def test_ddt_models_untracking(self): |
There was a problem hiding this comment.
| 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)
tests/panels/test_sql.py
Outdated
| self.assertTrue(len(query["stacktrace"]) > 0) | ||
|
|
||
| @override_settings(DEBUG_TOOLBAR_CONFIG={"TRACK_DDT_MODELS": True}) | ||
| def test_ddt_models_tracking(self): |
There was a problem hiding this comment.
| 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)
|
I think I can live with the SQL search. I don't like it because anything can match on it, but I'm not sure how to exactly improve it. @matthiask any thoughts? |
|
To clarify, a query such as `select * from my_other_table WHERE my_column = 'debug_toolbar.historyentry'; would match. For any possible query that a Django application is running, we're looking for a specific string in it. There's a possibility of false positives (matches that we don't actually want to match on) |
Description
Added a new context variable
allow_ddt_models_trackingto control whetherSQLPaneltracks DDT queries.allow_ddt_models_trackingcontextvar, defaulting toFalseSQLPanelnow checks this variable to decide if DDT models queries should be tracked.Fixes #2165
Checklist:
docs/changes.rst.