Add option to dump profiler stats #204
Conversation
tox.ini
Outdated
| @@ -4,6 +4,7 @@ envlist = py27,py36,py37,py38,stylecheck | |||
| [testenv] | |||
| deps = | |||
| pytest | |||
| flask<2.3.0 # For "before_first_request" | |||
There was a problem hiding this comment.
Can you further clarify why this is needed?
We can't be pinning Flask to an old version unless it's a super temp pin until Flask drops a bugfix release... but Flask is now on 3.0.0 so methinks we'd want to test with that...
There was a problem hiding this comment.
test/basic_app.py still uses the before_first_request decorator which was removed in Flask 2.3.0.
Additional changes would be necessary to make the extension compatible with 3.0.0. If desired, I can prepare an additional PR for that.
There was a problem hiding this comment.
This is no longer part of the PR now that a few other PR's were merged re: compatibility.
Dosenpfand
force-pushed
the
profiler-dump-stats
branch
3 times, most recently
from
45c8f39
to
c3f731c
Compare
Done. |
There was a problem hiding this comment.
After the updates that @jeffwidman suggested, I think this looks good. Thanks for submitting it!
| @@ -88,7 +92,14 @@ def process_response(self, request, response): | |||
|
|
|||
| self.stats = stats | |||
| self.function_calls = function_calls | |||
| # destroy the profiler just in case | |||
There was a problem hiding this comment.
Hmm, this comment seems semi-interesting, although I don't see an explicit del call here. Does it somehow just fall out of scope?
Just wondering if this is a contextual comment we should continue pulling forward in the codebase or if it's outdated.
There was a problem hiding this comment.
I thought it's outdated but if you think otherwise I can restore it.
jeffwidman
force-pushed
the
profiler-dump-stats
branch
from
c3f731c
to
a0de8b6
Compare
|
I was able to resolve the merge conflict locally, but am running into permission errors when trying to push updates to the PR or to push updates that merge the PR into master. But if we can get past that, I think this one is ready to merge. |
|
@Dosenpfand can you rebase? And also address my last question? |
Dosenpfand
force-pushed
the
profiler-dump-stats
branch
from
a0de8b6
to
1aa7509
Compare
|
@jeffwidman Done. Sorry for my late reaction. |
|
It looks like it's all good except for one style check that is failing. Once that's fixed I think it is ready to merge. |
Head branch was pushed to by a user without write access
|
@macnewbold |
macnewbold
force-pushed
the
profiler-dump-stats
branch
from
76e2eaa
to
f18bcc7
Compare
Add a config option
DEBUG_TB_PROFILER_DUMP_FILENAMEto dump profile stats to a file, so they can be further evaluated, e.g. withflameprof.