Feature flag design proposal

[corverroos]

Problem to be solved

Once we have released versions of charon running in the wild (public testnets), we need to be much more careful and controlled about how we rollout new features. Some features might be high risk, and we might want phase then in gradually over time (e.g. simnet only, then devnet, then testnet, and then mainnet).

Some considerations:

• We do not really have classic SaaS "environments" like dev, staging and prod.
• We also do not really have a single linear set of "networks"/"chains" like devnet, testnet, prod.
• There are a myriad of different "networks"/"chains" that charon can be deployed to: multiple testnets, gnosis chain, etc.
• Some users might want to run "beta" features on mainnet.
• Features should have a default "state", but this must be configurable via flags/config.
• Be careful with flags, since they cannot be removed easily, see prysm.

Proposed solution

• Introduce a new package/concept called rollout.
• Features are "rolled out" in three phases: alpha, beta, gold.
• Features in alpha are unstable and only to be used/tested by the core team.
• Features in beta are more stable can be used/tested by the community (opt in), but still lacks confidence for general availability.
• Features in gold are stable and generally available.
• Package rollout enumerates a set of Feature strings that are each hardcoded to a Phase.
• Charon run command has a flag --rollout which defaults to gold but can be set to alpha or beta.
• This flag controls which features are active.
• Features are enabled/disabled in the code by simple if statements: if rollout.Enabled(rollout.FeatureFoo) {
• Features are rolled out by updating their hardcoded phase.
• The feature and check should be removed and permanently integrated/enabled after a full release in gold.
• Individual features can be enabled/disabled via a unstructured flag --rollout-enable=csv,list,of,features / --rollout-disable
• This flag just warns if features do not exist (or have been removed).

[OisinKyne]

Personally I would swap the term gold for something like stable, which I think has more general recognition amongst devs. Maybe also a deprecated state?

As for the term rollout, i don't mind it, but it feels a bit verb-y than a declaration, like rollout is what we're doing, less so what the flag needs to be called. I could also see us using featureset or features even? Not sold on these suggestions, but floating them for the purpose of discussion

[leolara]

I also prefer stable and featureset

[leolara]

I think featureset package should export a very simple method like func Active(Feature) bool that will be used through out. It is very important that featureset do not depend on other packages as most packages will depend on it and would be a cyclic dependency.

[corverroos]

I'm fine with stable and feature_set. I explicitly didn't choose "feature" or "release" since it is already used for other things.

Note that we need to add the the feature state/phase to PRs/git commit, the result be would look like:

core/scheduler: implement some new cool feature

blah blah blah, this is great

category: feature
ticket: #123
feature_set: stable

Note the overuse of the term "feature"... Also, the change log has a section "features". Btw, the idea is to not include non-stable/non-gold features in the changelog. Or they could be added to their own section (Alpha Features/Beta Features).

And in the code it would look like:

if featureset.Active(featureset.FeatureFooBarQuz) {

}

Also if we use Active in the code, do we align the flag overrides --feature-set-enable=foo,bar,qux for --feature-set-activate=foo,bar,qux? And what is the opposite of active, deactive? --feature-set-deactivate? I feel enable/disable is more common.

---

I like my original suggestion more, but up to you guys...

core/scheduler: implement some new cool feature

blah blah blah, this is great

category: feature
ticket: #123
rollout: stable

And in the code it would look like:

if rollout.Enabled(rollout.FeatureFooBarQuz) {

}

Less stuttering of the term "feature"...

[corverroos]

Current suggestion:

Package: featureset
Enum: featureset.FeatureFooBar
Method: if featureset.Enabled(featureset.FeatureFooBar)
Statuses: alpha, beta, stable
Flags: --feature-set=alpha, --feature-set-enable=some_cool_thing,and_something_else, --feature-set-disable=buggy-bug
PR category: feature_set: stable

[dB2510]

Note the overuse of the term "feature"... Also, the change log has a section "features".

I agree with this that the term feature has been used and referred to a lot of places. Maybe we can name the package as featureflags rather that featureset directly.

I am down with features rolling our in phases like alpha, beta and stable. For flags, since we have 3 statuses we can have something --alpha, --beta, or --stable with boolean flags. In my opinion from user's perspective this looks more intuitive.

Suggestion for PR body:

core/scheduler: implement some new cool feature

blah blah blah, this is great

category: feature
ticket: #123
phase: stable

OR

core/scheduler: implement some new cool feature

blah blah blah, this is great

category: feature
ticket: #123
status: stable

[leolara]

Package: featureset
Enum: featureset.FeatureFooBar
Method: if featureset.Enabled(featureset.FeatureFooBar)
Statuses: alpha, beta, stable
Flags: --feature-set=alpha, --feature-set-enable=some_cool_thing,and_something_else, --feature-set-> disable=buggy-bug

Ok with this.

PR category: feature_set: stable

What is this a github label or for the PR body?

Also featureset package will need some setter public methods featureset.Set(featureset) featureset.Enable(feature) which will set some private globals. We need to define what are the effects depending on the order of how they are called. An alternative would be featureset.Set(featureset, enable feature[], disable feature[]) so then we do not have to think about the order

[corverroos]

@leolara Sorry "PR category" was a typo, it should rather by "PR tag", see https://github.com/ObolNetwork/charon/blob/main/docs/contributing.md#pr-template

[leolara]

@corverroos alright man!!

[corverroos]

Ito featureset package API, I was thinking:

// Package featureset defines a set of global features and the status of the rollout phase. 
package featureset

// phase enumerates the roll out phases of a feature
type phase int  <<-- Note this doesn't need to be exported

const (
  phaseAlpha = iota + 1  <<-- Zero should not be valid enum
  phaseBeta
  phaseStable
)

// Feature is a feature being roll out in phases
type Feature string

const ( <<-- Note that we do not prefix the features with `Feature`, this reduces stutter and makes API more concise, same as time package.
  // FooSomething introduces something to foo, see https://github.com/ObolNetwork/charon/issues/397.
  FooSomething Feature = "foo_something"
  
  // BarMessages introcudes different message types for the bar service, see https://github.com/ObolNetwork/charon/issues/123. 
  BarMessages Feature = "bar_messages"
  
  // QuxConsensus enables the new fancy BFT qux consensus implementation, see https://github.com/ObolNetwork/charon/issues/397.
  QuxConsensus Feature = "qux_consensus"
)

var (
  // phases defines the current rollout phase of each feature.
  phases = map[Feature]Phase {
    FooSomething: phaseStable,
    BarMessages: phaseBeta,
    QuxConsensus: phaseAlpha,
  }
  
  // minPhase defines the minimum enabled phase.
  minPhase = phaseStable  
)

// Config configures the feature set package.
type Config struct { <<-- As per standard pattern, the fields are bound as flags.
    // MinPhase defines the minimum enabled phase.
    MinPhase string
    // Enabled overrides defaults and enables a list of features.
    Enabled []string
    // Disabled overrides defaults and disables a list of features.
    Disabled []string
}

// Init initialises the global feature set state.
func Init(config Config) error {
  // parse config strings
  // set minPhase
  // for each Enabled, set phases[feature] = math.MaxInt
  // for each Disabled, set phases[feature] = 0
  // Warn if any enabled/disabled feature does not exist.
}

// Enabled returns true if the feature is enabled.
func Enabled(feature Feature) bool {
   return phases[feature] >= minPhase
}

[leolara]

@corverroos please, put it in a few files, like constants.go config.go and featureset.go (with Init and Enabled). Having everything in the same file makes it more difficult to edit and find.

[corverroos]

Do we really need to split 100 lines of code into separate files?

[leolara]

In general in go we separate by component or subcomponent subject not by number of lines