launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00314
[Merge] lp:~mbp/launchpad/flags-webapp into lp:launchpad
Martin Pool has proposed merging lp:~mbp/launchpad/flags-webapp into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Further development of feature flags:
* register a FeatureController for the duration of a web request
* log active features and scopes at the bottom of the base page template
* add per_thread FeatureController
* add 'features' and 'feature_scopes' macros for page templates
* developer documentation for feature flags
* feature flags only run their query when they are first accessed, and only look up scopes when necessary, in case the scope checks are expensive
Depends on feature changes not yet merged to devel, therefore targeted to db-devel.
--
https://code.launchpad.net/~mbp/launchpad/flags-webapp/+merge/31122
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mbp/launchpad/flags-webapp into lp:launchpad.
=== modified file 'lib/canonical/configure.zcml'
--- lib/canonical/configure.zcml 2010-07-24 00:07:54 +0000
+++ lib/canonical/configure.zcml 2010-07-28 07:31:22 +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/lp/app/templates/base-layout.pt'
--- lib/lp/app/templates/base-layout.pt 2010-07-23 15:03:41 +0000
+++ lib/lp/app/templates/base-layout.pt 2010-07-28 07:31:22 +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,8 @@
<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 +184,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-07-28 07:31:22 +0000
@@ -5,6 +5,26 @@
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-07-28 07:31:22 +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-07-28 07:31:22 +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-07-28 07:31:22 +0000
@@ -1,17 +1,46 @@
# 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',
+ ]
+
__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 +63,86 @@
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):
+ 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)
=== 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-07-28 07:31:22 +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,16 @@
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 +62,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 +114,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-07-28 07:31:22 +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
Follow ups
