Fixes #1939: Enable cProfile of tasks #2882
Conversation
|
I'm not entirely sure how to add tests for this particular case, but if you feel they would be useful and point me in the right direction I am more than happy to add. |
|
Can one of the admins verify this patch? |
| # writeable and readable by Pulp. | ||
|
|
||
| [profiling] | ||
| # enable: true |
There was a problem hiding this comment.
This should default to off yes?
|
|
||
| if config.get('profiling', 'enabled') is True: |
There was a problem hiding this comment.
Can we unguard this import statement and move it up with the other standard imports? We can import it 100 percent of the time I think.
|
@ehelms Thank you for submitting this PR! Can a release note be added. I think you'll have to make this file, but another PR already has one so maybe just make something similar and the first one to merge wins. |
|
Also can the following be added to the commit so that Redmine associates it correctly: |
|
Also Travis is saying there are flake8 errors which need to be fixed. I think I saw just one. Click details in the 'failing checks' area at the bottom of this PR to look at the travis report. |
|
How do you feel about updating the debugging docs? I imagine it would be a section at the top of the above link^. It would briefly explain the behavior of the feature. Beyond that, you can mostly just point them at the server.conf details instead of duplicating that text. |
There was a problem hiding this comment.
Thank you so much for this PR! This is a great feature. I left several comments with changes requested. I would really like to see all of them, but if there are ones you don't want to do please let me know and we can handle that as followup work.
Thank you @ehelms!
|
ok test |
|
@ehelms regarding tests, I don't think you need to add any. I think ensuring all existing tests pass is important. I just kicked off a test runner. If you did really want to add tests you would put them in this module here. |
|
To be honest, I don't understand the flake8 output. |
|
I have updated the following:
|
|
@ehelms I looked at the flake8 output and actually flake8 is confused. It's reporting a python flake8 issue on a .conf file. Please ignore the flake8 issue. |
| @@ -0,0 +1,11 @@ | |||
| ======================= | |||
There was a problem hiding this comment.
1 line needs to be added to the index file that indexes these release notes. Otherwise sphinx won't discover this file.
| @@ -25,6 +26,7 @@ | |||
| from pulp.server.managers.repo import _common as common_utils | |||
| from pulp.server.managers import factory as managers | |||
| from pulp.server.managers.schedule import utils | |||
| from pulp.server import config | |||
There was a problem hiding this comment.
Can this be moved above all pulp.server imports. That would put it just after the last pulp.common import.
|
Updated |
| @@ -136,6 +136,10 @@ def _load_config(self): | |||
| 'download_interval': '30', | |||
| 'download_concurrency': '5' | |||
| }, | |||
| 'profiling': { | |||
| 'enabled': False, | |||
There was a problem hiding this comment.
I think the tests are failing because this is False but it expects 'false'. Crazy I know. Looking earlier in the file is the only reason I know. Please push a new version and I can kick off the tests again.
ehelms
force-pushed
the
fix-1939
branch
2 times, most recently
from
d171870
to
ab368c1
Compare
|
The good news is that the test failures now look sane. The bad news is there are 104 test failures. I think those 104 test failures are due to the mock objects which mock the config loading stuff at test time is not robust. |
Adds the ability to turn on cProfiles of individual Pulp tasks and dumping of the tasks into a directory. This can have an impact on performance. The trade off is that users could investigate individual tasks or provide these cProfiles to developers to help detect anomalies or systematic issues. https://pulp.plan.io/issues/1939
|
Woot! Tests are passing, stupid flak8 |
There was a problem hiding this comment.
@ehelms you diiiiiiiiiiiiiiiid it! Great job all around. I think this is good to go. I'm going to ask one other core dev to approve prior to merging, but I'm approving mine now.
Thank you so much for this. Would you want to demo this at a sprint demo on Jan 5th or would you want someone else to do that?
Adds the ability to turn on cProfiles of individual Pulp tasks
and dumping of the tasks into a directory. This can have an impact
on performance. The trade off is that users could investigate
individual tasks or provide these cProfiles to developers to help
detect anomalies or systematic issues.