-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Track and display middleware time in the toolbar #2049
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
68ca369
0c94a14
6714742
ebb7571
e029174
cb45e18
6715a21
df7fc44
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,6 +47,10 @@ def content(self): | |
| (_("System CPU time"), _("%(stime)0.3f ms") % stats), | ||
| (_("Total CPU time"), _("%(total)0.3f ms") % stats), | ||
| (_("Elapsed time"), _("%(total_time)0.3f ms") % stats), | ||
| ( | ||
| _("Toolbar time"), | ||
| _("%(toolbar_time)0.3f ms") % stats, | ||
| ), | ||
| ( | ||
| _("Context switches"), | ||
| _("%(vcsw)d voluntary, %(ivcsw)d involuntary") % stats, | ||
|
|
@@ -109,6 +113,9 @@ def generate_stats(self, request, response): | |
| # stats['urss'] = self._end_rusage.ru_idrss | ||
| # stats['usrss'] = self._end_rusage.ru_isrss | ||
|
|
||
| if hasattr(self, "_toolbar_start_time"): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. EAFP: Maybe consider catching the AttributeError instead of the default if-branch. |
||
| stats["toolbar_time"] = (perf_counter() - self._toolbar_start_time) * 1000 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider storing the duration with an explicit type using |
||
|
|
||
| self.record_stats(stats) | ||
|
|
||
| def generate_server_timing(self, request, response): | ||
|
|
@@ -120,7 +127,16 @@ def generate_server_timing(self, request, response): | |
| self.record_server_timing( | ||
| "total_time", "Elapsed time", stats.get("total_time", 0) | ||
| ) | ||
| self.record_server_timing( | ||
| "toolbar_time", "Toolbar time", stats.get("toolbar_time", 0) | ||
| ) | ||
|
|
||
| @staticmethod | ||
| def _elapsed_ru(start, end, name): | ||
| return end.get(name) - start.get(name) | ||
|
|
||
| def enable_instrumentation(self): | ||
| self._toolbar_start_time = perf_counter() | ||
|
|
||
| async def aenable_instrumentation(self): | ||
| self._toolbar_start_time = perf_counter() | ||
|
Comment on lines
+138
to
+142
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both are synchronous, and instrumentation usually lives in the CPU and has no IO. Thus, I don't think asyncIO is all that helpful even in the future. |
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -73,7 +73,15 @@ def enabled_panels(self) -> list[Panel]: | |||||||||||||||
| """ | ||||||||||||||||
| Get a list of panels enabled for the current request. | ||||||||||||||||
| """ | ||||||||||||||||
| return [panel for panel in self._panels.values() if panel.enabled] | ||||||||||||||||
| panels = [panel for panel in self._panels.values() if panel.enabled] | ||||||||||||||||
| # Ensure TimerPanel is first in order to measure the full time of the toolbar's processing. | ||||||||||||||||
| timer_panel = next( | ||||||||||||||||
| (panel for panel in panels if panel.panel_id == "TimerPanel"), None | ||||||||||||||||
| ) | ||||||||||||||||
|
Comment on lines
+76
to
+80
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are looping over the panels twice, O(n)=2*n. I think with a little rearrangement, this can be done in a single loop. |
||||||||||||||||
| if timer_panel: | ||||||||||||||||
|
Comment on lines
+78
to
+81
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great place for a walrus:
Suggested change
|
||||||||||||||||
| panels.remove(timer_panel) | ||||||||||||||||
| panels.insert(0, timer_panel) | ||||||||||||||||
|
Comment on lines
+82
to
+83
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe replace the item instead of the stack operation.
Suggested change
|
||||||||||||||||
| return panels | ||||||||||||||||
|
|
||||||||||||||||
| @property | ||||||||||||||||
| def csp_nonce(self) -> str | None: | ||||||||||||||||
|
|
||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -290,6 +290,10 @@ def test_concurrent_async_sql_page(self): | |||||
| len(response.toolbar.get_panel_by_id("SQLPanel").get_stats()["queries"]), 2 | ||||||
| ) | ||||||
|
|
||||||
| def test_timer_panel_first(self): | ||||||
| toolbar = DebugToolbar(self.request, self.get_response) | ||||||
| self.assertEqual(toolbar.enabled_panels[0].panel_id, "TimerPanel") | ||||||
|
|
||||||
|
|
||||||
| @override_settings(DEBUG=True) | ||||||
| class DebugToolbarIntegrationTestCase(IntegrationTestCase): | ||||||
|
|
@@ -620,6 +624,7 @@ def test_server_timing_headers(self): | |||||
| r'TimerPanel_stime;dur=(\d)*(\.(\d)*)?;desc="System CPU time", ', | ||||||
| r'TimerPanel_total;dur=(\d)*(\.(\d)*)?;desc="Total CPU time", ', | ||||||
| r'TimerPanel_total_time;dur=(\d)*(\.(\d)*)?;desc="Elapsed time", ', | ||||||
| r'TimerPanel_toolbar_time;dur=(\d)*(\.(\d)*)?;desc="Toolbar time", ', | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Toolbar is ambiguous outside this repo. Maybe stick with DebugToolbar, like it's currently phrased in most places.
Suggested change
|
||||||
| r'SQLPanel_sql_time;dur=(\d)*(\.(\d)*)?;desc="SQL 1 queries", ', | ||||||
| r'CachePanel_total_time;dur=0;desc="Cache 0 Calls"', | ||||||
| ] | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Toolbar is ambiguous outside this repo. Maybe stick with DebugToolbar, like it's currently phrased in most places.