launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00624
[Merge] lp:~mbp/launchpad/flags-webapp into lp:launchpad/devel
Martin Pool has proposed merging lp:~mbp/launchpad/flags-webapp into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Since <https://code.edge.launchpad.net/~mbp/launchpad/flags-webapp/+merge/31122> was accepted into db-devel, I'd like to also land it into devel, since the db dependencies are now there.
--
https://code.launchpad.net/~mbp/launchpad/flags-webapp/+merge/32833
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mbp/launchpad/flags-webapp into lp:launchpad/devel.
=== modified file 'lib/canonical/configure.zcml'
--- lib/canonical/configure.zcml 2010-07-24 00:07:54 +0000
+++ lib/canonical/configure.zcml 2010-08-17 02:31:17 +0000
@@ -1,4 +1,4 @@
-<!-- Copyright 2009 Canonical Ltd. This software is licensed under the
+<!-- Copyright 2009, 2010 Canonical Ltd. This software is licensed under the
GNU Affero General Public License version 3 (see the file LICENSE).
-->
@@ -21,6 +21,7 @@
<include package="lp.services.job" />
<include package="lp.services.memcache" />
<include package="lp.services.profile" />
+ <include package="lp.services.features" />
<include package="lp.services.scripts" />
<include package="lp.services.worlddata" />
<include package="lp.services.salesforce" />
=== modified file 'lib/canonical/launchpad/webapp/servers.py'
--- lib/canonical/launchpad/webapp/servers.py 2010-08-02 02:23:26 +0000
+++ lib/canonical/launchpad/webapp/servers.py 2010-08-17 02:31:17 +0000
@@ -79,6 +79,10 @@
from canonical.lazr.timeout import set_default_timeout_function
+from lp.services.features.flags import (
+ NullFeatureController,
+ )
+
class StepsToGo:
"""
@@ -838,6 +842,9 @@
self.needs_datetimepicker_iframe = False
self.needs_json = False
self.needs_gmap2 = False
+ # stub out the FeatureController that would normally be provided by
+ # the publication mechanism
+ self.features = NullFeatureController()
@property
def uuid(self):
=== modified file 'lib/lp/app/browser/tests/base-layout.txt'
--- lib/lp/app/browser/tests/base-layout.txt 2010-08-04 13:38:22 +0000
+++ lib/lp/app/browser/tests/base-layout.txt 2010-08-17 02:31:17 +0000
@@ -135,10 +135,11 @@
Has application tabs: False
Has side portlets: False
At least ... queries issued in ... seconds
+ Features: {}
+ in scopes {}
r...
-->
-
Page Headings
-------------
=== modified file 'lib/lp/app/templates/base-layout.pt'
--- lib/lp/app/templates/base-layout.pt 2010-08-06 15:40:39 +0000
+++ lib/lp/app/templates/base-layout.pt 2010-08-17 02:31:17 +0000
@@ -14,6 +14,8 @@
site_message modules/canonical.config/config/launchpad/site_message;
icingroot string:${rooturl}+icing/rev${revno};
icingroot_contrib string:${rooturl}+icing-contrib/rev${revno};
+ features request/features;
+ feature_scopes request/features/scopes;
CONTEXTS python:{'template':template, 'context': context, 'view':view};
"
><metal:doctype define-slot="doctype"><tal:doctype tal:replace="structure string:<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">"; /></metal:doctype>
@@ -171,7 +173,7 @@
<tal:template>
<tal:comment
- define="log modules/canonical.launchpad.webapp.adapter/summarize_requests"
+ define="log modules/canonical.launchpad.webapp.adapter/summarize_requests;"
replace="structure string:<!--
Facet name: ${view/menu:selectedfacetname}
Page type: ${view/macro:pagetype}
@@ -181,6 +183,9 @@
At least ${log}
+ Features: ${request/features/usedFlags}
+ in scopes ${request/features/usedScopes}
+
r${revno}
-->" />
</tal:template>
=== modified file 'lib/lp/services/features/__init__.py'
--- lib/lp/services/features/__init__.py 2010-07-21 11:05:14 +0000
+++ lib/lp/services/features/__init__.py 2010-08-17 02:31:17 +0000
@@ -5,6 +5,25 @@
These can be turned on and off by admins, and can affect particular
defined scopes such as "beta users" or "production servers."
-
-See <https://dev.launchpad.net/LEP/FeatureFlags>
-"""
+"""
+
+import threading
+
+
+__all__ = [
+ 'getFeatureFlag',
+ 'per_thread',
+ ]
+
+
+per_thread = threading.local()
+"""Holds the default per-thread feature controller in its .features attribute.
+
+Framework code is responsible for setting this in the appropriate context, eg
+when starting a web request.
+"""
+
+
+def getFeatureFlag(flag):
+ """Get the value of a flag for this thread's scopes."""
+ return per_thread.features.getFlag(flag)
=== added file 'lib/lp/services/features/configure.zcml'
--- lib/lp/services/features/configure.zcml 1970-01-01 00:00:00 +0000
+++ lib/lp/services/features/configure.zcml 2010-08-17 02:31:17 +0000
@@ -0,0 +1,19 @@
+<!-- Copyright 2010 Canonical Ltd. This software is licensed under the
+ GNU Affero General Public License version 3 (see the file LICENSE).
+-->
+
+<configure
+ xmlns="http://namespaces.zope.org/zope";
+ xmlns:browser="http://namespaces.zope.org/browser";>
+
+ <subscriber
+ for="canonical.launchpad.webapp.interfaces.IStartRequestEvent"
+ handler="lp.services.features.webapp.start_request"
+ />
+
+ <subscriber
+ for="zope.app.publication.interfaces.IEndRequestEvent"
+ handler="lp.services.features.webapp.end_request"
+ />
+
+</configure>
=== added directory 'lib/lp/services/features/doc'
=== added file 'lib/lp/services/features/doc/features.txt'
--- lib/lp/services/features/doc/features.txt 1970-01-01 00:00:00 +0000
+++ lib/lp/services/features/doc/features.txt 2010-08-17 02:31:17 +0000
@@ -0,0 +1,127 @@
+****************************
+Feature Flag Developer Guide
+****************************
+
+Introduction
+************
+
+The point of feature flags is to let us turn some features of Launchpad on
+and off without changing the code or restarting the application, and to
+expose different features to different subsets of users.
+
+See <https://dev.launchpad.net/LEP/FeatureFlags> for more discussion and
+rationale.
+
+The typical use for feature flags is within web page requests but they can
+also be used in asynchronous jobs or apis or other parts of Launchpad.
+
+Internal model for feature flags
+********************************
+
+A feature flag maps from a *name* to a *value*. The specific value used
+for a particular request is determined by a set of zero or more *scopes*
+that apply to that request, by finding the *rule* with the highest
+*priority*.
+
+Flags are defined by a *name* that typically looks like a Python
+identifier, for example ``notification.global.text``. A definition is
+given for a particular *scope*, which also looks like a dotted identifier,
+for example ``user.beta`` or ``server.edge``. This is just a naming
+convention, and they do not need to correspond to Python modules.
+
+The value is stored in the database as just a Unicode string, and it might
+be interpreted as a boolean, number, human-readable string or whatever.
+
+The default for flags is to be None if they're not set in the database, so
+that should be a sensible baseline default state.
+
+Performance model
+*****************
+
+Flags are supposed to be cheap enough that you can introduce them without
+causing a performance concern.
+
+If the page does not check any flags, no extra work will be done. The
+first time a page checks a flag, all the rules will be read from the
+database and held in memory for the duration of the request.
+
+Scopes may be expensive in some cases, such as checking group membership.
+Whether a scope is active or not is looked up the first time it's needed
+within a particular request.
+
+The standard page footer identifies the flags and scopes that were
+actually used by the page.
+
+Naming conventions
+******************
+
+We have naming conventions for feature flags and scopes, so that people can
+understand the likely impact of a particular flag and so they can find all
+the flags likely to affect a feature.
+
+So for any flag we want to say:
+
+* What application area does this affect? (malone, survey, questions,
+ code, etc)
+
+* What specific feature does it change?
+
+* What affect does it have on this feature? The most common is "enabled"
+ but for some other we want to specify a specific value as well such as
+ "date" or "size".
+
+These are concatenated with dots so the overall feature name looks a bit
+like a Python module name.
+
+A similar approach is used for scopes.
+
+Checking flags in page templates
+********************************
+
+You can conditionally show some text like this::
+
+ <tal:survey condition="features/user_survey.enabled">
+ •
+ <a href="http://survey.example.com/";>Take our survey!</a>
+ </tal:survey>
+
+You can use the built-in TAL feature of prepending ``not:`` to the
+condition, and for flags that have a value you could use them in
+``tal:replace`` or ``tal:attributes``.
+
+If you just want to simply insert some text taken from a feature, say
+something like::
+
+ Message of the day: ${motd.text}
+
+Templates can also check whether the request is in a particular scope, but
+before using this consider whether the code will always be bound to that
+scope or whether it would be more correct to define a new feature::
+
+ <p tal:condition="feature_scopes/server.staging">
+ Staging server: all data will be discarded daily!</p>
+
+Checking flags in code
+**********************
+
+The Zope traversal code establishes a `FeatureController` for the duration
+of a request. The object can be obtained through either
+`request.features` or `lp.services.features.per_thread.features`. This
+provides various useful methods including `getFlag` to look up one feature
+(memoized), and `isInScope` to check one scope (also memoized).
+
+As a convenience, `lp.services.features.getFeatureFlag` looks up a single
+flag in the thread default controller.
+
+Debugging feature usage
+***********************
+
+The flags active during a page request, and the scopes that were looked
+up are visible in the comment at the bottom of every standard Launchpad
+page.
+
+Defining scopes
+***************
+
+
+.. vim: ft=rst
=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py 2010-07-22 12:45:51 +0000
+++ lib/lp/services/features/flags.py 2010-08-17 02:31:17 +0000
@@ -1,17 +1,47 @@
# Copyright 2010 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
-__all__ = ['FeatureController']
+__all__ = [
+ 'FeatureController',
+ 'NullFeatureController',
+ ]
+
__metaclass__ = type
+from storm.locals import Desc
+
from lp.services.features.model import (
+ FeatureFlag,
getFeatureStore,
- FeatureFlag,
)
+class Memoize(object):
+
+ def __init__(self, calc):
+ self._known = {}
+ self._calc = calc
+
+ def lookup(self, key):
+ if key in self._known:
+ return self._known[key]
+ v = self._calc(key)
+ self._known[key] = v
+ return v
+
+
+class ScopeDict(object):
+ """Allow scopes to be looked up by getitem"""
+
+ def __init__(self, features):
+ self.features = features
+
+ def __getitem__(self, scope_name):
+ return self.features.isInScope(scope_name)
+
+
class FeatureController(object):
"""A FeatureController tells application code what features are active.
@@ -34,52 +64,101 @@
be one per web app request.
Intended performance: when this object is first constructed, it will read
- the whole current feature flags from the database. This will take a few
- ms. The controller is then supposed to be held in a thread-local for the
- duration of the request.
+ the whole feature flag table from the database. It is expected to be
+ reasonably small. The scopes may be expensive to compute (eg checking
+ team membership) so they are checked at most once when they are first
+ needed.
+
+ The controller is then supposed to be held in a thread-local and reused
+ for the duration of the request.
See <https://dev.launchpad.net/LEP/FeatureFlags>
"""
- def __init__(self, scopes):
+ def __init__(self, scope_check_callback):
"""Construct a new view of the features for a set of scopes.
- """
- self._store = getFeatureStore()
- self._scopes = self._preenScopes(scopes)
- self._cached_flags = self._queryAllFlags()
-
- def getScopes(self):
- return frozenset(self._scopes)
-
- def getFlag(self, flag_name):
- return self._cached_flags.get(flag_name)
+
+ :param scope_check_callback: Given a scope name, says whether
+ it's active or not.
+ """
+ self._known_scopes = Memoize(scope_check_callback)
+ self._known_flags = Memoize(self._checkFlag)
+ # rules are read from the database the first time they're needed
+ self._rules = None
+ self.scopes = ScopeDict(self)
+
+ def getFlag(self, flag):
+ """Get the value of a specific flag.
+
+ :param flag: A name to lookup. e.g. 'recipes.enabled'
+ :return: The value of the flag determined by the highest priority rule
+ that matched.
+ """
+ return self._known_flags.lookup(flag)
+
+ def _checkFlag(self, flag):
+ self._needRules()
+ if flag in self._rules:
+ for scope, value in self._rules[flag]:
+ if self._known_scopes.lookup(scope):
+ return value
+
+ def isInScope(self, scope):
+ return self._known_scopes.lookup(scope)
+
+ def __getitem__(self, flag_name):
+ """FeatureController can be indexed.
+
+ This is to support easy zope traversal through eg
+ "request/features/a.b.c". We don't support other collection
+ protocols.
+
+ Note that calling this the first time for any key may cause
+ arbitrarily large amounts of work to be done to determine if the
+ controller is in any scopes relevant to this flag.
+ """
+ return self.getFlag(flag_name)
def getAllFlags(self):
- """Get the feature flags active for the current scopes.
+ """Return a dict of all active flags.
- :returns: dict from flag_name (unicode) to value (unicode).
+ This may be expensive because of evaluating many scopes, so it
+ shouldn't normally be used by code that only wants to know about one
+ or a few flags.
"""
- return dict(self._cached_flags)
+ self._needRules()
+ return dict((f, self.getFlag(f)) for f in self._rules)
- def _queryAllFlags(self):
+ def _loadRules(self):
+ store = getFeatureStore()
d = {}
- rs = (self._store
- .find(FeatureFlag,
- FeatureFlag.scope.is_in(self._scopes))
- .order_by(FeatureFlag.priority)
- .values(FeatureFlag.flag, FeatureFlag.value))
- for flag, value in rs:
- d[str(flag)] = value
+ rs = (store
+ .find(FeatureFlag)
+ .order_by(Desc(FeatureFlag.priority))
+ .values(FeatureFlag.flag, FeatureFlag.scope,
+ FeatureFlag.value))
+ for flag, scope, value in rs:
+ d.setdefault(str(flag), []).append((str(scope), value))
return d
- def _preenScopes(self, scopes):
- # for convenience turn strings to unicode
- us = []
- for s in scopes:
- if isinstance(s, unicode):
- us.append(s)
- elif isinstance(s, str):
- us.append(unicode(s))
- else:
- raise TypeError("invalid scope: %r" % s)
- return us
+ def _needRules(self):
+ if self._rules is None:
+ self._rules = self._loadRules()
+
+ def usedFlags(self):
+ """Return dict of flags used in this controller so far."""
+ return dict(self._known_flags._known)
+
+ def usedScopes(self):
+ """Return {scope: active} for scopes that have been used so far."""
+ return dict(self._known_scopes._known)
+
+
+class NullFeatureController(FeatureController):
+ """For use in testing: everything is turned off"""
+
+ def __init__(self):
+ FeatureController.__init__(self, lambda scope: None)
+
+ def _loadRules(self):
+ return []
=== modified file 'lib/lp/services/features/tests/test_flags.py'
--- lib/lp/services/features/tests/test_flags.py 2010-07-22 12:45:51 +0000
+++ lib/lp/services/features/tests/test_flags.py 2010-08-17 02:31:17 +0000
@@ -12,7 +12,11 @@
from canonical.testing import layers
from lp.testing import TestCase
-from lp.services.features import model
+from lp.services.features import (
+ getFeatureFlag,
+ model,
+ per_thread,
+ )
from lp.services.features.flags import (
FeatureController,
)
@@ -20,11 +24,10 @@
notification_name = 'notification.global.text'
notification_value = u'\N{SNOWMAN} stormy Launchpad weather ahead'
-example_scope = 'beta_user'
testdata = [
- (example_scope, notification_name, notification_value, 100),
+ ('beta_user', notification_name, notification_value, 100),
('default', 'ui.icing', u'3.0', 100),
('beta_user', 'ui.icing', u'4.0', 300),
]
@@ -40,6 +43,14 @@
from storm.tracer import debug
debug(True)
+ def makeControllerInScopes(self, scopes):
+ """Make a controller that will report it's in the given scopes."""
+ call_log = []
+ def scope_cb(scope):
+ call_log.append(scope)
+ return scope in scopes
+ return FeatureController(scope_cb), call_log
+
def populateStore(self):
store = model.getFeatureStore()
for (scope, flag, value, priority) in testdata:
@@ -49,37 +60,51 @@
value=value,
priority=priority))
- def test_defaultFlags(self):
- # the sample db has no flags set
- control = FeatureController([])
- self.assertEqual({},
- control.getAllFlags())
-
def test_getFlag(self):
self.populateStore()
- control = FeatureController(['default'])
+ control, call_log = self.makeControllerInScopes(['default'])
self.assertEqual(u'3.0',
control.getFlag('ui.icing'))
+ self.assertEqual(['beta_user', 'default'], call_log)
+
+ def test_getItem(self):
+ # for use in page templates, the flags can be treated as a dict
+ self.populateStore()
+ control, call_log = self.makeControllerInScopes(['default'])
+ self.assertEqual(u'3.0',
+ control['ui.icing'])
+ self.assertEqual(['beta_user', 'default'], call_log)
+ # after looking this up the value is known and the scopes are
+ # positively and negatively cached
+ self.assertEqual({'ui.icing': u'3.0'}, control.usedFlags())
+ self.assertEqual(dict(beta_user=False, default=True),
+ control.usedScopes())
def test_getAllFlags(self):
# can fetch all the active flags, and it gives back only the
- # highest-priority settings
+ # highest-priority settings. this may be expensive and shouldn't
+ # normally be used.
self.populateStore()
- control = FeatureController(['default', 'beta_user'])
+ control, call_log = self.makeControllerInScopes(
+ ['beta_user', 'default'])
self.assertEqual(
{'ui.icing': '4.0',
notification_name: notification_value},
control.getAllFlags())
+ # evaluates all necessary flags; in this test data beta_user shadows
+ # default settings
+ self.assertEqual(['beta_user'], call_log)
def test_overrideFlag(self):
# if there are multiple settings for a flag, and they match multiple
# scopes, the priorities determine which is matched
self.populateStore()
- default_control = FeatureController(['default'])
+ default_control, call_log = self.makeControllerInScopes(['default'])
self.assertEqual(
u'3.0',
default_control.getFlag('ui.icing'))
- beta_control = FeatureController(['default', 'beta_user'])
+ beta_control, call_log = self.makeControllerInScopes(
+ ['beta_user', 'default'])
self.assertEqual(
u'4.0',
beta_control.getFlag('ui.icing'))
@@ -87,9 +112,53 @@
def test_undefinedFlag(self):
# if the flag is not defined, we get None
self.populateStore()
- control = FeatureController(['default', 'beta_user'])
+ control, call_log = self.makeControllerInScopes(
+ ['beta_user', 'default'])
self.assertIs(None,
control.getFlag('unknown_flag'))
- no_scope_flags = FeatureController([])
+ no_scope_flags, call_log = self.makeControllerInScopes([])
self.assertIs(None,
no_scope_flags.getFlag('ui.icing'))
+
+ def test_threadGetFlag(self):
+ self.populateStore()
+ # the start-of-request handler will do something like this:
+ per_thread.features, call_log = self.makeControllerInScopes(
+ ['default', 'beta_user'])
+ try:
+ # then application code can simply ask without needing a context
+ # object
+ self.assertEqual(u'4.0', getFeatureFlag('ui.icing'))
+ finally:
+ per_thread.features = None
+
+ def testLazyScopeLookup(self):
+ # feature scopes may be a bit expensive to look up, so we do it only
+ # when it will make a difference to the result.
+ self.populateStore()
+ f, call_log = self.makeControllerInScopes(['beta_user'])
+ self.assertEqual(u'4.0', f.getFlag('ui.icing'))
+ # to calculate this it should only have had to check we're in the
+ # beta_users scope; nothing else makes a difference
+ self.assertEqual(dict(beta_user=True), f._known_scopes._known)
+
+ def testUnknownFeature(self):
+ # looking up an unknown feature gives you None
+ self.populateStore()
+ f, call_log = self.makeControllerInScopes([])
+ self.assertEqual(None, f.getFlag('unknown'))
+ # no scopes need to be checked because it's just not in the database
+ # and there's no point checking
+ self.assertEqual({}, f._known_scopes._known)
+ self.assertEquals([], call_log)
+ # however, this we have now negative-cached the flag
+ self.assertEqual(dict(unknown=None), f.usedFlags())
+ self.assertEqual(dict(), f.usedScopes())
+
+ def testScopeDict(self):
+ # can get scopes as a dict, for use by "feature_scopes/server.demo"
+ f, call_log = self.makeControllerInScopes(['beta_user'])
+ self.assertEqual(True, f.scopes['beta_user'])
+ self.assertEqual(False, f.scopes['alpha_user'])
+ self.assertEqual(True, f.scopes['beta_user'])
+ self.assertEqual(['beta_user', 'alpha_user'], call_log)
=== added file 'lib/lp/services/features/webapp.py'
--- lib/lp/services/features/webapp.py 1970-01-01 00:00:00 +0000
+++ lib/lp/services/features/webapp.py 2010-08-17 02:31:17 +0000
@@ -0,0 +1,41 @@
+# Copyright 2010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Connect Feature flags into webapp requests."""
+
+__all__ = []
+
+__metaclass__ = type
+
+import canonical.config
+
+from lp.services.features import (
+ per_thread,
+ )
+from lp.services.features.flags import (
+ FeatureController,
+ )
+
+
+class ScopesFromRequest(object):
+ """Identify feature scopes based on request state."""
+
+ def __init__(self, request):
+ self._request = request
+
+ def lookup(self, scope_name):
+ parts = scope_name.split('.')
+ if len(parts) == 2:
+ if parts[0] == 'server':
+ return canonical.config.config['launchpad']['is_' + parts[1]]
+
+
+def start_request(event):
+ """Register FeatureController."""
+ event.request.features = per_thread.features = FeatureController(
+ ScopesFromRequest(event.request).lookup)
+
+
+def end_request(event):
+ """Done with this FeatureController."""
+ event.request.features = per_thread.features = None
