Merge lp:~mars/launchpad/add-profiling-feature-flag into lp:launchpad

Proposed by Māris Fogels
Status: Work in progress
Proposed branch: lp:~mars/launchpad/add-profiling-feature-flag
Merge into: lp:launchpad
Prerequisite: lp:~mars/launchpad/feature-flag-fixture
Diff against target: 408 lines (+88/-51)
3 files modified
lib/canonical/launchpad/doc/profiling.txt (+21/-5)
lib/lp/services/profile/profile.py (+13/-2)
lib/lp/services/profile/tests.py (+54/-44)
To merge this branch: bzr merge lp:~mars/launchpad/add-profiling-feature-flag
Reviewer Review Type Date Requested Status
Launchpad code reviewers Pending
Review via email: mp+39179@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Robert Collins (lifeless) wrote :

Hi, we still need a config setting to control profiling because we do
not ever want it on in prod.

Here's what I think we need, for clarity:
C - config
F - flags
 - on prod
   C - allow_profiling False: no profiling, no how, no way
 - no devel
   C - allow_profiling true: profiling anytime
 - on staging
   C - allow_profiling True:
   F - profiling on request by developers [use a team scope to id the developer]
   F - profiling of all requests [use the default scope to turn it on
for all requests]

That is, the check for 'should we profile' is:
config.allow_profiling and getFeatureFlag('profile') == 'on'

-Rob

Unmerged revisions

11598. By Māris Fogels

Typo

11597. By Māris Fogels

Better comments

11596. By Māris Fogels

Merged feature-flag-fixture into add-profiling-feature-flag.

11595. By Māris Fogels

Merged feature-flag-fixture into add-profiling-feature-flag.

11594. By Māris Fogels

Merged feature-flag-fixture into add-profiling-feature-flag.

11593. By Māris Fogels

Merged in Graham's fixes for the feature flags helper setting None values. Added a test to verify the fix.

11592. By Māris Fogels

