jazzband / django-silk

Silky smooth profiling for Django
MIT License
4.42k stars 333 forks source link

Gracefully error out when there are concurrent profilers #692

Closed albertyw closed 9 months ago

albertyw commented 9 months ago

Fixes #682 Unblocks #686

codecov[bot] commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (23ff43b) 86.65% compared to head (6537b6e) 86.67%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #692 +/- ## ========================================== + Coverage 86.65% 86.67% +0.02% ========================================== Files 52 52 Lines 2113 2117 +4 ========================================== + Hits 1831 1835 +4 Misses 282 282 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

robinchow commented 8 months ago

I'd like to point out while this stop silk from interfering with processing a request, it also ends up not profiling that particular request. Prior to Python 3.11, the effect was similar but different (it would disable/stop the first profiler, and then start this current one). Neither outcome seems great, especially since this problem will happen when Django is processing multiple long-running requests at the same time (I've made a minimal repo here to highlight this issue)

I think at a minimum, the documentation should tell users that this issue exists. In terms of a proper fix, I'm not sure at the moment if that's possible without using another profiler that properly supports multithreaded applications (such as https://github.com/sumerc/yappi)

50-Course commented 8 months ago

Thank you for pointing out this oversight, @robinchow. I'd take a look into yappi

albertyw commented 8 months ago

Prior to Python 3.11, the effect was similar but different (it would disable/stop the first profiler, and then start this current one)

3.11 or 3.12? This code gives an error in python 3.12 but no error in 3.11:

import cProfile
p1 = cProfile.Profile()
p1.enable()
p2 = cProfile.Profile()
p2.enable()
robinchow commented 8 months ago

@albertyw ah yes, apologies, prior to Python 3.12.