Support Microsoft Feature Flag schema feature flag schema v1.1.0 by default

[zhiyuanliang-ms]

We want to augment the FeatureManagement-Dotnet to support the Microsoft Feature Flag schema v1.1.0

{
    "FeatureManagement": {
        "FeatureFlags": [
            // Microsoft Feature Flag objects
        ]
    }
}

The Microsoft Feature Flag definitions will be listed in the "FeatureFlags" array.

It is not allowed to use different schemas at the same time. If we can find the “FeatureFlags” array in the “FeatureManagement” section, we will only parse the Microsoft feature flag definitions listed in the "FeatureFlags" array. The old schema (if any) will be ignored.

[RichardChen820]

Is _FeatureFlagArraySchemaEnabled better? Anyway，we'd avoid using the “serverSide” word, since it is not meaningful for FeatureManagement library

[jimmyca15]

There is no such thing as server side schema. This library exists completely independently from Azure App Configuration. All notion of server side should be eliminated. Instead it should be referred to as the Azure App Configuration feature flag schema.

[RichardChen820]

The current PR title makes sense to me, this PR is exactly for that purpose, support App Config Schema. But I think we should not name anything with "app configuration" in the code. This library should not have any strong correlation with App configuration, it's independent.

[zhenlan]

"App Configuration" is the branding. It's like the FM library adopts a schema that is created by App Configuration.

[RichardChen820]

So you are suggesting naming correlated variables with "AppConfiguration" in the code?

[jimmyca15]

We are augmenting the library to support the "Azure App Configuration feature flag schema". In that sense there is a depenency.

[RichardChen820]

Once FeatureFlags:[] section exists, we no longer treat other root configurations as feature flags, is this intended?

[zhiyuanliang-ms]

Yes. Besides, FeatureFlags:{} and FeatureFlags:[] will be treated as the same thing by IConfiguration.

[RichardChen820]

I'm trying to imagine such a user case below in Kubernetes (since App Config Kubernetes Provider would be the only place that provides the server-side schema).

A customer has a configuration file that includes FeatureManagement section (old schema) in his app. He uses Kubernetes Provider to get some feature flags from Azure App Configuration and expect the feature flags from App Config can overwrite the feature flags in his local file if duplicated, but keep the local feature flags if the App Config end doesn't have them.

I'm not sure whether this is a valid user case we should support. But apparently, we can't handle this case with the current code.

[RichardChen820]

Should we throw an exception if the child of client_filters is not an array?

[zhiyuanliang-ms]

I think we should just ignore it if we cannot parse it.

[zhenlan]

Schema validation should be based on the schema
https://github.com/Azure/AppConfiguration/blob/main/docs/FeatureManagement/FeatureFlag.v1.1.0.schema.json

[RichardChen820]

Should we handle the situation that the name and parameters are missing?

[zhiyuanliang-ms]

If the name is missing, it will not enter the if branch. The parameters is ok to be missing.

[jimmyca15]

PR description should reference the schema we are aiming to support:

https://github.com/Azure/AppConfiguration/blob/main/docs/FeatureManagement/FeatureFlag.v1.1.0.schema.json

[jimmyca15]

This is assigned to with an as implying this can be null. Looks like a possible null-ref.

[zhiyuanliang-ms]

Sorry, I didn't understand. _configuration cannot be null, otherwise an exception will be thrown in the constructor.
IConfiguration can always be casted to IConfigurationSection. I think there could not be a null-ref.

[jimmyca15]

Interlocked usage here can allow a simultaneously executing thread to bypass the critical section and return from the method without initializing. With the requirement to initialize here a lock cannot be avoided. Single usage of lock on initialization should be acceptable.

[jimmyca15]

If featureFlagsConfigurationSection.GetChildren() returns empty, then All would return true. Seems unintended.

[zhiyuanliang-ms]

This is intended. If there is "FeatureFlags" section, either it is empty or an array, we consider it as using the App Config schema.

[jimmyca15]

A few suggestions in this section. Get the section while checking for it's existence. Check if feature flags section has any children while doing an all check.

            IConfigurationSection featureFlagsConfigurationSection = featureManagementConfiguration
                    .GetChildren()
                    .FirstOrDefault(section => 
                        string.Equals(
                            section.Key,
                            AzureAppConfigurationFeatureFlagFields.FeatureFlagsSectionName,
                            StringComparison.OrdinalIgnoreCase));

            if (featureFlagsConfigurationSection != null)
            {
                if (!string.IsNullOrEmpty(featureFlagsConfigurationSection.Value))
                {
                    return false;
                }

                featureFlagChildren = featureFlagsConfigurationSection.GetChildren();

                return featureFlagChildren.Any() && featureFlagChildren.All(section => int.TryParse(section.Key, out int _));
            }

            return false;

[jimmyca15]

Better to get the section with FirstOrDefault, rather than just check for it's existence, else you will just need to get it again later.

[jimmyca15]

Suggested change

	private string GetFeatureFlagSectionName(IConfigurationSection section)
	private string GetFeatureFlagName(IConfigurationSection section)

[jimmyca15]

Or

Suggested change

	private string GetFeatureFlagSectionName(IConfigurationSection section)
	private string GetFeatureName(IConfigurationSection section)

Given the caller uses a variable named featureName

[RichardChen820]

lock can sure only one thread can enter this block, what is the reason for using Interlocked.Read(ref _initialized) to atomically get the value?

Mark int32 type _initialized as volatile then judge _initialized == 0 is fine. Am I missing anything?

[RichardChen820]

Or use

if (Interlocked.CompareExchange(ref _initialized, 1, 0))==0 {
_azureAppConfigurationFeatureFlagSchemaEnabled = hasAzureAppConfigurationFeatureFlagSchema; }

is also fine.

[jimmyca15]

Interlocked exchange does perform an atomic swap, but it also has the added benefit of forcing a read from memory. Here, interlocked avoids cached read from CPU. Marking the variable as volatile will have lasting effect throughout the lifetime of the application whereas interlocked usage in the init check only happens during initialization.

[mrm9084]

@jimmyca15 @zhiyuanliang-ms

We want to augment the FeatureManagement-Dotnet to support the [Azure App Configuration feature flag schema] (https://github.com/Azure/AppConfiguration/blob/main/docs/FeatureManagement/FeatureFlag.v1.1.0.schema.json)
{
"FeatureManagement": {
"FeatureFlags": [
// App Configuration Feature Flag objects
]
}
}

Isn't this the "V2" schema. I thought we decided not to use it?

[zhiyuanliang-ms]

@mrm9084 This is not the V2 schema because this PR is targeting on the main branch. The variant and telemetry part are implemented in the preview/release v4 branch which is for the ongoing preview release. We didn't plan the App Config feature flag schema support for the preview release.

[zhiyuanliang-ms]

@zhenlan @jimmyca15 @RichardChen820 As discussed in the email: "Microsoft Feature Flag schema should be the name we use going forward", I made some update to this PR.