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
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 all 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
12 changes: 1 addition & 11 deletions ...Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1218,21 +1218,11 @@ private void EnsureFeatureManagementVersionInspected()
{
if (!_isFeatureManagementVersionInspected)
{
const string FeatureManagementMinimumVersion = "3.2.0";

_isFeatureManagementVersionInspected = true;

if (_requestTracingEnabled && _requestTracingOptions != null)
{
string featureManagementVersion = TracingUtils.GetAssemblyVersion(RequestTracingConstants.FeatureManagementAssemblyName);

// If the version is less than 3.2.0, log the schema version warning
if (featureManagementVersion != null && Version.Parse(featureManagementVersion) < Version.Parse(FeatureManagementMinimumVersion))
{
_logger.LogWarning(LogHelper.BuildFeatureManagementMicrosoftSchemaVersionWarningMessage());
}

_requestTracingOptions.FeatureManagementVersion = featureManagementVersion;
_requestTracingOptions.FeatureManagementVersion = TracingUtils.GetAssemblyVersion(RequestTracingConstants.FeatureManagementAssemblyName);

_requestTracingOptions.FeatureManagementAspNetCoreVersion = TracingUtils.GetAssemblyVersion(RequestTracingConstants.FeatureManagementAspNetCoreAssemblyName);
}
Expand Down
3 changes: 0 additions & 3 deletions src/Microsoft.Extensions.Configuration.AzureAppConfiguration/Constants/LoggingConstants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,5 @@ internal class LoggingConstants
public const string RefreshSkippedNoClientAvailable = "Refresh skipped because no endpoint is accessible.";
public const string RefreshFailedToGetSettingsFromEndpoint = "Failed to get configuration settings from endpoint";
public const string FailingOverToEndpoint = "Failing over to endpoint";
public const string FeatureManagementMicrosoftSchemaVersionWarning = "Your application may be using an older version of " +
"Microsoft.FeatureManagement library that isn't compatible with Microsoft.Extensions.Configuration.AzureAppConfiguration. Please update " +
"the Microsoft.FeatureManagement package to version 3.2.0 or later.";
}
}
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
Copy link
Member

@avanigupta avanigupta Jul 8, 2024

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?

Copy link
Contributor Author

@amerjusupovic amerjusupovic Jul 8, 2024

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?

Copy link
Member

@zhiyuanliang-ms zhiyuanliang-ms Jul 9, 2024

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"}
  ]
}

Copy link
Member

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

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.

Copy link
Member

@avanigupta avanigupta Jul 9, 2024

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";
}
}
133 changes: 99 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 @@ -30,9 +30,104 @@ public Task<IEnumerable<KeyValuePair<string, string>>> ProcessKeyValue(Configura

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

// Check if we need to process the feature flag using the microsoft schema
if ((featureFlag.Variants != null && featureFlag.Variants.Any()) || featureFlag.Allocation != null || featureFlag.Telemetry != null)
{
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 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>>();

Copy link
Member

@jimmyca15 jimmyca15 Jul 8, 2024

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))
{
return keyValues;
}

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);

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

keyValues.Add(new KeyValuePair<string, string>($"{clientFiltersPath}:Name", clientFilter.Name));

foreach (KeyValuePair<string, string> kvp in new JsonFlattener().FlattenJson(clientFilter.Parameters))
{
keyValues.Add(new KeyValuePair<string, string>($"{clientFiltersPath}: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 +148,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 +165,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 +314,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
6 changes: 0 additions & 6 deletions src/Microsoft.Extensions.Configuration.AzureAppConfiguration/LogHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,13 +88,7 @@ public static string BuildLastEndpointFailedMessage(string endpoint)
public static string BuildFallbackClientLookupFailMessage(string exceptionMessage)
{
return $"{LoggingConstants.FallbackClientLookupError}\n{exceptionMessage}";
}

public static string BuildFeatureManagementMicrosoftSchemaVersionWarningMessage()
{
return LoggingConstants.FeatureManagementMicrosoftSchemaVersionWarning;
}

public static string BuildRefreshFailedDueToFormattingErrorMessage(string exceptionMessage)
{
return $"{LoggingConstants.RefreshFailedDueToFormattingError}\n{exceptionMessage}";
Expand Down
Loading