Page MenuHomePhabricator

Discussion: Improving the way we use Feature Flagging
Closed, DuplicatePublic

Description

Lately when thinking about promoting stuff to stable, looking through the code, I've seen that the promotion tasks are not really as simple as changing the configuration, but also entail changing code, which make the promotions more prone to errors and forgetting about something in the codebase.

The reason usually is that we flag behaviour and styles around the codebase using config+mode checks, instead of just a single flag. (For example checking that $wmMFAnotherFeature is true and that $mode is beta in several places around the code base).

The result of such practice is that when we have to promote something from mode to mode, we have to grep the codebase changing code in many places, and get that merged + config change, instead of doing just a config change. Then if we want to roll back something, we have to revert the config change and also revert the changes to the codebase instead of just changing the config.

All this gets specially brittle when you add CSS to the mix, where I've seen namespaced CSS groups of rules with the .beta class, that are not even clear to which feature they belong, and that are not feature flagged at all, just mode namespaced. Those are at high risk of being forgotten and rot there forever, or breaking the feature on rollout because of not being promoted, or increasing the complexity of promotions and rollbacks.

I think we need to align as a development practice and try feature flagging differently

My proposal is to stop using the mode in the code base, and just rely on a config variable when in the code we need to check if a feature is enabled (instead of $wmMFAnotherFeature is true and that mode is beta, we just check that $wmMFAnotherFeature is true).

In CSS, we should have flag classes added to the body or html element, and feature flag our CSS changes accordingly to the feature flag instead of the mode in most cases (.wmMFAnotherFeature { .breaking-changes {...} } instead of .beta { .breaking-changes {...} }).

Then, we should find a single point in the codebase where we transform configuration variables into those feature flags, so that there is a single point where we check mode + config + other factors and turn it into an enabled or disabled feature flag.

For example if there is a config variable like this:

$wmMFAnotherFeature = array(
  'stable' => false,
  'beta' => true
);

Then in just one place of the codebase we transform $wmMFAnotherFeature[$mode] into the flag.

That flag will be used by php and js for doing or not stuff, and it will be added to the body class to enable or disable the feature (by php there may be caching concerns, by JS there may be FOUCs, we need to discuss).

For config variables for features that are in a mode and we foresee promotion or rollback that are global and mode independent (so that they force us to hardcode the relationship between enablement and the mode in the code itself) like this:

$wmMFAnotherChange = true;

We should migrate the config format to support the rest of the factors to take into account for having it enabled or disabled, any of these would work:

$wmMFAnotherChange = true;
$wmMFAnotherModes = ['beta'];
$wmMFAnotherChange = array(
  'stable' => true,
  'beta' => true
);
// Imaginary bucketing
$wmMFAnotherChange = array(
  'stable' => 0.001
  'beta' => 1
);

With this changes in place, disabling/enabling/rolling-back a feature (per wiki and by default) per mode or other factors just should become a config change, so no change to the codebase would be required, and less risk to leave pieces behind or break things on promotion.


Thoughts? Is this sensible?

Do we want to do it?

Is there something already existing to do this?

Would it be an interesting extension to develop for the ecosystem? (feature flagging support with progressive rollouts)

Depending on the discussion I'll create an epic and flesh it out.

Outcomes

Event Timeline

Sounds good to me. Deprecating isBetaGroupMember in MobileFrontend js would be a good starting point... And then .beta class

My one major open question is how we use beta in this new world. I ponder the need for the SkinMinervaBeta skin and thus the MobileContext isBetaGroupMember method.

I actually had very much the same thought in the shower this morning…


There are distinct configuration files for MF and MFβ for the Wikipedias – IMO this should be the case for the codebase too – so the feature flags can be configured separately too, i.e. no need for a hash!

Bucketing anonymous users will be impossible on the server (Varnish!) but possible with a little top-loaded JavaScript.

The idea of scoping features in JavaScript and CSS with classes on the head element is an established practice and I'm all for it. However, a consequence of our caching infrastructure is that features might be rolled out per-page without a little massaging on the client, i.e. a little top-loaded JavaScript.

If beta is useful as it is or we want to substitute it for something like betafeatures is a different discussion I think.

The mobilefrontend beta is a group of enabled featureflags, and we can keep using it.

@Jdlrobson Php's isBetaGroupMember is very useful in such case for setting the featureflags by marrying config + isBetaGroupMember.

As you mention, the following seem like we could get rid of:

  • In JS isBetaGroupMember
  • .beta css class
  • SkinMinervaBeta

That would force moving code using such code to standalone features that are feature flagged. It could be some work and risk for regressions, but it would be worth it IMO.

@Jhernandez - is there a reason this is in tracking? Should we move to needs analysis?

Can probably be folded into the other discussion...

(T141087)
marking as low to mimic that discussion