-
Notifications
You must be signed in to change notification settings - Fork 27
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
using pyinstrument to log traceback of slow callbacks #2341
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2341 +/- ##
======================================
Coverage 72.0% 72.1%
======================================
Files 509 509
Lines 20022 20025 +3
Branches 1969 1969
======================================
+ Hits 14431 14445 +14
+ Misses 5104 5097 -7
+ Partials 487 483 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
causing test_unhealthy_app_with_slow_callbacks to fail
raising /slow to 2.0 seocnds
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 like statistical profilers. I guess you checked joerick/pyinstrument#83?
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 indentation to check maybe?
packages/service-library/src/servicelib/monitor_slow_callbacks.py
Outdated
Show resolved
Hide resolved
@@ -22,6 +22,8 @@ sqlalchemy | |||
psycopg2-binary | |||
pyyaml | |||
|
|||
# used for monitoring of slow callbacks | |||
pyinstrument |
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.
Since you add a new dependency in a library, you should compile the requirements of the services it includes
- search which services are affected by
grep --include=\*.in -nrw -e 'packages/service-library/requirements/_base.in'
- compile upgrade each service
cd services/service/requirements
make touch
make reqs upgrade=pyinstrument
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.
Nice one, will do this.
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.
Ran this on all packages and no other packages require extra dependencies.
There are only some dependencies which are trying to be updated, like pytest-coverage etc... Those are out of the scope of this PR, will ignore them.
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.
did you use upgrade=pyinstrument
option?? That should not happen!
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 always forget about that option, usually I end up checking stuff manually.
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.
@GitHK Please do not forget to upgrade the requirements of the services!
Right now it works because the servicelib setup
is installing pyinstrument but the frozen requirements do not show all the dependencies
packages/service-library/src/servicelib/monitor_slow_callbacks.py
Outdated
Show resolved
Hide resolved
packages/service-library/src/servicelib/monitor_slow_callbacks.py
Outdated
Show resolved
Hide resolved
Sadly enough I've checked this. If there would be a big performance downfall I would remove the library and revert back to the previous implementation. |
What do these changes do?
During the analysis of logs coming from production there were lots of entries similar to:
Adding
pyinstrument.Profiler
will help format the output of the slow callbacks. A full traceback of the call will be logged.WARNING: there is a slight performance hit for running the profiler. I think this is worth it in order to have more meaningful log messages which will help debugging the stack.
Trivia
While looking for the current solution, aioprof was considered and in part used as inspiration. Our codebase already had most of the tests and implementation in place to easily integrate
pyinstrument
on top of whichaioprof
is built.Related issue/s
How to test
Checklist