Merge from devel

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/doc/profiling.txt'
2--- lib/canonical/launchpad/doc/profiling.txt 2010-08-26 15:57:56 +0000
3+++ lib/canonical/launchpad/doc/profiling.txt 2010-10-22 20:36:24 +0000
4@@ -100,11 +100,10 @@
5
6 The feature has two modes.
7
8-- It can be configured to optionally profile requests. To turn this on, in
9- ``launchpad-lazr.conf`` (e.g.,
10- ``configs/development/launchpad-lazr.conf``) , in the ``[profiling]``
11- section, set ``profiling_allowed: True``. As of this writing, this
12- is the default value for development.
13+- It can be configured to optionally profile requests. To turn this on you
14+ must add a feature flag that sets the 'request_profiling' flag to 'on'.
15+ This flag can be set directly in the database or using bin/harness. See
16+ https://dev.launchpad.net/LEP/FeatureFlags for the details.
17
18 Once it is turned on, you can insert /++profile++/ in the URL to get
19 basic instructions on how to use the feature. You use the
20@@ -123,6 +122,19 @@
21 lp/services/profile is hooked up properly. This is intended to be a
22 smoke test. The unit tests verify further functionality.
23
24+ # XXX mars 2010-09-23
25+ # We will temporarily turn on the profiling feature so that this test
26+ # still passes. Profiling used to be active for all developers, but the
27+ # feature flags work has shut it off. This fixture can be removed once
28+ # profiling for developers is active by default once again.
29+ >>> from lp.services.features.testing import FeatureFixture
30+ >>> fixture = FeatureFixture({'request_profiling': 'on'})
31+ >>> fixture.setUp()
32+
33+ >>> from lp.services.features import getFeatureFlag
34+ >>> getFeatureFlag('request_profiling')
35+ u'on'
36+
37 >>> response = http('GET /++profile++ HTTP/1.0')
38 >>> '<h1>Profiling Information</h1>' in response.getBody()
39 True
40@@ -214,3 +226,7 @@
41 >>> shutil.rmtree(pagetests_profile_dir)
42 >>> shutil.rmtree(profile_dir)
43 >>> old_config = config.pop('memory_profile')
44+
45+
46+ # Turn profiling off
47+ >>> fixture.cleanUp()
48
49=== modified file 'lib/lp/services/profile/profile.py'
50--- lib/lp/services/profile/profile.py 2010-08-27 14:20:28 +0000
51+++ lib/lp/services/profile/profile.py 2010-10-22 20:36:24 +0000
52@@ -30,12 +30,15 @@
53 memory,
54 resident,
55 )
56+from lp.services import features
57
58
59 class ProfilingOops(Exception):
60 """Fake exception used to log OOPS information when profiling pages."""
61
62
63+# This variable holds all of the data about an active profile run for the
64+# duration of the request.
65 _profilers = threading.local()
66
67
68@@ -46,8 +49,16 @@
69 If profiling is enabled, start a profiler for this thread. If memory
70 profiling is requested, save the VSS and RSS.
71 """
72- if not config.profiling.profiling_allowed:
73+ if features.getFeatureFlag('request_profiling'):
74+ # Set a flag so that the end_request hook knows it has some work to
75+ # do. We can not use the feature flag itself to control our
76+ # end_request event handlers because the handler that tears down the
77+ # feature flags and the handler that ends the profiling run do not
78+ # fire in a predictable order.
79+ _profilers.active = True
80+ else:
81 return
82+
83 actions = get_desired_profile_actions(event.request)
84 if config.profiling.profile_all_requests:
85 actions.add('log')
86@@ -71,7 +82,7 @@
87 @adapter(IEndRequestEvent)
88 def end_request(event):
89 """If profiling is turned on, save profile data for the request."""
90- if not config.profiling.profiling_allowed:
91+ if not getattr(_profilers, 'active', False):
92 return
93 try:
94 actions = _profilers.actions
95
96=== modified file 'lib/lp/services/profile/tests.py'
97--- lib/lp/services/profile/tests.py 2010-08-26 22:10:56 +0000
98+++ lib/lp/services/profile/tests.py 2010-10-22 20:36:24 +0000
99@@ -14,7 +14,6 @@
100 import time
101 import unittest
102
103-from lp.testing import TestCase
104 from bzrlib.lsprof import BzrProfiler
105 from zope.app.publication.interfaces import EndRequestEvent
106 from zope.component import getSiteManager
107@@ -26,7 +25,11 @@
108 )
109 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
110 from canonical.launchpad.webapp.interfaces import StartRequestEvent
111+from canonical.testing import layers
112 from lp.services.profile import profile
113+from lp.services.features.testing import FeatureFixture
114+from lp.testing import TestCase
115+
116
117 EXAMPLE_HTML_START = '''\
118 <html><head><title>Random!</title></head>
119@@ -43,6 +46,8 @@
120
121 class BaseTest(TestCase):
122
123+ layer = layers.DatabaseFunctionalLayer
124+
125 def _get_request(self, path='/'):
126 """Return a test request for the given path."""
127 return LaunchpadTestRequest(PATH_INFO=path)
128@@ -58,13 +63,19 @@
129 getattr(profile._profilers, name, None), None,
130 'Profiler state (%s) is dirty; %s.' % (name, message))
131
132+ def turnOnProfiling(self):
133+ """Turn on the request_profiling feature flag."""
134+ self.useFixture(FeatureFixture({'request_profiling': 'on'}))
135+
136+ def turnOffProfiling(self):
137+ """Turn off the request_profiling feature flag."""
138+ self.useFixture(FeatureFixture({'request_profiling': None}))
139+
140 def pushProfilingConfig(
141- self, profiling_allowed='False', profile_all_requests='False',
142- memory_profile_log=''):
143+ self, profile_all_requests='False', memory_profile_log=''):
144 """This is a convenience for setting profile configs."""
145 self.pushConfig(
146 'profiling',
147- profiling_allowed=profiling_allowed,
148 profile_all_requests=profile_all_requests,
149 memory_profile_log=memory_profile_log)
150
151@@ -86,11 +97,12 @@
152 delattr(profile._profilers, name)
153 TestCase.tearDown(self)
154
155- def test_config_stops_profiling(self):
156- """The ``profiling_allowed`` configuration should disable all
157+ def test_feature_flag_stops_profiling(self):
158+ """The ``request_profiling`` feature flag should disable all
159 profiling, even if it is requested"""
160+ self.turnOffProfiling()
161 self.pushProfilingConfig(
162- profiling_allowed='False', profile_all_requests='True',
163+ profile_all_requests='True',
164 memory_profile_log='.')
165 profile.start_request(self._get_start_event('/++profile++show,log'))
166 self.assertCleanProfilerState('config was ignored')
167@@ -98,7 +110,7 @@
168 def test_optional_profiling_without_marked_request_has_no_profile(self):
169 """Even if profiling is allowed, it does not happen with a normal
170 request."""
171- self.pushProfilingConfig(profiling_allowed='True')
172+ self.turnOnProfiling()
173 profile.start_request(self._get_start_event('/'))
174 self.assertEqual(profile._profilers.actions, set())
175 self.assertIs(getattr(profile._profilers, 'profiler', None), None)
176@@ -108,7 +120,7 @@
177 def test_optional_profiling_with_show_request_starts_profiling(self):
178 """If profiling is allowed and a request with the "show" marker
179 URL segment is made, profiling starts."""
180- self.pushProfilingConfig(profiling_allowed='True')
181+ self.turnOnProfiling()
182 profile.start_request(self._get_start_event('/++profile++show/'))
183 self.assertIsInstance(profile._profilers.profiler, BzrProfiler)
184 self.assertIs(
185@@ -119,7 +131,7 @@
186 def test_optional_profiling_with_log_request_starts_profiling(self):
187 """If profiling is allowed and a request with the "log" marker
188 URL segment is made, profiling starts."""
189- self.pushProfilingConfig(profiling_allowed='True')
190+ self.turnOnProfiling()
191 profile.start_request(self._get_start_event('/++profile++log/'))
192 self.assertIsInstance(profile._profilers.profiler, BzrProfiler)
193 self.assertIs(
194@@ -130,7 +142,7 @@
195 def test_optional_profiling_with_combined_request_starts_profiling(self):
196 """If profiling is allowed and a request with the "log" and
197 "show" marker URL segment is made, profiling starts."""
198- self.pushProfilingConfig(profiling_allowed='True')
199+ self.turnOnProfiling()
200 profile.start_request(self._get_start_event('/++profile++log,show/'))
201 self.assertIsInstance(profile._profilers.profiler, BzrProfiler)
202 self.assertIs(
203@@ -141,7 +153,7 @@
204 def test_optional_profiling_with_reversed_request_starts_profiling(self):
205 """If profiling is allowed and a request with the "show" and
206 "log" marker URL segment is made, profiling starts."""
207- self.pushProfilingConfig(profiling_allowed='True')
208+ self.turnOnProfiling()
209 # The fact that this is reversed from the previous request is the only
210 # difference from the previous test. Also, it doesn't have a
211 # trailing slash. :-P
212@@ -154,8 +166,8 @@
213
214 def test_forced_profiling_registers_action(self):
215 """profile_all_requests should register as a log action"""
216- self.pushProfilingConfig(
217- profiling_allowed='True', profile_all_requests='True')
218+ self.turnOnProfiling()
219+ self.pushProfilingConfig(profile_all_requests='True')
220 profile.start_request(self._get_start_event('/'))
221 self.assertIsInstance(profile._profilers.profiler, BzrProfiler)
222 self.assertIs(
223@@ -167,7 +179,7 @@
224 """If profiling is allowed and a request with the marker URL segment
225 is made incorrectly, profiling does not start and help is an action.
226 """
227- self.pushProfilingConfig(profiling_allowed='True')
228+ self.turnOnProfiling()
229 profile.start_request(self._get_start_event('/++profile++/'))
230 self.assertIs(getattr(profile._profilers, 'profiler', None), None)
231 self.assertIs(
232@@ -179,8 +191,8 @@
233 """If profiling is forced and a request with the marker URL segment
234 is made incorrectly, profiling starts and help is an action.
235 """
236- self.pushProfilingConfig(
237- profiling_allowed='True', profile_all_requests='True')
238+ self.turnOnProfiling()
239+ self.pushProfilingConfig(profile_all_requests='True')
240 profile.start_request(self._get_start_event('/++profile++/'))
241 self.assertIsInstance(profile._profilers.profiler, BzrProfiler)
242 self.assertIs(
243@@ -189,8 +201,8 @@
244 self.assertEquals(profile._profilers.actions, set(('help', 'log')))
245
246 def test_memory_profile_start(self):
247- self.pushProfilingConfig(
248- profiling_allowed='True', memory_profile_log='.')
249+ self.turnOnProfiling()
250+ self.pushProfilingConfig(memory_profile_log='.')
251 profile.start_request(self._get_start_event('/'))
252 self.assertIs(getattr(profile._profilers, 'profiler', None), None)
253 self.assertIsInstance(profile._profilers.memory_profile_start, tuple)
254@@ -198,8 +210,8 @@
255 self.assertEqual(profile._profilers.actions, set())
256
257 def test_combo_memory_and_profile_start(self):
258- self.pushProfilingConfig(
259- profiling_allowed='True', memory_profile_log='.')
260+ self.turnOnProfiling()
261+ self.pushProfilingConfig(memory_profile_log='.')
262 profile.start_request(self._get_start_event('/++profile++show/'))
263 self.assertIsInstance(profile._profilers.profiler, BzrProfiler)
264 self.assertIsInstance(profile._profilers.memory_profile_start, tuple)
265@@ -267,10 +279,11 @@
266 #########################################################################
267 # Tests
268 def test_config_stops_profiling(self):
269- """The ``profiling_allowed`` configuration should disable all
270+ """The ``request_profiling`` feature should disable all
271 profiling, even if it is requested"""
272+ self.turnOffProfiling()
273 self.pushProfilingConfig(
274- profiling_allowed='False', profile_all_requests='True',
275+ profile_all_requests='True',
276 memory_profile_log=self.memory_profile_log)
277 request = self.endRequest('/++profile++show,log')
278 self.assertIs(getattr(request, 'oops', None), None)
279@@ -282,7 +295,7 @@
280 def test_optional_profiling_without_marked_request_has_no_profile(self):
281 """Even if profiling is allowed, it does not happen with a normal
282 request."""
283- self.pushProfilingConfig(profiling_allowed='True')
284+ self.turnOnProfiling()
285 request = self.endRequest('/')
286 self.assertIs(getattr(request, 'oops', None), None)
287 self.assertEqual(self.getAddedResponse(request), '')
288@@ -293,7 +306,7 @@
289 def test_optional_profiling_with_show_request_profiles(self):
290 """If profiling is allowed and a request with the "show" marker
291 URL segment is made, profiling starts."""
292- self.pushProfilingConfig(profiling_allowed='True')
293+ self.turnOnProfiling()
294 request = self.endRequest('/++profile++show/')
295 self.assertIsInstance(request.oops, ErrorReport)
296 self.assertIn('Top Inline Time', self.getAddedResponse(request))
297@@ -304,7 +317,7 @@
298 def test_optional_profiling_with_log_request_profiles(self):
299 """If profiling is allowed and a request with the "log" marker
300 URL segment is made, profiling starts."""
301- self.pushProfilingConfig(profiling_allowed='True')
302+ self.turnOnProfiling()
303 request = self.endRequest('/++profile++log/')
304 self.assertIsInstance(request.oops, ErrorReport)
305 response = self.getAddedResponse(request)
306@@ -319,7 +332,7 @@
307 def test_optional_profiling_with_combined_request_profiles(self):
308 """If profiling is allowed and a request with the "log" and
309 "show" marker URL segment is made, profiling starts."""
310- self.pushProfilingConfig(profiling_allowed='True')
311+ self.turnOnProfiling()
312 request = self.endRequest('/++profile++log,show')
313 self.assertIsInstance(request.oops, ErrorReport)
314 response = self.getAddedResponse(request)
315@@ -333,8 +346,8 @@
316
317 def test_forced_profiling_logs(self):
318 """profile_all_requests should register as a log action"""
319- self.pushProfilingConfig(
320- profiling_allowed='True', profile_all_requests='True')
321+ self.turnOnProfiling()
322+ self.pushProfilingConfig(profile_all_requests='True')
323 request = self.endRequest('/')
324 self.assertIsInstance(request.oops, ErrorReport)
325 response = self.getAddedResponse(request)
326@@ -351,7 +364,7 @@
327 """If profiling is allowed and a request with the marker URL segment
328 is made incorrectly, profiling does not start and help is an action.
329 """
330- self.pushProfilingConfig(profiling_allowed='True')
331+ self.turnOnProfiling()
332 request = self.endRequest('/++profile++')
333 self.assertIs(getattr(request, 'oops', None), None)
334 response = self.getAddedResponse(request)
335@@ -365,8 +378,8 @@
336 """If profiling is forced and a request with the marker URL segment
337 is made incorrectly, profiling starts and help is an action.
338 """
339- self.pushProfilingConfig(
340- profiling_allowed='True', profile_all_requests='True')
341+ self.turnOnProfiling()
342+ self.pushProfilingConfig(profile_all_requests='True')
343 request = self.endRequest('/++profile++')
344 self.assertIsInstance(request.oops, ErrorReport)
345 response = self.getAddedResponse(request)
346@@ -382,9 +395,8 @@
347
348 def test_memory_profile(self):
349 "Does the memory profile work?"
350- self.pushProfilingConfig(
351- profiling_allowed='True',
352- memory_profile_log=self.memory_profile_log)
353+ self.turnOnProfiling()
354+ self.pushProfilingConfig(memory_profile_log=self.memory_profile_log)
355 request = self.endRequest('/')
356 self.assertIs(getattr(request, 'oops', None), None)
357 self.assertEqual(self.getAddedResponse(request), '')
358@@ -400,9 +412,8 @@
359
360 def test_memory_profile_with_non_defaults(self):
361 "Does the memory profile work with an oops and pageid?"
362- self.pushProfilingConfig(
363- profiling_allowed='True',
364- memory_profile_log=self.memory_profile_log)
365+ self.turnOnProfiling()
366+ self.pushProfilingConfig(memory_profile_log=self.memory_profile_log)
367 request = self.endRequest('/++profile++show/no-such-file',
368 KeyError(), pageid='Foo')
369 log = self.getMemoryLog()
370@@ -413,9 +424,8 @@
371 self.assertCleanProfilerState()
372
373 def test_combo_memory_and_profile(self):
374- self.pushProfilingConfig(
375- profiling_allowed='True',
376- memory_profile_log=self.memory_profile_log)
377+ self.turnOnProfiling()
378+ self.pushProfilingConfig(memory_profile_log=self.memory_profile_log)
379 request = self.endRequest('/++profile++show/')
380 self.assertIsInstance(request.oops, ErrorReport)
381 self.assertIn('Top Inline Time', self.getAddedResponse(request))
382@@ -424,7 +434,7 @@
383 self.assertCleanProfilerState()
384
385 def test_profiling_oops_is_informational(self):
386- self.pushProfilingConfig(profiling_allowed='True')
387+ self.turnOnProfiling()
388 request = self.endRequest('/++profile++show/')
389 response = self.getAddedResponse(request)
390 self.assertIsInstance(request.oops, ErrorReport)
391@@ -433,7 +443,7 @@
392 self.assertCleanProfilerState()
393
394 def test_real_oops_trumps_profiling_oops(self):
395- self.pushProfilingConfig(profiling_allowed='True')
396+ self.turnOnProfiling()
397 request = self.endRequest('/++profile++show/no-such-file',
398 KeyError('foo'))
399 self.assertIsInstance(request.oops, ErrorReport)
400@@ -444,7 +454,7 @@
401 self.assertCleanProfilerState()
402
403 def test_oopsid_is_in_profile_filename(self):
404- self.pushProfilingConfig(profiling_allowed='True')
405+ self.turnOnProfiling()
406 request = self.endRequest('/++profile++log/')
407 self.assertIn("-" + request.oopsid + "-", self.getProfilePaths()[0])
408 self.assertCleanProfilerState()