Update README file for variants

[amerjusupovic]

No description provided.

[rossgrambo]

Could you add the main new sections to the index?

[zhenlan]

Is the On alias something new? What's the reason we want to advertise an alternative name?

[amerjusupovic]

I wasn't part of the original planning, but it was part of the variants documentation doc that I referenced for my PR. I assumed it was because seeing a flag called AlwaysOn when you can actually override it to be off with a variant's StatusOverride property would be confusing.

[jimmyca15]

Given the presence of 'On' I would suggest we drop "AlwaysOn" from being mentioned here.

[rossgrambo]

I'm happy with On as well. But someone looking at a config for the first time should be able to determine what AlwaysOn means. Perhaps we can leave a note at the bottom of the readme that AlwaysOn was the previous name for On?

[zhenlan]

We will have to change the portal and CLI for file exporting to json and yaml. The file import then has to deal with both AlwaysOn and On. Is the renaming worth it?

The AlwaysOn is a half-internal workaround solution. I bet most AppConfig feature flag users never heard about it. I know its name is not ideal, but how bad it is, especially given the renaming cost?

[jimmyca15]

Also wanted to mention why this came up. Previously, there never was a reason for a user to see or use "AlwaysOn". A simple "on" feature could be represented as a boolean literal like my previous comment.

This changes with variants. Now to write a flag with variants in appsettings.json, users have to specify a filter. The idea was to make the declaration shorter than "AlwaysOn" which was never really used anyhow.

Under the hood, "On" and "AlwaysOn" both work. The suggestion wasn't a rename, rather just to support "On" as well.

[zhenlan]

I understand the motivation and I understand the AlwaysOn naming is not ideal.

I guess the file import part must understand AlwaysOn (and On in the future if we add it). I didn't search the CLI code, but looks like our sync library has it.

[jimmyca15]

Okay, we do use it. I'm fine to avoid the change to introduce "On" then.

[amerjusupovic]

Ok, so should I not mention On in this file at all in that case?

[zhenlan]

Right, and removal of On in code.

[zhenlan]

What happens if the Seed is not specified? Do we use a default value?

[amerjusupovic]

Yea I should mention this, we create a new string using the feature name and use that instead of the Seed value. I guess that would be the default value.

[rossgrambo]

featurename + "\nallocation"

[jimmyca15]

I would consider it appropriate for us to mention that if seed is omitted, then a default seed based off the flag name is used. But I would say away from mentioning our exact implementation.

[zhenlan]

Yes, it will be great to clarify.

[zhenlan]

So this is required to be a JSON? Can I have more than one key value?

[amerjusupovic]

Maybe I can explain that better, but it can be either a JSON or just a string after ConfigurationValue and it will return the configuration section starting with ConfigurationValue as the key. From there, you can index it like a normal configuration section. You can add any number of values or any structure of nested JSONs if you want.

[zhenlan]

Can it be a number or a boolean?

[amerjusupovic]

Yes, anything that could normally be in a configuration file like this, should I mention each of those?

[zhenlan]

Yes, I think so. As an example, I think we should show a primitive value rather than a more complex JSON.

[zhenlan]

While nothing is wrong in this example, it's unclear what the StatusOverride is for. Can we have a more real-world example?

[rossgrambo]

If we do add an example for StatusOverride, we should make it very clear that it's a bandaid for a specific situation.

That situation is if you want to use allocation to control true and false without making code changes.

If you're writing a new app or adding new features, use variants with allocation. If you want simple true and false, you can achieve it with a targeting filter. If you want "variants" for your existing true and false, use the defaults.

Honestly, I prefer not having an example for StatusOverride as I don't want new customers grabbing the snippet.

[zhenlan]

There must be some misunderstanding. I can agree the StatusOverride is a bandaid fix (in terms of design) but it is NOT just for existing users who don't want to make code changes.

The allocation is more powerful than the targeting filter (see an example later). We will want our customers (existing or new) to adopt the allocation with variants. However, without StatusOverride, they will have to write everything like

