launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01665
[Merge] lp:~mars/launchpad/add-profiling-feature-flag into lp:launchpad/devel
Māris Fogels has proposed merging lp:~mars/launchpad/add-profiling-feature-flag into lp:launchpad/devel with lp:~mars/launchpad/feature-flag-fixture as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#645768 need feature flag test fixture
https://bugs.launchpad.net/bugs/645768
Hi,
This branch changes our profiling code to use feature flags instead of config values. The code takes advantage of the new FeatureFixture, added in the prerequisite branch, to do the heavy lifting. Later we will be able to turn profiling on and off from the Launchpad UI.
--
https://code.launchpad.net/~mars/launchpad/add-profiling-feature-flag/+merge/39179
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mars/launchpad/add-profiling-feature-flag into lp:launchpad/devel.
=== modified file 'lib/canonical/launchpad/doc/profiling.txt'
--- lib/canonical/launchpad/doc/profiling.txt 2010-08-26 15:57:56 +0000
+++ lib/canonical/launchpad/doc/profiling.txt 2010-10-22 20:36:24 +0000
@@ -100,11 +100,10 @@
The feature has two modes.
-- It can be configured to optionally profile requests. To turn this on, in
- ``launchpad-lazr.conf`` (e.g.,
- ``configs/development/launchpad-lazr.conf``) , in the ``[profiling]``
- section, set ``profiling_allowed: True``. As of this writing, this
- is the default value for development.
+- It can be configured to optionally profile requests. To turn this on you
+ must add a feature flag that sets the 'request_profiling' flag to 'on'.
+ This flag can be set directly in the database or using bin/harness. See
+ https://dev.launchpad.net/LEP/FeatureFlags for the details.
Once it is turned on, you can insert /++profile++/ in the URL to get
basic instructions on how to use the feature. You use the
@@ -123,6 +122,19 @@
lp/services/profile is hooked up properly. This is intended to be a
smoke test. The unit tests verify further functionality.
+ # XXX mars 2010-09-23
+ # We will temporarily turn on the profiling feature so that this test
+ # still passes. Profiling used to be active for all developers, but the
+ # feature flags work has shut it off. This fixture can be removed once
+ # profiling for developers is active by default once again.
+ >>> from lp.services.features.testing import FeatureFixture
+ >>> fixture = FeatureFixture({'request_profiling': 'on'})
+ >>> fixture.setUp()
+
+ >>> from lp.services.features import getFeatureFlag
+ >>> getFeatureFlag('request_profiling')
+ u'on'
+
>>> response = http('GET /++profile++ HTTP/1.0')
>>> '<h1>Profiling Information</h1>' in response.getBody()
True
@@ -214,3 +226,7 @@
>>> shutil.rmtree(pagetests_profile_dir)
>>> shutil.rmtree(profile_dir)
>>> old_config = config.pop('memory_profile')
+
+
+ # Turn profiling off
+ >>> fixture.cleanUp()
=== modified file 'lib/lp/services/profile/profile.py'
--- lib/lp/services/profile/profile.py 2010-08-27 14:20:28 +0000
+++ lib/lp/services/profile/profile.py 2010-10-22 20:36:24 +0000
@@ -30,12 +30,15 @@
memory,
resident,
)
+from lp.services import features
class ProfilingOops(Exception):
"""Fake exception used to log OOPS information when profiling pages."""
+# This variable holds all of the data about an active profile run for the
+# duration of the request.
_profilers = threading.local()
@@ -46,8 +49,16 @@
If profiling is enabled, start a profiler for this thread. If memory
profiling is requested, save the VSS and RSS.
"""
- if not config.profiling.profiling_allowed:
+ if features.getFeatureFlag('request_profiling'):
+ # Set a flag so that the end_request hook knows it has some work to
+ # do. We can not use the feature flag itself to control our
+ # end_request event handlers because the handler that tears down the
+ # feature flags and the handler that ends the profiling run do not
+ # fire in a predictable order.
+ _profilers.active = True
+ else:
return
+
actions = get_desired_profile_actions(event.request)
if config.profiling.profile_all_requests:
actions.add('log')
@@ -71,7 +82,7 @@
@adapter(IEndRequestEvent)
def end_request(event):
"""If profiling is turned on, save profile data for the request."""
- if not config.profiling.profiling_allowed:
+ if not getattr(_profilers, 'active', False):
return
try:
actions = _profilers.actions
=== modified file 'lib/lp/services/profile/tests.py'
--- lib/lp/services/profile/tests.py 2010-08-26 22:10:56 +0000
+++ lib/lp/services/profile/tests.py 2010-10-22 20:36:24 +0000
@@ -14,7 +14,6 @@
import time
import unittest
-from lp.testing import TestCase
from bzrlib.lsprof import BzrProfiler
from zope.app.publication.interfaces import EndRequestEvent
from zope.component import getSiteManager
@@ -26,7 +25,11 @@
)
from canonical.launchpad.webapp.servers import LaunchpadTestRequest
from canonical.launchpad.webapp.interfaces import StartRequestEvent
+from canonical.testing import layers
from lp.services.profile import profile
+from lp.services.features.testing import FeatureFixture
+from lp.testing import TestCase
+
EXAMPLE_HTML_START = '''\
<html><head><title>Random!</title></head>
@@ -43,6 +46,8 @@
class BaseTest(TestCase):
+ layer = layers.DatabaseFunctionalLayer
+
def _get_request(self, path='/'):
"""Return a test request for the given path."""
return LaunchpadTestRequest(PATH_INFO=path)
@@ -58,13 +63,19 @@
getattr(profile._profilers, name, None), None,
'Profiler state (%s) is dirty; %s.' % (name, message))
+ def turnOnProfiling(self):
+ """Turn on the request_profiling feature flag."""
+ self.useFixture(FeatureFixture({'request_profiling': 'on'}))
+
+ def turnOffProfiling(self):
+ """Turn off the request_profiling feature flag."""
+ self.useFixture(FeatureFixture({'request_profiling': None}))
+
def pushProfilingConfig(
- self, profiling_allowed='False', profile_all_requests='False',
- memory_profile_log=''):
+ self, profile_all_requests='False', memory_profile_log=''):
"""This is a convenience for setting profile configs."""
self.pushConfig(
'profiling',
- profiling_allowed=profiling_allowed,
profile_all_requests=profile_all_requests,
memory_profile_log=memory_profile_log)
@@ -86,11 +97,12 @@
delattr(profile._profilers, name)
TestCase.tearDown(self)
- def test_config_stops_profiling(self):
- """The ``profiling_allowed`` configuration should disable all
+ def test_feature_flag_stops_profiling(self):
+ """The ``request_profiling`` feature flag should disable all
profiling, even if it is requested"""
+ self.turnOffProfiling()
self.pushProfilingConfig(
- profiling_allowed='False', profile_all_requests='True',
+ profile_all_requests='True',
memory_profile_log='.')
profile.start_request(self._get_start_event('/++profile++show,log'))
self.assertCleanProfilerState('config was ignored')
@@ -98,7 +110,7 @@
def test_optional_profiling_without_marked_request_has_no_profile(self):
"""Even if profiling is allowed, it does not happen with a normal
request."""
- self.pushProfilingConfig(profiling_allowed='True')
+ self.turnOnProfiling()
profile.start_request(self._get_start_event('/'))
self.assertEqual(profile._profilers.actions, set())
self.assertIs(getattr(profile._profilers, 'profiler', None), None)
@@ -108,7 +120,7 @@
def test_optional_profiling_with_show_request_starts_profiling(self):
"""If profiling is allowed and a request with the "show" marker
URL segment is made, profiling starts."""
- self.pushProfilingConfig(profiling_allowed='True')
+ self.turnOnProfiling()
profile.start_request(self._get_start_event('/++profile++show/'))
self.assertIsInstance(profile._profilers.profiler, BzrProfiler)
self.assertIs(
@@ -119,7 +131,7 @@
def test_optional_profiling_with_log_request_starts_profiling(self):
"""If profiling is allowed and a request with the "log" marker
URL segment is made, profiling starts."""
- self.pushProfilingConfig(profiling_allowed='True')
+ self.turnOnProfiling()
profile.start_request(self._get_start_event('/++profile++log/'))
self.assertIsInstance(profile._profilers.profiler, BzrProfiler)
self.assertIs(
@@ -130,7 +142,7 @@
def test_optional_profiling_with_combined_request_starts_profiling(self):
"""If profiling is allowed and a request with the "log" and
"show" marker URL segment is made, profiling starts."""
- self.pushProfilingConfig(profiling_allowed='True')
+ self.turnOnProfiling()
profile.start_request(self._get_start_event('/++profile++log,show/'))
self.assertIsInstance(profile._profilers.profiler, BzrProfiler)
self.assertIs(
@@ -141,7 +153,7 @@
def test_optional_profiling_with_reversed_request_starts_profiling(self):
"""If profiling is allowed and a request with the "show" and
"log" marker URL segment is made, profiling starts."""
- self.pushProfilingConfig(profiling_allowed='True')
+ self.turnOnProfiling()
# The fact that this is reversed from the previous request is the only
# difference from the previous test. Also, it doesn't have a
# trailing slash. :-P
@@ -154,8 +166,8 @@
def test_forced_profiling_registers_action(self):
"""profile_all_requests should register as a log action"""
- self.pushProfilingConfig(
- profiling_allowed='True', profile_all_requests='True')
+ self.turnOnProfiling()
+ self.pushProfilingConfig(profile_all_requests='True')
profile.start_request(self._get_start_event('/'))
self.assertIsInstance(profile._profilers.profiler, BzrProfiler)
self.assertIs(
@@ -167,7 +179,7 @@
"""If profiling is allowed and a request with the marker URL segment
is made incorrectly, profiling does not start and help is an action.
"""
- self.pushProfilingConfig(profiling_allowed='True')
+ self.turnOnProfiling()
profile.start_request(self._get_start_event('/++profile++/'))
self.assertIs(getattr(profile._profilers, 'profiler', None), None)
self.assertIs(
@@ -179,8 +191,8 @@
"""If profiling is forced and a request with the marker URL segment
is made incorrectly, profiling starts and help is an action.
"""
- self.pushProfilingConfig(
- profiling_allowed='True', profile_all_requests='True')
+ self.turnOnProfiling()
+ self.pushProfilingConfig(profile_all_requests='True')
profile.start_request(self._get_start_event('/++profile++/'))
self.assertIsInstance(profile._profilers.profiler, BzrProfiler)
self.assertIs(
@@ -189,8 +201,8 @@
self.assertEquals(profile._profilers.actions, set(('help', 'log')))
def test_memory_profile_start(self):
- self.pushProfilingConfig(
- profiling_allowed='True', memory_profile_log='.')
+ self.turnOnProfiling()
+ self.pushProfilingConfig(memory_profile_log='.')
profile.start_request(self._get_start_event('/'))
self.assertIs(getattr(profile._profilers, 'profiler', None), None)
self.assertIsInstance(profile._profilers.memory_profile_start, tuple)
@@ -198,8 +210,8 @@
self.assertEqual(profile._profilers.actions, set())
def test_combo_memory_and_profile_start(self):
- self.pushProfilingConfig(
- profiling_allowed='True', memory_profile_log='.')
+ self.turnOnProfiling()
+ self.pushProfilingConfig(memory_profile_log='.')
profile.start_request(self._get_start_event('/++profile++show/'))
self.assertIsInstance(profile._profilers.profiler, BzrProfiler)
self.assertIsInstance(profile._profilers.memory_profile_start, tuple)
@@ -267,10 +279,11 @@
#########################################################################
# Tests
def test_config_stops_profiling(self):
- """The ``profiling_allowed`` configuration should disable all
+ """The ``request_profiling`` feature should disable all
profiling, even if it is requested"""
+ self.turnOffProfiling()
self.pushProfilingConfig(
- profiling_allowed='False', profile_all_requests='True',
+ profile_all_requests='True',
memory_profile_log=self.memory_profile_log)
request = self.endRequest('/++profile++show,log')
self.assertIs(getattr(request, 'oops', None), None)
@@ -282,7 +295,7 @@
def test_optional_profiling_without_marked_request_has_no_profile(self):
"""Even if profiling is allowed, it does not happen with a normal
request."""
- self.pushProfilingConfig(profiling_allowed='True')
+ self.turnOnProfiling()
request = self.endRequest('/')
self.assertIs(getattr(request, 'oops', None), None)
self.assertEqual(self.getAddedResponse(request), '')
@@ -293,7 +306,7 @@
def test_optional_profiling_with_show_request_profiles(self):
"""If profiling is allowed and a request with the "show" marker
URL segment is made, profiling starts."""
- self.pushProfilingConfig(profiling_allowed='True')
+ self.turnOnProfiling()
request = self.endRequest('/++profile++show/')
self.assertIsInstance(request.oops, ErrorReport)
self.assertIn('Top Inline Time', self.getAddedResponse(request))
@@ -304,7 +317,7 @@
def test_optional_profiling_with_log_request_profiles(self):
"""If profiling is allowed and a request with the "log" marker
URL segment is made, profiling starts."""
- self.pushProfilingConfig(profiling_allowed='True')
+ self.turnOnProfiling()
request = self.endRequest('/++profile++log/')
self.assertIsInstance(request.oops, ErrorReport)
response = self.getAddedResponse(request)
@@ -319,7 +332,7 @@
def test_optional_profiling_with_combined_request_profiles(self):
"""If profiling is allowed and a request with the "log" and
"show" marker URL segment is made, profiling starts."""
- self.pushProfilingConfig(profiling_allowed='True')
+ self.turnOnProfiling()
request = self.endRequest('/++profile++log,show')
self.assertIsInstance(request.oops, ErrorReport)
response = self.getAddedResponse(request)
@@ -333,8 +346,8 @@
def test_forced_profiling_logs(self):
"""profile_all_requests should register as a log action"""
- self.pushProfilingConfig(
- profiling_allowed='True', profile_all_requests='True')
+ self.turnOnProfiling()
+ self.pushProfilingConfig(profile_all_requests='True')
request = self.endRequest('/')
self.assertIsInstance(request.oops, ErrorReport)
response = self.getAddedResponse(request)
@@ -351,7 +364,7 @@
"""If profiling is allowed and a request with the marker URL segment
is made incorrectly, profiling does not start and help is an action.
"""
- self.pushProfilingConfig(profiling_allowed='True')
+ self.turnOnProfiling()
request = self.endRequest('/++profile++')
self.assertIs(getattr(request, 'oops', None), None)
response = self.getAddedResponse(request)
@@ -365,8 +378,8 @@
"""If profiling is forced and a request with the marker URL segment
is made incorrectly, profiling starts and help is an action.
"""
- self.pushProfilingConfig(
- profiling_allowed='True', profile_all_requests='True')
+ self.turnOnProfiling()
+ self.pushProfilingConfig(profile_all_requests='True')
request = self.endRequest('/++profile++')
self.assertIsInstance(request.oops, ErrorReport)
response = self.getAddedResponse(request)
@@ -382,9 +395,8 @@
def test_memory_profile(self):
"Does the memory profile work?"
- self.pushProfilingConfig(
- profiling_allowed='True',
- memory_profile_log=self.memory_profile_log)
+ self.turnOnProfiling()
+ self.pushProfilingConfig(memory_profile_log=self.memory_profile_log)
request = self.endRequest('/')
self.assertIs(getattr(request, 'oops', None), None)
self.assertEqual(self.getAddedResponse(request), '')
@@ -400,9 +412,8 @@
def test_memory_profile_with_non_defaults(self):
"Does the memory profile work with an oops and pageid?"
- self.pushProfilingConfig(
- profiling_allowed='True',
- memory_profile_log=self.memory_profile_log)
+ self.turnOnProfiling()
+ self.pushProfilingConfig(memory_profile_log=self.memory_profile_log)
request = self.endRequest('/++profile++show/no-such-file',
KeyError(), pageid='Foo')
log = self.getMemoryLog()
@@ -413,9 +424,8 @@
self.assertCleanProfilerState()
def test_combo_memory_and_profile(self):
- self.pushProfilingConfig(
- profiling_allowed='True',
- memory_profile_log=self.memory_profile_log)
+ self.turnOnProfiling()
+ self.pushProfilingConfig(memory_profile_log=self.memory_profile_log)
request = self.endRequest('/++profile++show/')
self.assertIsInstance(request.oops, ErrorReport)
self.assertIn('Top Inline Time', self.getAddedResponse(request))
@@ -424,7 +434,7 @@
self.assertCleanProfilerState()
def test_profiling_oops_is_informational(self):
- self.pushProfilingConfig(profiling_allowed='True')
+ self.turnOnProfiling()
request = self.endRequest('/++profile++show/')
response = self.getAddedResponse(request)
self.assertIsInstance(request.oops, ErrorReport)
@@ -433,7 +443,7 @@
self.assertCleanProfilerState()
def test_real_oops_trumps_profiling_oops(self):
- self.pushProfilingConfig(profiling_allowed='True')
+ self.turnOnProfiling()
request = self.endRequest('/++profile++show/no-such-file',
KeyError('foo'))
self.assertIsInstance(request.oops, ErrorReport)
@@ -444,7 +454,7 @@
self.assertCleanProfilerState()
def test_oopsid_is_in_profile_filename(self):
- self.pushProfilingConfig(profiling_allowed='True')
+ self.turnOnProfiling()
request = self.endRequest('/++profile++log/')
self.assertIn("-" + request.oopsid + "-", self.getProfilePaths()[0])
self.assertCleanProfilerState()
Follow ups
