launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

Robert Collins has proposed merging lp:~lifeless/launchpad/profile into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~lifeless/launchpad/profile/+merge/51078

Allow overriding the profiling_allowed config entry via a feature flag. This will let us have profiling on staging for developers only. \o/
-- 
https://code.launchpad.net/~lifeless/launchpad/profile/+merge/51078
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/profile into lp:launchpad.

=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py	2011-02-20 21:15:57 +0000
+++ lib/lp/services/features/flags.py	2011-02-24 06:48:01 +0000
@@ -61,6 +61,10 @@
      'boolean',
      'Enables use of memcached where it is supported.',
      'enabled'),
+    ('profiling.enabled',
+     'boolean',
+     'Overrides config.profiling.profiling_allowed to permit profiling.'
+     ''),
     ('publicrestrictedlibrarian',
      'boolean',
      'Redirects to private librarian files instead of proxying them.',

=== modified file 'lib/lp/services/profile/configure.zcml'
--- lib/lp/services/profile/configure.zcml	2010-08-26 02:00:51 +0000
+++ lib/lp/services/profile/configure.zcml	2011-02-24 06:48:01 +0000
@@ -7,6 +7,11 @@
     xmlns:browser="http://namespaces.zope.org/browser";>
 
     <subscriber
+        for="zope.app.publication.interfaces.IBeforeTraverseEvent"
+        handler="lp.services.profile.profile.before_traverse"
+        />
+
+    <subscriber
         handler="lp.services.profile.profile.start_request"
         />
 

=== modified file 'lib/lp/services/profile/profile.py'
--- lib/lp/services/profile/profile.py	2011-02-17 16:58:57 +0000
+++ lib/lp/services/profile/profile.py	2011-02-24 06:48:01 +0000
@@ -14,6 +14,7 @@
 
 from bzrlib.lsprof import BzrProfiler
 from chameleon.zpt.template import PageTemplateFile
+from zope.app.publication.interfaces import IBeforeTraverseEvent
 from zope.app.publication.interfaces import IEndRequestEvent
 from zope.component import (
     adapter,
@@ -30,6 +31,7 @@
     memory,
     resident,
     )
+from lp.services.features import getFeatureFlag
 
 
 class ProfilingOops(Exception):
@@ -38,21 +40,49 @@
 
 _profilers = threading.local()
 
+def before_traverse(event):
+    """Handle profiling when enabled via the profiling.enabled feature flag."""
+    # This event is raised on each step of traversal so needs to be lightweight
+    # and not assume that profiling has not started - but this is equally well
+    # done in _maybe_profile so that function takes care of it. We have to use
+    # this event (or add a new one) because we depend on the feature flags
+    # system being configured and usable, and on the principal being known.
+    if getFeatureFlag('profiling.enabled'):
+        _maybe_profile(event)
+
 
 @adapter(IStartRequestEvent)
 def start_request(event):
-    """Handle profiling.
-
+    """Handle profiling when configured as permitted."""
+    if not config.profiling.profiling_allowed:
+        return
+    _maybe_profile(event)
+
+
+def _maybe_profile(event):
+    """Setup profiling as requested.
+    
     If profiling is enabled, start a profiler for this thread. If memory
     profiling is requested, save the VSS and RSS.
+
+    If already profiling, this is a no-op.
     """
-    if not config.profiling.profiling_allowed:
-        return
+    try:
+        if _profilers.profiling:
+            # Already profiling - e.g. called in from both start_request and
+            # before_traverse, or by successive before_traverse on one request.
+            return
+    except AttributeError:
+        # The first call in on a new thread cannot be profiling at the start.
+        pass
+    print "doing the do", event.request['PATH_INFO']
     actions = get_desired_profile_actions(event.request)
+    print "actions", event.request['PATH_INFO'], actions
     if config.profiling.profile_all_requests:
         actions.add('log')
     _profilers.actions = actions
     _profilers.profiler = None
+    _profilers.profiling = True
     if actions:
         if actions.difference(('help', )):
             # If this assertion has reason to fail, we'll need to add code
@@ -71,14 +101,15 @@
 @adapter(IEndRequestEvent)
 def end_request(event):
     """If profiling is turned on, save profile data for the request."""
-    if not config.profiling.profiling_allowed:
-        return
     try:
-        actions = _profilers.actions
+        if not _profilers.profiling:
+            return
+        _profilers.profiling = False
     except AttributeError:
         # Some tests don't go through all the proper motions, like firing off
         # a start request event.  Just be quiet about it.
         return
+    actions = _profilers.actions
     del _profilers.actions
     request = event.request
     # Create a timestamp including milliseconds.

=== modified file 'lib/lp/services/profile/tests.py'
--- lib/lp/services/profile/tests.py	2010-10-22 10:24:18 +0000
+++ lib/lp/services/profile/tests.py	2011-02-24 06:48:01 +0000
@@ -16,7 +16,10 @@
 
 from lp.testing import TestCase
 from bzrlib.lsprof import BzrProfiler
-from zope.app.publication.interfaces import EndRequestEvent
+from zope.app.publication.interfaces import (
+    BeforeTraverseEvent,
+    EndRequestEvent,
+    )
 from zope.component import getSiteManager
 
 import canonical.launchpad.webapp.adapter as da
@@ -26,6 +29,8 @@
     )
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
 from canonical.launchpad.webapp.interfaces import StartRequestEvent
+from canonical.testing import layers
+from lp.services.features.testing import FeatureFixture
 from lp.services.profile import profile
 
 EXAMPLE_HTML_START = '''\
@@ -69,22 +74,26 @@
             memory_profile_log=memory_profile_log)
 
 
-class TestRequestStartHandler(BaseTest):
-    """Tests for the start handler of the profiler integration.
-
-    See lib/canonical/doc/profiling.txt for an end-user description of
-    the functionality.
-    """
+class TestCleanupProfiler(BaseTest):
+    """Add a tearDown that will cleanup the profiler if it is running."""
 
     def tearDown(self):
         "Do the usual tearDown, plus clean up the profiler object."
         if getattr(profile._profilers, 'profiler', None) is not None:
             profile._profilers.profiler.stop()
             del profile._profilers.profiler
-        for name in ('actions', 'memory_profile_start'):
+        for name in ('actions', 'memory_profile_start', 'profiling'):
             if getattr(profile._profilers, name, None) is not None:
                 delattr(profile._profilers, name)
-        TestCase.tearDown(self)
+        super(TestCleanupProfiler, self).tearDown()
+
+
+class TestRequestStartHandler(TestCleanupProfiler):
+    """Tests for the start handler of the profiler integration.
+
+    See lib/canonical/doc/profiling.txt for an end-user description of
+    the functionality.
+    """
 
     def test_config_stops_profiling(self):
         """The ``profiling_allowed`` configuration should disable all
@@ -450,5 +459,21 @@
         self.assertCleanProfilerState()
 
 
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)
+class TestBeforeTraverseHandler(TestCleanupProfiler):
+
+    layer = layers.DatabaseFunctionalLayer
+
+    def test_can_enable_profiling_over_config(self):
+        # The flag profiling.enabled wins over a config that has
+        # disabled profiling. This permits the use of profiling on qastaging
+        # and similar systems.
+        self.pushProfilingConfig(
+            profiling_allowed='False', profile_all_requests='True',
+            memory_profile_log='.')
+        event = BeforeTraverseEvent(None,
+            self._get_request('/++profile++show,log'))
+        with FeatureFixture({'profiling.enabled':'on'}):
+            profile.before_traverse(event)
+            self.assertTrue(profile._profilers.profiling)
+            self.assertIsInstance(profile._profilers.profiler, BzrProfiler)
+            self.assertEquals(profile._profilers.actions, set(('show', 'log')))