if (await fm.GetVariantAsync("foo").Configuration["ConfigurationValue"] == "true") ... 
// or
if (await fm.GetVariantAsync("foo").Name == "On") ... 

instead of simply

if (await fm.IsEnabledAsync("foo"))...

Our whole MS.FM.AspNetCore package is essentially built on top of the IsEnabledAsync API. Without StatusOverride, that package can be thrown out of the window. We all know On/Off accounts for the majority of the real-world usage. This is why StatusOverride is important for both existing and new customers.

We should explain what the StatusOverride is for and why it's useful. Here is an example that you can't achieve with targeting, and is meaningful in the real world.

"Allocation": {
    "Percentile": [{
        "Variant": "On",
        "From": 10,
        "To": 20
    }],
    "DefaultWhenEnabled":  "Off",
    "Seed": "Black-Friday-Feature-Group"
},
"Variants": [
    { 
        "Name": "On", 
        "ConfigurationValue": true
    },
    { 
        "Name": "Off", 
        "ConfigurationValue": false,
        "StatusOverride": "Disabled"
    }    
],
"EnabledFor": [ 
    { 
        "Name": "AlwaysOn" 
    } 
] 

[amerjusupovic]

I've pretty much added this example to the StatusOverride section now

[jimmyca15]

I think it would be worth it to call out each property of Allocation individually and describe what it's for. E.g.

The Allocation setting of a feature flag has the following properties.

DefaultWhenDisabled: Specifies which variant should be used when a variant is requested while the flag is considered disabled.
DefaultWhenEnabled: ...
...Other properties...

And then go into the specific behavior of this example.

[jimmyca15]

if they match the targeting context or calculated percentile for that context

I would leave out reference to targeting context. Instead mention the intent, like if the user being evaluated is specified in the users section, or if the user is in a group specified in the groups section, or if the user fits into the specified percentage allocation.

A separate section can be used, if necessary, to describe how that user/group info is determined (targeting context)

[jimmyca15]

Suggested change

The `StatusOverride` property, if set for the assigned variant, overrides a feature's state. By default, this property is equal to `None`, which does not affect the feature state. If `StatusOverride` is equal to `Enabled` for the assigned variant, then the feature is enabled. If `StatusOverride` is equal to `Disabled` for the assigned variant, then the feature is disabled.

`StatusOverride` is an optional property of a variant that allows it to override whether a flag should be considered enabled or disabled. This gives variants an opportunity to extend the evaluation of a feature flag. If a caller is checking whether a flag that has variants is enabled, then variant allocation will be performed to see if an allocated variant is set up to override the result. By default, this property is set to `None`, which means the variant doesn't affect whether the flag is considered enabled or disabled. Setting `StatusOverride` to `Enabled` allows the variant, when chosen, to override a flag to be enabled. Setting `StatusOverride` to `Disabled` provides the opposite functionality, therefore disabling the flag when the variant is chosen.

[jimmyca15]

On second thought. I might suggest rearranging this to first introduce the concept of a variants overriding the enabled state of a flag. Then go into the property that is used to do it.

[zhenlan]

Can I omit both ConfigurationReference and ConfigurationValue?

[amerjusupovic]

Yes, the Variant.Configuration value will just be null

[zhenlan]

Great, we can make it clear in the description.

This enables customers to have very concise variants like the one below.

"Variants": [
    { 
        "Name": "On"
    },
    { 
        "Name": "Off", 
        "StatusOverride": "Disabled"
    }    
]

[zhenlan]

I hope we can explain a little more about the scenarios where this property can be useful. Can we add a sentence like below at the beginning of this paragraph?

If you are using a feature flag with binary variants, the StatusOverride property can be very helpful. It allows you to keep using APIs like IsEnabledAsync and FeatureGateAttribute in your application, all while benefiting from the new features that come with variants, such as percentile allocation and seed.

[amerjusupovic]

I think the example should be immediately followed by its explanation, could we add something similar to this sentence just before the example maybe? In a separate paragraph from the introduction?

[zhenlan]

Sounds good. That works.

[zhenlan]

In this example, the ConfigurationValue doesn't matter much. Can we remove it for both variants?