Download prof files#2286
Conversation
for more information, see https://pre-commit.ci
…-toolbar into download-prof-files
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Pull request overview
This PR adds the ability to download profiling data as .prof files, addressing issue #1798. The feature is disabled by default and requires configuring the PROFILER_PROFILE_ROOT setting to specify where profile files should be saved.
Key changes:
- Added
PROFILER_PROFILE_ROOTconfiguration setting to control where profile files are saved - Implemented profile file generation in the profiling panel with signed filenames for security
- Created a new download view with URL endpoint to serve the profile files
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
debug_toolbar/settings.py |
Added PROFILER_PROFILE_ROOT default setting (None) |
debug_toolbar/panels/profiling.py |
Modified to generate and save .prof files when PROFILER_PROFILE_ROOT is configured; signs filename for secure download |
debug_toolbar/views.py |
Added download_prof_file view to handle file downloads with signature validation |
debug_toolbar/toolbar.py |
Registered new download URL endpoint |
debug_toolbar/urls.py |
Minor whitespace addition |
debug_toolbar/templates/debug_toolbar/panels/profiling.html |
Added download link UI and reformatted indentation |
debug_toolbar/static/debug_toolbar/css/toolbar.css |
Added styling for download control |
tests/panels/test_profiling.py |
Added comprehensive tests for profile file generation and download functionality |
docs/configuration.rst |
Documented the new PROFILER_PROFILE_ROOT setting |
docs/changes.rst |
Added changelog entry for the new feature |
debug_toolbar/panels/sql/tracking.py |
Unrelated changes to handle executemany parameters in SQL tracking |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hello everyone sorry for not finishing this. I have been a bit caught up working on other OSS projects . I promise to push new commits this weekend to fix the reviews co pilot left |
…-toolbar into download-prof-files
e690762 to
09f2ba0
Compare
tim-schilling
left a comment
There was a problem hiding this comment.
Thank you for continuing to work on this 🚀. I've got some questions about the direction of this. If you're confused, or think I may be wrong, please let me know. I'm happy to hop on a call and chat about this too!
for more information, see https://pre-commit.ci
| } | ||
|
|
||
|
|
||
| def _print_stats_from_function_calls(func_list: list[FunctionCall]) -> str: |
There was a problem hiding this comment.
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.
pstats.Stats can be printed to any stream-like object. So you can get a io.StringIO and simply call print_stats. I don't believe the is a need to recreate the funcitonality, right?
There was a problem hiding this comment.
The challenge is the panel doesn't show the same content.
There was a problem hiding this comment.
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.
tim-schilling
left a comment
There was a problem hiding this comment.
So I'm in favor of this, but it's just enough to get it merged. I would appreciate another opinion on this approach. I worry it's overly brittle. I suspect we'll end up redoing a lot of this as a part of integrating Yappi and fixing the profiling panel in general.
I had help from Claude to generate the Stats.print_stats reproduction.
codingjoe
left a comment
There was a problem hiding this comment.
Hi there,
The second opinion arrived; I hope you don't mind.
I like the idea. However, the current implementation adds a pretty high maintenance burden onto this package. If you can make this work with a buffer object, like I described below, the scales might tip for me.
Cheers!
Joe
| } | ||
|
|
||
|
|
||
| def _print_stats_from_function_calls(func_list: list[FunctionCall]) -> str: |
There was a problem hiding this comment.
pstats.Stats can be printed to any stream-like object. So you can get a io.StringIO and simply call print_stats. I don't believe the is a need to recreate the funcitonality, right?
| <form method="get" action="{% url 'djdt:profiling_download' %}" target="_blank"> | ||
| {{ profiling_download_form.as_div }} | ||
| <button>Download</button> | ||
| </form> |
There was a problem hiding this comment.
Why use a form and a button for a GET request? Why not use an a-tag with a download attribute.
There was a problem hiding this comment.
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.
| from debug_toolbar.toolbar import DebugToolbar | ||
|
|
||
| app_name = APP_NAME | ||
|
|
| @dataclass | ||
| class ViewPathMetadata: |
There was a problem hiding this comment.
Dataclasses are great class factories but not great datastructure primitives. NamedTuples are based on C-structs and much more suitable for immutable data structures.
| @dataclass | |
| class ViewPathMetadata: | |
| class ViewPathMetadata(typing.NamedTuple): |
| url_name: str | ||
|
|
||
|
|
||
| def get_view_path_metadata(request: HttpRequest) -> ViewPathMetadata: |
There was a problem hiding this comment.
This could be a from_request factory class method on the actual ViewPathMetadata class.
| url_name = _("<unavailable>") | ||
|
|
||
| return ViewPathMetadata( | ||
| view_func=get_name_from_obj(func), |
There was a problem hiding this comment.
If you want to stick with a dataclass, this should probably be part of post_init.
| 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") |
There was a problem hiding this comment.
The Content-Disposition header would enable you to propose a filename to the browser.
| # 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Description
This pr is based on the work of @andoriyaprashant which he made to fix #1798. I am continuing his work to get it over the finish line. His pr is this #2146 .
Fixes #1798
Checklist:
docs/changes.rst.