Adds profiling tools and exposes profiler web UI by RobRuana · Pull Request #81 · magfest/sideboard

RobRuana · 2017-05-06T19:12:20Z

This is based on the built-in cherrypy profiler, but with some improvements.

The web UI looks like this:

…including the function name

…disabled

RobRuana · 2017-05-06T19:13:38Z

 plugins/*
 !plugins/conftest.py
 data/sessions/*
+data/profiler/*


Profiler data files are stored in data/profiler/ by default.

RobRuana · 2017-05-06T19:14:13Z



 [cherrypy]
+checker.check_skipped_app_config = boolean(default=False)


I moved this line just to put it in alphabetical order. In retrospect that was probably silly, but oh well!

RobRuana · 2017-05-06T19:18:30Z

           'ConfigurationError', 'parse_config',
           'is_listy', 'listify', 'serializer', 'cached_property', 'class_property', 'entry_point',
           'stopped', 'on_startup', 'on_shutdown', 'mainloop', 'ajax', 'renders_template', 'render_with_templates',
-           'restricted', 'all_restricted', 'register_authenticator'


Funny this line was missing a comma, so I think the two items were concatenated and register_authenticatorDaemonTask was part of __all__.

On a stylistic note, in my other projects I've started defining __all__ in my submodules and then doing import * to make the submodule exports available in the parent module. For example inside sideboard.lib._profiler I'll have:

__all__ = ['cleanup_profiler', 'profile', 'Profiler', 'ProfileAggregator']

and then in the parent module I'll have:

from sideboard.lib._profiler import *

That way each individual module is responsible for what it is exporting, and the parent module can easily import everything for easy access elsewhere. You get the same functionality we have here, but without an unwieldy __all__ list.

Funny this line was missing a comma, so I think the two items were concatenated and register_authenticatorDaemonTask was part of all.

Whoops! I had exactly the same thing happen in a different codebase at work recently. It's one of those typos you don't notice until it actually causes an error. Good catch!

...You get the same functionality we have here, but without an unwieldy all list.

Hmm, I'll have to think of that. I kind of like the way we have it now, because sideboard.lib is the place where you import non-SQLAlchemy Sideboard utilities, which means that there's a single file where you can look to find a complete list of everything that Sideboard exposes. That's a desirable property for a codebase to have IMO.

RobRuana · 2017-05-06T19:21:56Z

+
+    @cherrypy.expose
+    def menu(self):
+        yield '<h2>Profiling Runs</h2>'


I am not crazy about manually concatenating a bunch of HTML strings together, but this is how the cherrypy module was doing it, and I didn't feel like rewriting a bunch of working code.

RobRuana · 2017-05-06T19:22:39Z

+if config['cherrypy']['profiling.on']:
+    # If profiling is turned on then expose the web UI, otherwise ignore it.
+    from sideboard.lib import Profiler
+    cherrypy.tree.mount(Profiler(config['cherrypy']['profiling.path']), '/profiler')


This is where we conditionally enable the profiler web UI.

RobRuana · 2017-05-06T19:24:15Z

+    pass
+
+
+def test_profile_is_noop(monkeypatch):


The only thing I am testing is that the @profile decorator is a no-op when profiling is disabled. This is the only code that will be running in production. Everything else in the profiler module is per se testing code.

RobRuana · 2017-05-06T19:25:05Z

 deps= -rrequirements.txt
 commands=
-    coverage run --source sideboard -m py.test {posargs}
+    coverage run --source sideboard -m py.test {posargs} sideboard


This was preventing tests from running in our Vagrant deployments, because it would try to test everything in the plugins directory as well.

Yikes, good call.

RobRuana · 2017-05-06T19:26:29Z

 is_test_running = True

+# Prevent plugins from loading during tests
+plugins_dir = '%(root)s/test_plugins'


This is a non-existent directory to prevent sideboard from attempting to load plugins during tests.

Minor suggestion to make that more self-documenting: use does_not_exist instead of test_plugins.

I actually originally had that exact string! And then I was like, "well, maybe we DO want some test_plugins!" But I think you're right (and we have already have tests for loading test plugins).

kitsuta

This looks pretty good! This may seem like a silly question, but if a lot of this is just extending functionality in CherryPy, is there no way we can link to CherryPy's functions to extend them rather than copy-pasting their contents?

kitsuta · 2017-05-06T20:20:09Z

+
+    from sideboard.lib import profile
+
+    def some_long_running_function():


This example function doesn't have the decorator described above. An oversight, maybe?

Good catch!

kitsuta · 2017-05-06T20:21:24Z

+@entry_point
+def cleanup_profiler():
+    """
+    Deletes all `*.prof` files in the profiler's data directory.


What cases would this be useful? And if possible, can you describe an example of why you might want to do this in the stringdoc?

After a load test run you can have hundreds or thousands of profiling files. This is just a convenient way of deleting them.

kitsuta · 2017-05-06T20:23:35Z

+        return '''<html>
+        <head><title>Sideboard Profiler</title></head>
+        <frameset cols="300, 1*">
+            <frame src="menu"/>


Frames? Really? ;)

Blame the cherrypy folks! 😜

RobRuana · 2017-05-06T20:49:25Z

If you look through the commit history, you'll see I was originally just extending their classes. After switching from profile to cProfile I think I had almost every method overridden, and at that point I was like "forget it, it's easier to not use their stuff at all".

kitsuta · 2017-05-06T21:03:59Z

Fair enough!

EliAndrewC

Great stuff! I had a couple of minor suggestions, mostly style suggestions, most of which you can feel free to ignore if you prefer the style the way you have it.

EliAndrewC · 2017-05-07T20:13:12Z

+# If the profiler is disabled, then the @sideboard.lib.profile decorator
+# becomes a no-op and no performance penalty is incurred. The web interface
+# will not be created and visits to http://servername/profiler/ will 404.
+# True to enable, False to disable.


FWIW, I believe that "True to enable, False to disable" is a unnecessary comment given that this config option is explicitly declared to be a boolean.

Yes, the only valid values are True and False, but what do they do? The config setting is "profiling.on", and while it would seem obvious that "on" means "enabled" I decided to be explicit. Especially because the rest of the description uses the terms "enable" and "disable".

Take the first sentence:

Enable or disable the Sideboard profiler.

And the last sentence:

True to enable, False to disable.

No ambiguity there.

Fair enough. I can't argue with "Explicit is better than implicit."

EliAndrewC · 2017-05-07T20:13:57Z

+# decorator will be aggregated over time. Individual profiler files will be
+# created for each call, but the stats reported in each file will be the
+# aggregate of all previous runs, plus the current run. This will smooth out
+# how the profiler data changes over time, but it will be hard to guage the


Typo: guage.

Nice catch!

EliAndrewC · 2017-05-07T20:18:42Z

           'ConfigurationError', 'parse_config',
           'is_listy', 'listify', 'serializer', 'cached_property', 'class_property', 'entry_point',
           'stopped', 'on_startup', 'on_shutdown', 'mainloop', 'ajax', 'renders_template', 'render_with_templates',
-           'restricted', 'all_restricted', 'register_authenticator'


Funny this line was missing a comma, so I think the two items were concatenated and register_authenticatorDaemonTask was part of all.

Whoops! I had exactly the same thing happen in a different codebase at work recently. It's one of those typos you don't notice until it actually causes an error. Good catch!

...You get the same functionality we have here, but without an unwieldy all list.

Hmm, I'll have to think of that. I kind of like the way we have it now, because sideboard.lib is the place where you import non-SQLAlchemy Sideboard utilities, which means that there's a single file where you can look to find a complete list of everything that Sideboard exposes. That's a desirable property for a codebase to have IMO.

EliAndrewC · 2017-05-07T21:07:05Z

+    profiling.on = boolean(default=True)
+    profiling.path = string(default="%(root)s/data/profiler")
+    profiling.aggregate = boolean(default=False)
+    profiling.strip_dirs = boolean(default=False)


I think it might be clearer if we put example values rather than transcribing the actual definition of the config options, e.g.

[cherrypy] profiling.on = True profiling.strip_dirs = False profiling.aggregate = False profiling.path = "%(root)s/data/profiler"

Having talked to a lot of developers who aren't familiar with configobj, I think putting example values like that will be less confusing for newcomers than putting the ConfigObj optionb definition syntax here.

EliAndrewC · 2017-05-07T21:08:10Z

+
+import cherrypy
+from sideboard.config import config
+from sideboard.lib import entry_point, listify


Silly nitpick: the suggested canonical way to import config is to say from sideboard.lib import config.

Suggested by whom? Cause that sounds like crazy talk.

I mean, I'll change it. But time comes you convince me that importing config from lib makes sense, say your prayers because the end of days is upon us.

Suggested by whom?

https://www.youtube.com/watch?v=O07NghuK2ow&t=3s

The idea is that anything which is importable by plugins should be imported from sideboard.lib. We explicitly expose sideboard.lib.config so as to allow plugins to access Sideboard's config in case they need to look something up.

I'm not dead-set on this, but in general I like the idea of having "the canonical place where X should be imported from", and for anything deliberately exposed to plugins then that place is under sideboard.lib.

EliAndrewC · 2017-05-07T21:39:53Z

+        configspec.ini
+    """
+    if config['cherrypy'].get('profiling.on', False):
+        profiling_path = config['cherrypy'].get('profiling.path', None)


Two notes:

See above about not needing to say .get here.

What happens if someone sets an invalid path for profiling_path?

Internally this is just calling pstats.dump_stats(), so whatever that does with a bad path.

https://docs.python.org/3/library/profile.html#pstats.Stats.dump_stats

I'll add a comment explaining that.

That's fine - I'm definitely okay with a "garbage-in garbage-out" approach here.

EliAndrewC · 2017-05-07T21:47:00Z

+
+    def __init__(self, path=None):
+        if not path:
+            path = os.path.join(os.path.dirname(__file__), 'profile')


This seems like a bad default, since this makes the default inside the sideboard/lib directory. Unless I'm misunderstanding, I think it would be better to just say this:

def __init__(self, path=config['cherrypy']['profiling.path']):

I realize that in practice we're always passing a value, but leaving a nonsensical default value here seems bad.

Ah, yeah, this was copied out of cherrypy verbatim. I can change it.

EliAndrewC · 2017-05-07T21:51:45Z

 is_test_running = True

+# Prevent plugins from loading during tests
+plugins_dir = '%(root)s/test_plugins'


Minor suggestion to make that more self-documenting: use does_not_exist instead of test_plugins.

EliAndrewC · 2017-05-07T21:52:07Z

+
 [cherrypy]
 engine.autoreload.on = False
+profiling.on = 'False'


Minor style suggestion: use False instead of 'False'.

EliAndrewC · 2017-05-07T21:52:22Z

 deps= -rrequirements.txt
 commands=
-    coverage run --source sideboard -m py.test {posargs}
+    coverage run --source sideboard -m py.test {posargs} sideboard


Yikes, good call.

EliAndrewC

+1

RobRuana added 4 commits May 5, 2017 21:33

Exposes cherrypy profiler on http://servername/profiler/

cf536e6

Adds improved implementation of cherrypy profiler

c1b73a7

Adds some profiler docs, improves the profiler file naming scheme by …

6781fb8

…including the function name

Adds test to verify that profile decorator is a noop if profiling is …

eef2122

…disabled

RobRuana commented May 6, 2017

View reviewed changes

Fixes tests in test_configuration

ed48f13

kitsuta reviewed May 6, 2017

View reviewed changes

Fixes docs per code review

4bc2483

RobRuana requested a review from EliAndrewC May 6, 2017 21:13

EliAndrewC requested changes May 7, 2017

View reviewed changes

Changes requested in code review

b08ac65

RobRuana merged commit 24d9fd6 into master May 8, 2017

RobRuana deleted the expose_cherrypy_profiler branch May 8, 2017 00:04

EliAndrewC reviewed May 8, 2017

View reviewed changes



		[cherrypy]
		checker.check_skipped_app_config = boolean(default=False)


		from sideboard.lib import profile

		def some_long_running_function():

Conversation

RobRuana commented May 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RobRuana May 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kitsuta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RobRuana commented May 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kitsuta commented May 6, 2017

Uh oh!

EliAndrewC left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

RobRuana commented May 6, 2017 •

edited

Loading

RobRuana May 6, 2017 •

edited

Loading

RobRuana commented May 6, 2017 •

edited

Loading