Skip to content

Support both .NET and microsoft schema for feature flags #566

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
amerjusupovic merged 8 commits into from
Jul 10, 2024
Merged

Support both .NET and microsoft schema for feature flags #566

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3a6e57f
adding new private methods for dotnet versus microsoft schema, fixing…
amerjusupovic Jun 25, 2024
0a8d11b
remove unused method
amerjusupovic Jun 25, 2024
0251661
Merge branch 'preview' into ajusupovic/support-featuremanagement-dotn…
amerjusupovic Jun 28, 2024
8e96d6d
remove schema warning, PR comments
amerjusupovic Jul 8, 2024
1f8ddbd
Merge branch 'ajusupovic/support-featuremanagement-dotnet-schema' of …
amerjusupovic Jul 8, 2024
661aa62
unify process feature flag approaches with id, fix schema check
amerjusupovic Jul 8, 2024
a602820
fix tests to match changes
amerjusupovic Jul 8, 2024
d27c7c2
check for empty variants when deciding schema
amerjusupovic Jul 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ...sions.Configuration.AzureAppConfiguration/FeatureManagement/FeatureManagementConstants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,5 +44,10 @@ internal class FeatureManagementConstants
public const string ETag = "ETag";
public const string FeatureFlagId = "FeatureFlagId";
public const string FeatureFlagReference = "FeatureFlagReference";

// Dotnet schema keys

@avanigupta avanigupta Jul 8, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We added these fields for .NET schema and then removed the fields when we moved to Microsoft schema in the previous PR.
Now that we're adding support for .NET schema again, do we need to re-add Conditional/AlwaysOn/Status/Disabled fields?

@amerjusupovic amerjusupovic Jul 8, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe those were all as a result of variants, so since the feature management library is only processing flags as .NET schema if they don't include variants, these fields shouldn't be necessary. @zhiyuanliang-ms can you fact check me here?

@zhiyuanliang-ms zhiyuanliang-ms Jul 9, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The field AlwaysOn is still being used by .NET schema. ref doc

In our preview release, we even support On filter which was introduced by @amer's PR to support variant.

A feature flag like this:

{
  "id": "AlwaysOnFeature",
  "enabled": true
}

will look like this in .NET schema

"AlwaysOnFeature": {
  "EnabledFor": [
     {"Name": "AlwaysOn"}
  ]
}

@zhiyuanliang-ms zhiyuanliang-ms Jul 9, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But .NET schema also support declaring feature flag in this way:

"AlwaysOnFeature": true

That's how currently .NET provider does. ref

these fields shouldn't be necessary

Yes.

@avanigupta avanigupta Jul 9, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. So even though FM library supports "AlwaysOn" feature, we dont need to add that filter in the provider. Enabled/disabled features can be represented as {"feature1": true} or {"feature2": false}.
And FM library is not expecting Status field with .NET schema.

