Page MenuHomePhabricator

Provide basic FeatureManagement in Vector codebase
Open, HighPublic8 Estimate Story Points

Description

Task

In order to easily manage DesktopImprovements feature, provide a simple mechanism that allows engineers to:

  • easily register new features
  • allow admins to enable/disable feature by editing well defined configuration
  • allow user to opt-in / opt-out to Desktop Improvemends project

Existing solution

The similar work is already done in MobileFrontend extension. Although that code has couple problems that we would like to solve before porting it:

  • ServiceWiring should not depend upon RequestContext::getMain() as we found out that API contexts can be different
  • Too many objects provide isEnabled() method and it's difficult to understand when to use which
  • Code should work as a lib, therefore it shouldn't not assume any environment/request. In order to verify if Feature is available, we need to pass IContextSource to the check method.
  • There is no clear place where the FeatureManager is Initialized, it triggers on the first $mediaWikiServices->getService('FeatureManager') therefore it leads to unexpected behavior when calling hooks
  • Existing solution is pretty verbose, creating single Feature requires us to define name, config variable, group. It provides getNameKey and getDescriptionKey for translations. The Feature interface should be

simple and provide what's required. For example, The feature has no notion of the ResourceLoaderModule/Styles, therefore, when implementing Feature, in one place we had to register it, and in the second place
we had to do if ($featureManager->isAvailableInCurrentContrext( $featureName ) ) { $out->addModules( ... ) }. That is bad, as it doesn't feel like FeatureManager doesn't solve any problem, we could just do
sth like if (!empty(array_filter($config->get( "wg$featureName" )) { $out->addModules(...); } and get almost the same thing.

Proposal:

Provide 2 interfaces (FMS stands for FeatureManagementSystem):

  • FMS/IFeature - definition of a single feature
  • FMS/ISet - definition of system mode
  • FMS/IUserSelectableSet set that extends ISet, and additionally can be enabled/disabled by user

and FeatureManager class that binds everything together.

The FeatureManager should provide:

Setters:
registerFeature( $name, IFeature ) to register new feature
registerSet( $name, $ISet ) to register new set
switchSetState( $setName, $newState, IContextSource ) - allow user to switch the set state (only for opt-in/opt-out states )

Getters:
hasFeature( $featureName, IContextSource ) to check if user has access to given Feature
hasSet( $setName, IContextSource ) to check if has access (granted and/or enabled by himself ) to given set
getAllForSet( $setName, IContextSource) to fetch all features assigned to given set
getFeaturesForContext( IContextSource ) to fetch all available features in current context

Additionally it should trigger a hook FeatureManagerInitialization( $featureManager ) where different extensions could register it's own Features/Sets

Built in sets:

  • Stable -> the default mode for all features enabled for all users
  • Loggedin -> the feature set available only for logged-in users

Additionally provide:

  • DesktopImprovements -> the feature set for new DesktopImprovents project

Please, note that some sets, like LoggedIn are granted by system, and other sets like DesktopImprovements have default state that consists of enabled (stored in config) and opted_in stored in UserPreferences.
While implementing the system try to keep it as much compatible with MobileFrontend/FeatureManager as possible. Thanks to that we should move the MobileFrontend features into one system.

The initial delivery should include Legacy and Modern modes. These should be overrideable via a useskinversion URL query parameter (see T242381#5884415) in the feature dependency. The functionality to test query parameters should be available to arbitrary feature dependencies as well. For example, if you wanted to use the existing MobileFrontend feature manager to override the page issues treatment, the URL could be passed as a dependency of the MinervaPageIssuesNewTreatment feature and tested within.

Details

Related Gerrit Patches:
mediawiki/skins/Vector : masterMake SkinVector::isLegacy use the Feature Manager
mediawiki/skins/Vector : master[dev] add skin version query parameter override
mediawiki/skins/Vector : master[docs] [dev] [PHP] [FeatureManager] revise docs and add DynamicConfigRequirement test
mediawiki/skins/Vector : masterfeatureManager: Add DynamicConfigRequirement requirement
mediawiki/skins/Vector : masterfeatureManager: Pass context to Requirement::isMet
mediawiki/skins/Vector : masterfeatureManager: Add Requirement interface
mediawiki/skins/Vector : masterfeatureManager: Add typehints
mediawiki/skins/Vector : masterfeatureManager: Set -> Requirement
mediawiki/skins/Vector : masterBump required MediaWiki version
mediawiki/skins/Vector : masterfeatures: Add minimalist feature manager

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 6 2020, 1:23 PM
ovasileva triaged this task as High priority.Feb 6 2020, 1:27 PM
ovasileva added a subscriber: ovasileva.

Whoo! Thanks @polishdeveloper :)

nray added a subscriber: nray.Feb 6 2020, 11:26 PM
phuedx added a subscriber: phuedx.Feb 7 2020, 1:01 PM

hasSet( $setName, IContextSource ) to check if has access (granted and/or enabled by himself ) to given set

👍 This will help us avoiding instantiating a mode to check if the user is in it, e.g.

FMS/ISet - definition of system mode

The old term "mode" and the proposed term "set" don't feel right to me but I'm struggling to think of a better suggestion. Essentially, the feature manager is a sophisticated array_filter so perhaps something like IFilter, IFeatureTest, or ICheck? I prefer this verbiage because it's not tied up in the idea of a site mode or some condition of the user, e.g. being in a certain edit count bucket.

@phuedx yeah, I agree, the ISet is pretty problematic, I spent some time ago thinking on it, and my idea is that we could define sets of features per context. So for example, the mode could be:

  • user opted into DesktopImprovements project or AMC mode
  • user is logged-in or logged-out
  • user is viewing a page from a given namepace -- define set of features that are available for given namespace
  • user has more than 100 edits
  • an A/B test that based on some criteria buckets user and returns if given set is available or not.
  • user belongs to an ongoing experiment
  • a composite - for example only 100 edits that visits a specific namespace $set = new CompositeSet( [ new VisitingNamespace ( NS_TALK ), new ContributorEditsCheck( 100 ) )

Those modes can be:

  • some of those are selectable by user
  • some modes are assigned by system (bucketing)
  • some modes are set based on the current context (being logged in, viewing specific namespace)
  • visibility of some modes will be defined in the config (like AMC)

We could go on with possible scenarios. Currently, we use IUerMode, which makes sense for user selectable modes like Beta/AMC. But it doesn't make sense for cases like "AdvancedEditor", or "A/B test". I considered names like that:

  • IUserMode -> just keep the existing name
  • ISet -> because it's a feature set, right ? I also considered IFeatureSet, but then I thought, that IFeatureSet can be just a composite of multiple features, like for example if two features have to go together in some cases, then we could make a composite new FeatureSet( $featureA, $featureB ).
  • IScenario -> a specific scenario for user, but somehow that name doesn't stick
  • ISiteMode -> because at the end it is a site mode for that single context (title,namespace, user), but, on different page view user can have different ISideMode set, that might be consfusing.
  • IFeatureContext -> kinda makes sense, but at the end the FeatureContextdoesn't explain what it really is, and can be misunderstood that it is a IContextSource

    The ICheck and IFilter also sounds promising. I agree that both IUserMode and ISet are not the best names, and I'm also struggling to find a better name.

@Niedzielski @Jdrewniak @nray @Jdlrobson what do you think? Do you have any suggestions?

@polishdeveloper, looks good! Just reframing the above into concepts to make sure I got it:

  • Dependency: this might be the state of another object in the system like login state for a User, a site configuration, an A/B test bucket, a URL, a namespace, user edits, an extension, a service, or something else. These are the true, raw (dynamic) dependency inputs into the feature manager. These raw dependencies should never be interacted with directly anywhere except the feature manager.
  • Feature: this is an output of a feature manager. Given the proper raw dependency prerequisites, a feature is made available or unavailable. Clients of the feature manager are welcome to directly check for feature availability inline but frequently used sets of features should be formalized as a mode and use polymorphic behavior rather than conditionals. Lightweight / simple features can still be simple conditionals as is practical.
  • Mode: an all or nothing base feature set that is big enough to have it's own name. I think "advanced mode" / AMC mode is a good example where initially it may only be the logged in feature but grows to include a few more features like advanced menu, advanced page menu, and advanced special pages. There can still also be additional optional features that are specific to a mode, e.g., an on / off A/B test for search. I guess you could have multiple modes but I don't know what the use cases for that are.

Modes should probably generally make sense at the user level and be the most tested software configurations. There will also be specific optional features we'd want to test per mode, of course. That is, we wouldn't test every combination of feature. Only mode + certain features on / off based on the rollout strategy.

Demian added a subscriber: Demian.Feb 7 2020, 11:20 PM

As discussed:

Can the IContextSource parameter of IFeatureManager#hasFeature, #hasSet, #getAllForSet, and #getFeaturesForContext be optional? My thinking here is that interactions with the feature management system should be as natural as possible. It's not unreasonable to expect the feature management system to be able to guess what the most suitable context is, i.e.

FeatureManager.php
class FeatureManager implements IFeatureManager {
  private function getDefaultContext() : IContextSource {
    if ( defined( 'MW_API' ) ) {
      /* ... */
    }

    return RequestContext::getMain();
  }
}
Niedzielski updated the task description. (Show Details)Feb 14 2020, 6:07 PM
Restricted Application added a subscriber: Masumrezarock100. · View Herald TranscriptFeb 14 2020, 6:07 PM
Niedzielski updated the task description. (Show Details)Feb 14 2020, 6:08 PM
nray updated the task description. (Show Details)Feb 14 2020, 6:22 PM
nray added a comment.Feb 14 2020, 9:48 PM

a composite - for example only 100 edits that visits a specific namespace $set = new CompositeSet( [ new VisitingNamespace ( NS_TALK ), new ContributorEditsCheck( 100 ) )

I like the idea of having composable "modes" (I have an opinion on the naming for this below). I've been thinking about it more and more ever since @phuedx mentioned it in https://phabricator.wikimedia.org/T237635#5856174, and I think it will be critical if we want to eventually migrate this to core to avoid a bunch of one-off modes that only one feature uses (e.g. AMCPowerUserTalkPageNotABotMode) and that are tedious to setup.

Those modes can be:

  • some of those are selectable by user
  • some modes are assigned by system (bucketing)
  • some modes are set based on the current context (being logged in, viewing specific namespace)
  • visibility of some modes will be defined in the config (like AMC)

We could go on with possible scenarios. Currently, we use IUerMode, which makes sense for user selectable modes like Beta/AMC. But it doesn't make sense for cases like "AdvancedEditor", or "A/B test". I considered names like that:

  • IUserMode -> just keep the existing name
  • ISet -> because it's a feature set, right ? I also considered IFeatureSet, but then I thought, that IFeatureSet can be just a composite of multiple features, like for example if two features have to go together in some cases, then we could make a composite new FeatureSet( $featureA, $featureB ).
  • IScenario -> a specific scenario for user, but somehow that name doesn't stick
  • ISiteMode -> because at the end it is a site mode for that single context (title,namespace, user), but, on different page view user can have different ISideMode set, that might be consfusing.
  • IFeatureContext -> kinda makes sense, but at the end the FeatureContextdoesn't explain what it really is, and can be misunderstood that it is a IContextSource The ICheck and IFilter also sounds promising. I agree that both IUserMode and ISet are not the best names, and I'm also struggling to find a better name.

@Niedzielski @Jdrewniak @nray @Jdlrobson what do you think? Do you have any suggestions?

I don't like the term UserMode as I think it's too restricting, but I find the term Mode to be more intuitive than Set to describe these things. I also suggest the terms Condition be considered as I think all of these things describe conditions that are either satisfied or not (like a conditional). IConditions are composable and can be combined together (also like a conditional). A feature is only "on" when all of the IConditions are true.

Additionally, it might be possible for an ICondition to accept params. I'm not sure how feasible this is, but I could imagine the config for this to be like:

$wgFancyNewFeature = [
  "userModes" => [ 'amc' ],
  "userEdits" => 100,
  "namespaces" => [ 'NS_TALK' ]
];

In this example, there would be three different classes that implement ICondition: UserModes, UserEdits, Namespaces. "FancyNewFeature" is only "on" when these three IConditions have been satisfied. Note that each ICondition could be used independently and that each ICondition can also accept configurable params both of which promote reusability by other features .

Niedzielski set the point value for this task to 8.Feb 19 2020, 4:01 PM

Estimated asynchronously at an extra-large.

Change 573626 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/skins/Vector@master] features: Add minimalist feature manager

https://gerrit.wikimedia.org/r/573626

@polishdeveloper: Sorry for the lack of context for the change above. There were/are a lot of good questions about the design of the feature management system (FMS) that emerged during review. Given the need to land an FMS in Vector soon, I proposed that we land the simplest version of your proposal that we could all agree on. Included in my patch is a list of steps that gets us from point A to what's described in this task.

Change 573626 merged by jenkins-bot:
[mediawiki/skins/Vector@master] features: Add minimalist feature manager

https://gerrit.wikimedia.org/r/573626

Change 575037 had a related patch set uploaded (by Polishdeveloper; owner: Polishdeveloper):
[mediawiki/skins/Vector@master] AutoloadNamespaces required MW 1.31

https://gerrit.wikimedia.org/r/575037

Change 575037 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Bump required MediaWiki version

https://gerrit.wikimedia.org/r/575037

@Niedzielski @phuedx can this be QAed by Edward or should it move to engineering sign off?

@Jdlrobson, only the initial patch has been merged so far. I believe @phuedx is planning one or more subsequent patches as part of this ticket.

ovasileva removed Edtadros as the assignee of this task.Mar 2 2020, 11:07 AM
ovasileva added a subscriber: Edtadros.

Change 576122 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/skins/Vector@master] featureManager: Set -> Requirement

https://gerrit.wikimedia.org/r/576122

Change 576123 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/skins/Vector@master] featureManager: Add typehints

https://gerrit.wikimedia.org/r/576123

Change 576124 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/skins/Vector@master] featureManager: Add Requirement interface

