-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Download prof files #2286
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?
Download prof files #2286
Changes from 23 commits
48ac250
49f29eb
7ec7f25
fe4e305
5db558d
74ddf5e
5bbb211
a285d82
2bd5cfe
e698cde
66e2d50
3896fe3
09f2ba0
9829c58
cfc244b
df258a2
efbd2b9
9cbdf04
05a505c
34dde67
bdb6bf5
776e4ba
25fa939
f1c86c2
d03fec3
7235ab8
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 |
|---|---|---|
| @@ -1,14 +1,26 @@ | ||
| import cProfile | ||
| import logging | ||
| import os | ||
| from colorsys import hsv_to_rgb | ||
| from pstats import Stats | ||
|
|
||
| from django import forms | ||
| from django.conf import settings | ||
| from django.contrib.auth.decorators import login_not_required | ||
| from django.http import Http404, HttpResponse, HttpResponseBadRequest | ||
| from django.urls import path | ||
| from django.utils import timezone | ||
| from django.utils.html import format_html | ||
| from django.utils.translation import gettext_lazy as _ | ||
| from django.views.decorators.http import require_GET | ||
|
|
||
| from debug_toolbar import settings as dt_settings | ||
| from debug_toolbar.decorators import render_with_toolbar_language, require_show_toolbar | ||
| from debug_toolbar.panels import Panel | ||
| from debug_toolbar.toolbar import DebugToolbar | ||
| from debug_toolbar.utils import get_view_path_metadata | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class FunctionCall: | ||
|
|
@@ -131,22 +143,69 @@ def cumtime_per_call(self): | |
| def indent(self): | ||
| return 16 * self.depth | ||
|
|
||
| def primitive_count(self): | ||
| return self.stats[0] | ||
|
|
||
| def _func_key(self): | ||
| # Mirrors pstats.func_std_string to produce the plain-text identifier | ||
| # used in downloaded .prof files. | ||
| # https://docs.python.org/3/library/profile.html#pstats.Stats | ||
| if self.func[:2] == ("~", 0): | ||
| name = self.func[2] | ||
| if name.startswith("<") and name.endswith(">"): | ||
| return f"{{{name[1:-1]}}}" | ||
| return name | ||
| return "%s:%d(%s)" % self.func | ||
|
|
||
| def serialize(self): | ||
| return { | ||
| "has_subfuncs": self.has_subfuncs, | ||
| "id": self.id, | ||
| "parent_ids": self.parent_ids, | ||
| "is_project_func": self.is_project_func(), | ||
| "indent": self.indent(), | ||
| "func_key": self._func_key(), | ||
| "func_std_string": self.func_std_string(), | ||
| "cumtime": self.cumtime(), | ||
| "cumtime_per_call": self.cumtime_per_call(), | ||
| "tottime": self.tottime(), | ||
| "tottime_per_call": self.tottime_per_call(), | ||
| "count": self.count(), | ||
| "primitive_count": self.primitive_count(), | ||
| } | ||
|
|
||
|
|
||
| def _print_stats_from_function_calls(func_list: list[FunctionCall]) -> str: | ||
| """ | ||
| Create a string that is matches what pstats.Stats.print_stats would | ||
| print out. | ||
| """ | ||
| # Column layout matches pstats output: ncalls right-justified in 9 | ||
| # chars, each time value formatted as f8 ("%8.3f"). The 3-space indent | ||
| # on the header and ordering lines is part of the pstats format. | ||
| # https://docs.python.org/3/library/profile.html#pstats.Stats.print_stats | ||
|
Comment on lines
+183
to
+186
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. I don't want to insinuate anything, but the comments read like AI code. Maybe we don't fan the fire and limit comments to only what can't be easily understood from the code itself.
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. I called out that I used Claude in the PR. I think in this case the comments help. There's syntax used in this function that isn't typical in my opinion, so having it explained helps.
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. Yes, sorry. I only read that comment later. Hm… depends. I think tests for "how this should look" actually help me personally more than the comments. But better too much documentation that too little 👍 At least my spidysense for AI still works. 🤠 |
||
| lines = [" Ordered by: cumulative time", ""] | ||
| lines.append( | ||
| " ncalls tottime percall cumtime percall filename:lineno(function)" | ||
| ) | ||
| for func in func_list: | ||
| nc = func["count"] | ||
| cc = func["primitive_count"] | ||
| # pstats shows nc/cc when they differ (recursive calls inflate nc) | ||
| ncalls_str = f"{nc}/{cc}" if nc != cc else str(nc) | ||
| # pstats prints 8 spaces instead of a percall value when the | ||
| # divisor is zero, preserving column alignment | ||
| tt_per = f"{func['tottime_per_call']:8.3f}" if nc else " " * 8 | ||
| ct_per = f"{func['cumtime_per_call']:8.3f}" if cc else " " * 8 | ||
| lines.append( | ||
| f"{ncalls_str:>9} {func['tottime']:8.3f} {tt_per}" | ||
| f" {func['cumtime']:8.3f} {ct_per} {func['func_key']}" | ||
| ) | ||
| # pstats ends output with two blank lines | ||
| lines.extend(["", ""]) | ||
| return "\n".join(lines) | ||
|
|
||
|
|
||
| class ProfilingPanel(Panel): | ||
| """ | ||
| Panel that displays profiling information. | ||
|
|
@@ -184,7 +243,6 @@ def generate_stats(self, request, response): | |
| self.stats.calc_callees() | ||
|
|
||
| root_func = cProfile.label(super().process_request.__code__) | ||
|
|
||
| if root_func in self.stats.stats: | ||
| root = FunctionCall(self.stats, root_func, depth=0) | ||
| func_list = [] | ||
|
|
@@ -197,4 +255,65 @@ def generate_stats(self, request, response): | |
| dt_settings.get_config()["PROFILER_MAX_DEPTH"], | ||
| cum_time_threshold, | ||
| ) | ||
| self.record_stats({"func_list": [func.serialize() for func in func_list]}) | ||
| try: | ||
| url_name = get_view_path_metadata(request).url_name | ||
| except Http404: | ||
| url_name = _("<unavailable>") | ||
| self.record_stats( | ||
| { | ||
| "func_list": [func.serialize() for func in func_list], | ||
| "profile_name": f"{url_name}-{timezone.now().isoformat()}", | ||
| } | ||
| ) | ||
|
|
||
| def get_stats(self): | ||
| """ | ||
| Access data stored by the panel. Returns a :class:`dict`. | ||
| """ | ||
| stats = super().get_stats() | ||
| # This isn't a stat, but is used in the context in the | ||
| # ProfilingPanel.content property when rendering the template. | ||
| stats["profiling_download_form"] = ProfilingDownloadForm( | ||
| initial={"request_id": self.toolbar.request_id} | ||
| ) | ||
| return stats | ||
|
|
||
| @classmethod | ||
| def get_urls(cls): | ||
| return [ | ||
| path("profiling_download/", profiling_download, name="profiling_download") | ||
| ] | ||
|
|
||
|
|
||
| class ProfilingDownloadForm(forms.Form): | ||
| """ | ||
| Validate params | ||
|
|
||
| request_id: The key for the store instance to be fetched. | ||
| """ | ||
|
|
||
| request_id = forms.CharField(widget=forms.HiddenInput()) | ||
|
|
||
|
|
||
| @require_GET | ||
| @login_not_required | ||
| @require_show_toolbar | ||
| @render_with_toolbar_language | ||
| def profiling_download(request): | ||
| form = ProfilingDownloadForm(request.GET) | ||
|
|
||
| if form.is_valid(): | ||
| request_id: str = form.cleaned_data["request_id"] | ||
| toolbar: DebugToolbar | None = DebugToolbar.fetch(request_id) | ||
| if toolbar is None: | ||
| return HttpResponseBadRequest("Request is no longer available.") | ||
| panel = toolbar.get_panel_by_id(ProfilingPanel.panel_id) | ||
| panel_stats = panel.get_stats() | ||
| if "func_list" not in panel_stats: | ||
| return HttpResponseBadRequest("No profiling data exists for this request.") | ||
| content = _print_stats_from_function_calls(panel_stats["func_list"]) | ||
| response = HttpResponse(content, content_type="text/plain") | ||
|
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. The |
||
| profile_name = panel_stats.get("profile_name") or f"request-{request_id}" | ||
| response["Content-Disposition"] = f'attachment; filename="{profile_name}.prof"' | ||
| return response | ||
| return HttpResponseBadRequest(f"Form errors: {form.errors}") | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,10 @@ | ||
| {% load i18n %} | ||
|
|
||
| <form method="get" action="{% url 'djdt:profiling_download' %}" target="_blank"> | ||
| {{ profiling_download_form.as_div }} | ||
| <button>Download</button> | ||
| </form> | ||
|
Comment on lines
+3
to
+6
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. Why use a form and a button for a GET request? Why not use an a-tag with a
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. I did it for consistency with the history panel which was originally done because the form used to inherit from SignedDataForm. I do like that it enforces the data validation more strictly.
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. Which currently is only an ID, right? I don't really care much, but without knowing the history, I just looked a little odd.
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. Yup, yup. That's correct. |
||
|
|
||
| <table> | ||
| <thead> | ||
| <tr> | ||
|
|
@@ -13,22 +19,22 @@ | |
| <tbody> | ||
| {% for call in func_list %} | ||
| <tr class="djdt-profile-row {% if call.is_project_func %}djdt-highlighted {% endif %} {% for parent_id in call.parent_ids %} djToggleDetails_{{ parent_id }}{% endfor %}" id="profilingMain_{{ call.id }}"> | ||
| <td> | ||
| <div data-djdt-styles="paddingLeft:{{ call.indent }}px"> | ||
| {% if call.has_subfuncs %} | ||
| <td> | ||
| <div data-djdt-styles="paddingLeft:{{ call.indent }}px"> | ||
| {% if call.has_subfuncs %} | ||
| <button type="button" class="djProfileToggleDetails djToggleSwitch" data-toggle-name="profilingMain" data-toggle-id="{{ call.id }}">-</button> | ||
| {% else %} | ||
| <span class="djNoToggleSwitch"></span> | ||
| {% endif %} | ||
| <span class="djdt-stack">{{ call.func_std_string|safe }}</span> | ||
| </div> | ||
| </td> | ||
| <td>{{ call.cumtime|floatformat:3 }}</td> | ||
| <td>{{ call.cumtime_per_call|floatformat:3 }}</td> | ||
| <td>{{ call.tottime|floatformat:3 }}</td> | ||
| <td>{{ call.tottime_per_call|floatformat:3 }}</td> | ||
| <td>{{ call.count }}</td> | ||
| </tr> | ||
| {% else %} | ||
| <span class="djNoToggleSwitch"></span> | ||
| {% endif %} | ||
| <span class="djdt-stack">{{ call.func_std_string|safe }}</span> | ||
| </div> | ||
| </td> | ||
| <td>{{ call.cumtime|floatformat:3 }}</td> | ||
| <td>{{ call.cumtime_per_call|floatformat:3 }}</td> | ||
| <td>{{ call.tottime|floatformat:3 }}</td> | ||
| <td>{{ call.tottime_per_call|floatformat:3 }}</td> | ||
| <td>{{ call.count }}</td> | ||
| </tr> | ||
| {% endfor %} | ||
| </tbody> | ||
| </table> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,4 +2,5 @@ | |
| from debug_toolbar.toolbar import DebugToolbar | ||
|
|
||
| app_name = APP_NAME | ||
|
|
||
|
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. Unrealted change. |
||
| urlpatterns = DebugToolbar.get_urls() | ||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,14 +6,17 @@ | |||||||
| import sys | ||||||||
| import warnings | ||||||||
| from collections.abc import Sequence | ||||||||
| from dataclasses import dataclass | ||||||||
| from pprint import PrettyPrinter, pformat | ||||||||
| from typing import Any | ||||||||
|
|
||||||||
| from asgiref.local import Local | ||||||||
| from django.http import QueryDict | ||||||||
| from django.http import HttpRequest, QueryDict | ||||||||
| from django.template import Node | ||||||||
| from django.urls import resolve | ||||||||
| from django.utils.html import format_html | ||||||||
| from django.utils.safestring import SafeString, mark_safe | ||||||||
| from django.utils.translation import gettext_lazy as _ | ||||||||
| from django.views.debug import get_default_exception_reporter_filter | ||||||||
|
|
||||||||
| from debug_toolbar import _compat as compat, _stubs as stubs, settings as dt_settings | ||||||||
|
|
@@ -415,3 +418,34 @@ def get_csp_nonce(request) -> str | None: | |||||||
| return csp_nonce | ||||||||
| # Django's built-in CSP support uses get_nonce(request) | ||||||||
| return compat.get_nonce(request) | ||||||||
|
|
||||||||
|
|
||||||||
| @dataclass | ||||||||
| class ViewPathMetadata: | ||||||||
|
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. Dataclasses are great class factories but not great datastructure primitives. NamedTuples are based on C-structs and much more suitable for immutable data structures.
Suggested change
|
||||||||
| view_func: str | ||||||||
| view_args: tuple[Any] | ||||||||
| view_kwargs: dict[str, Any] | ||||||||
| url_name: str | ||||||||
|
|
||||||||
|
|
||||||||
| def get_view_path_metadata(request: HttpRequest) -> ViewPathMetadata: | ||||||||
|
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. This could be a
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. Good call |
||||||||
| """ | ||||||||
| Retrieve the routing metadata for a request being routed through | ||||||||
| a particular path to the view. | ||||||||
| """ | ||||||||
| match = resolve(request.path_info) | ||||||||
| func, args, kwargs = match | ||||||||
|
|
||||||||
| if getattr(match, "url_name", False): | ||||||||
| url_name = match.url_name | ||||||||
| if match.namespaces: | ||||||||
| url_name = ":".join([*match.namespaces, url_name]) | ||||||||
| else: | ||||||||
| url_name = _("<unavailable>") | ||||||||
|
|
||||||||
| return ViewPathMetadata( | ||||||||
| view_func=get_name_from_obj(func), | ||||||||
|
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. If you want to stick with a dataclass, this should probably be part of post_init. |
||||||||
| view_args=args, | ||||||||
| view_kwargs=kwargs, | ||||||||
| url_name=url_name, | ||||||||
| ) | ||||||||
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.
I'm not sure about this. I could use a second opinion on whether we want to go this approach or not.
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.
pstats.Statscan be printed to any stream-like object. So you can get aio.StringIOand simply callprint_stats. I don't believe the is a need to recreate the funcitonality, right?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.
The challenge is the panel doesn't show the same content.
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.
The profiling panel as a whole needs a bit of a rework. The threshold concept just doesn't seem to work properly. It seems like we should lean more into the actual APIs of the standard library.
Getting this all resolved is on my todo list, but I'd really like to avoid it for this feature.