public const string DotnetSchemaSectionName = "FeatureManagement";
public const string DotnetSchemaEnabledFor = "EnabledFor";
public const string DotnetSchemaRequirementType = "RequirementType";
}
}
142 changes: 108 additions & 34 deletions ...Configuration.AzureAppConfiguration/FeatureManagement/FeatureManagementKeyValueAdapter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ internal class FeatureManagementKeyValueAdapter : IKeyValueAdapter
{
private FeatureFilterTracing _featureFilterTracing;
private int _featureFlagIndex = 0;
private bool _isMicrosoftSchema;

public FeatureManagementKeyValueAdapter(FeatureFilterTracing featureFilterTracing)
{
Expand All @@ -26,13 +27,110 @@ public FeatureManagementKeyValueAdapter(FeatureFilterTracing featureFilterTracin

public Task<IEnumerable<KeyValuePair<string, string>>> ProcessKeyValue(ConfigurationSetting setting, Uri endpoint, Logger logger, CancellationToken cancellationToken)
{
_isMicrosoftSchema = false;

@jimmyca15 jimmyca15 Jul 8, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm expecting this to be on a per flag basis.

@jimmyca15 jimmyca15 Jul 8, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise adding an unrelated flag that has variants will cause old flags that are working to break.

@avanigupta avanigupta Jul 8, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ProcessKeyValue is invoked once per flag. So each flag will either use .NET schema or MS schema. Am I missing something?

@jimmyca15 jimmyca15 Jul 8, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. The code would function correctly. I was thrown off because instance state is being used to flow a condition that's only relevant to the method invocation. To avoid that, the decision of which schema to use should be done by the one who calls parse feature flag and does not need to be saved as instance state.


FeatureFlag featureFlag = ParseFeatureFlag(setting.Key, setting.Value);

var keyValues = new List<KeyValuePair<string, string>>();

if (_isMicrosoftSchema)
{
keyValues = ProcessMicrosoftSchemaFeatureFlag(featureFlag, setting, endpoint);
}
else
{
keyValues = ProcessDotnetSchemaFeatureFlag(featureFlag, setting, endpoint);
}

return Task.FromResult<IEnumerable<KeyValuePair<string, string>>>(keyValues);
}

public bool CanProcess(ConfigurationSetting setting)
{
string contentType = setting?.ContentType?.Split(';')[0].Trim();

return string.Equals(contentType, FeatureManagementConstants.ContentType) ||
setting.Key.StartsWith(FeatureManagementConstants.FeatureFlagMarker);
}

public void InvalidateCache(ConfigurationSetting setting = null)
Comment thread
avanigupta marked this conversation as resolved.
Outdated
{
return;
}

public bool NeedsRefresh()
{
return false;
}

public void OnChangeDetected(ConfigurationSetting setting = null)
{
return;
}

public void OnConfigUpdated()
{
_featureFlagIndex = 0;

return;
}

private List<KeyValuePair<string, string>> ProcessDotnetSchemaFeatureFlag(FeatureFlag featureFlag, ConfigurationSetting setting, Uri endpoint)
{
var keyValues = new List<KeyValuePair<string, string>>();

@jimmyca15 jimmyca15 Jul 8, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ProcessMicrosoftSchemaFeatureFlag does


            if (string.IsNullOrEmpty(featureFlag.Id))
            {
                return keyValues;
            }

I'd suggest we unify the approach.

if (!string.IsNullOrEmpty(featureFlag.Id))
{
string featureFlagPath = $"{FeatureManagementConstants.DotnetSchemaSectionName}:{featureFlag.Id}";

if (featureFlag.Enabled)
{
if (featureFlag.Conditions?.ClientFilters == null || !featureFlag.Conditions.ClientFilters.Any())
{
keyValues.Add(new KeyValuePair<string, string>(featureFlagPath, true.ToString()));
}
else
{
for (int i = 0; i < featureFlag.Conditions.ClientFilters.Count; i++)
{
ClientFilter clientFilter = featureFlag.Conditions.ClientFilters[i];

_featureFilterTracing.UpdateFeatureFilterTracing(clientFilter.Name);

keyValues.Add(new KeyValuePair<string, string>($"{featureFlagPath}:{FeatureManagementConstants.DotnetSchemaEnabledFor}:{i}:Name", clientFilter.Name));
Comment thread
avanigupta marked this conversation as resolved.
Outdated

foreach (KeyValuePair<string, string> kvp in new JsonFlattener().FlattenJson(clientFilter.Parameters))
{
keyValues.Add(new KeyValuePair<string, string>($"{featureFlagPath}:{FeatureManagementConstants.DotnetSchemaEnabledFor}:{i}:Parameters:{kvp.Key}", kvp.Value));
}
}

//
// process RequirementType only when filters are not empty
if (featureFlag.Conditions.RequirementType != null)
{
keyValues.Add(new KeyValuePair<string, string>(
$"{featureFlagPath}:{FeatureManagementConstants.DotnetSchemaRequirementType}",
featureFlag.Conditions.RequirementType));
}
}
}
else
{
keyValues.Add(new KeyValuePair<string, string>($"{featureFlagPath}", false.ToString()));
}
}

return keyValues;
}

private List<KeyValuePair<string, string>> ProcessMicrosoftSchemaFeatureFlag(FeatureFlag featureFlag, ConfigurationSetting setting, Uri endpoint)
{
var keyValues = new List<KeyValuePair<string, string>>();

if (string.IsNullOrEmpty(featureFlag.Id))
{
return Task.FromResult<IEnumerable<KeyValuePair<string, string>>>(keyValues);
return keyValues;
}

string featureFlagPath = $"{FeatureManagementConstants.FeatureManagementSectionName}:{FeatureManagementConstants.FeatureFlagsSectionName}:{_featureFlagIndex}";
Expand All @@ -53,7 +151,7 @@ public Task<IEnumerable<KeyValuePair<string, string>>> ProcessKeyValue(Configura
{
ClientFilter clientFilter = featureFlag.Conditions.ClientFilters[i];

_featureFilterTracing.UpdateFeatureFilterTracing(clientFilter.Name);
_featureFilterTracing.UpdateFeatureFilterTracing(clientFilter.Name);

string clientFiltersPath = $"{featureFlagPath}:{FeatureManagementConstants.Conditions}:{FeatureManagementConstants.ClientFilters}:{i}";

Expand All @@ -70,7 +168,7 @@ public Task<IEnumerable<KeyValuePair<string, string>>> ProcessKeyValue(Configura
if (featureFlag.Conditions.RequirementType != null)
{
keyValues.Add(new KeyValuePair<string, string>(
$"{featureFlagPath}:{FeatureManagementConstants.Conditions}:{FeatureManagementConstants.RequirementType}",
$"{featureFlagPath}:{FeatureManagementConstants.Conditions}:{FeatureManagementConstants.RequirementType}",
featureFlag.Conditions.RequirementType));
}
}
Expand Down Expand Up @@ -219,37 +317,7 @@ public Task<IEnumerable<KeyValuePair<string, string>>> ProcessKeyValue(Configura
}
}

return Task.FromResult<IEnumerable<KeyValuePair<string, string>>>(keyValues);
}

public bool CanProcess(ConfigurationSetting setting)
{
string contentType = setting?.ContentType?.Split(';')[0].Trim();

return string.Equals(contentType, FeatureManagementConstants.ContentType) ||
setting.Key.StartsWith(FeatureManagementConstants.FeatureFlagMarker);
}

public void InvalidateCache(ConfigurationSetting setting = null)
{
return;
}

public bool NeedsRefresh()
{
return false;
}

public void OnChangeDetected(ConfigurationSetting setting = null)
{
return;
}

public void OnConfigUpdated()
{
_featureFlagIndex = 0;

return;
return keyValues;
}

private FormatException CreateFeatureFlagFormatException(string jsonPropertyName, string settingKey, string foundJsonValueKind, string expectedJsonValueKind)
Expand Down Expand Up @@ -346,6 +414,8 @@ private FeatureFlag ParseFeatureFlag(string settingKey, string settingValue)

case FeatureManagementConstants.Allocation:
{
_isMicrosoftSchema = true;

if (reader.Read() && reader.TokenType == JsonTokenType.StartObject)
{
featureFlag.Allocation = ParseFeatureAllocation(ref reader, settingKey);
Expand All @@ -364,6 +434,8 @@ private FeatureFlag ParseFeatureFlag(string settingKey, string settingValue)

case FeatureManagementConstants.Variants:
{
_isMicrosoftSchema = true;

if (reader.Read() && reader.TokenType == JsonTokenType.StartArray)
{
List<FeatureVariant> variants = new List<FeatureVariant>();
Expand Down Expand Up @@ -409,6 +481,8 @@ private FeatureFlag ParseFeatureFlag(string settingKey, string settingValue)

case FeatureManagementConstants.Telemetry:
{
_isMicrosoftSchema = true;

if (reader.Read() && reader.TokenType == JsonTokenType.StartObject)
{
featureFlag.Telemetry = ParseFeatureTelemetry(ref reader, settingKey);
Expand Down
Loading