https://gerrit.wikimedia.org/r/576124

Change 576125 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/skins/Vector@master] featureManager: Pass context to Requirement::isMet

https://gerrit.wikimedia.org/r/576125

Change 576122 merged by jenkins-bot:
[mediawiki/skins/Vector@master] featureManager: Set -> Requirement

https://gerrit.wikimedia.org/r/576122

@polishdeveloper I've rebased all of the unmerged patches. Thanks for your review!

Change 576123 merged by jenkins-bot:
[mediawiki/skins/Vector@master] featureManager: Add typehints

https://gerrit.wikimedia.org/r/576123

Change 576124 merged by jenkins-bot:
[mediawiki/skins/Vector@master] featureManager: Add Requirement interface

https://gerrit.wikimedia.org/r/576124

Change 579619 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/skins/Vector@master] featureManager: Add FullyInitialised requirement

https://gerrit.wikimedia.org/r/579619

Change 579619 merged by jenkins-bot:
[mediawiki/skins/Vector@master] featureManager: Add DynamicConfigRequirement requirement

https://gerrit.wikimedia.org/r/579619

Change 583441 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/Vector@master] [docs] [dev] [PHP] [FeatureManager] revise docs and add DynamicConfigRequirement test

https://gerrit.wikimedia.org/r/583441

Change 583441 merged by jenkins-bot:
[mediawiki/skins/Vector@master] [docs] [dev] [PHP] [FeatureManager] revise docs and add DynamicConfigRequirement test

https://gerrit.wikimedia.org/r/583441

Change 585049 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/Vector@master] [dev] add skin version query parameter override

https://gerrit.wikimedia.org/r/585049

Change 585291 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/skins/Vector@master] [WIP] Make SkinVector::isLegacy use the Feature Manager

https://gerrit.wikimedia.org/r/585291

Change 585049 merged by jenkins-bot:
[mediawiki/skins/Vector@master] [dev] add skin version query parameter override

https://gerrit.wikimedia.org/r/585049

phuedx added a comment.Fri, Apr 3, 5:02 PM

A brief update, with https://gerrit.wikimedia.org/r/585049 merged, I'm working on making it compatible with the Feature Manager.