Update to dynamic features. #157

jimmyca15 · 2021-12-14T19:05:24Z

This PR contains multiple changes. The two biggest changes are the separation of feature flags and dynamic features and the renaming of types to respect dynamic feature terminology. Most other changes follow as consequence. Details of the included changes are below.

Documentation updates, including the new configuration schema, to follow in a subsequent PR.

Update types to respect dynamic terminology. E.g. IFeatureManager -> IDynamicFeatureManager.
- Updated property/method summaries to distinguish dynamic features and feature flags.
Separate feature flags and dynamic features in configuration schema
- Backward compatability support for old configuration schema.
Breaking: Update custom feature definition provider to return feature flags and dynamic features.
- Added DynamicFeatureDefinition class
- renamed FeatureDefinition to FeatureFlagDefinition.
Breaking: IFeatureManager.GetFeatureNamesAsync renamed to IFeatureManager.GetFeatureFlagNamesAsync
Corrected namespace for built-in feature variant assigners.
- Microsoft.FeatureManagement -> Microsoft.FeatureManagement.Assigners
Added IDynamicFeatureManagerSnapshot
Added test for custom feature definition provider.
Added test for backcompat support for v1 configuration schema.

Old configuration schema example

{
    "FeatureManagement": {
        "FeatureFlag1": true,
        "FeatureFlag2": {
            "EnabledFor": [
                ...
            ]
        }
    }
}

New configuration schema example

{
    "FeatureManagement": {
        "FeatureFlags": {
            "FeatureFlag1": true,
            "FeatureFlag2": {
                "EnabledFor": [
                    ...
                ]
            }
        },
        "DynamicFeatures": {
            "DynamicFeature1": {
                "Assigner": "Targeting",
                "ConfigurationReference": "SomeSettings",
                "AssignmentParameters": {
                    ...
                }
            }
        }
    }
}

* Update types to respect dynamic feature name. E.g. IFeatureManager -> IDynamicFeatureManager. * Updated property/method summaries to distinguish dynamic features and feature flags. * Added IDynamicFeatureManagerSnapshot * Separate feature flags and dynamic features in configuration schema * Backward compatability support for old configuration schema. * Breaking: Update custom feature definition provider to return feature flags and dynamic features. * Added DynamicFeatureDefinition class * renamed FeatureDefinition to FeatureFlagDefinition. * Breaking: IFeatureManager.GetFeatureNamesAsync renamed to IFeatureManager.GetFeatureFlagNamesAsync * Corrected namespace for built-in feature variant assigners. * Microsoft.FeatureManagement -> Microsoft.FeatureManagement.Assigners * Added test for custom feature definition provider. * Added test for backcompat support for v1 configuration schema.

zhenlan · 2022-01-06T22:46:15Z

src/Microsoft.FeatureManagement/IDynamicFeatureManager.cs

+        /// <param name="feature">The name of the dynamic feature.</param>
+        /// <param name="cancellationToken">The cancellation token to cancel the operation.</param>
+        /// <returns>A typed representation of the feature variant that should be used for a given dynamic feature.</returns>
+        ValueTask<T> GetVariantAsync<T>(string feature, CancellationToken cancellationToken);


Add default for cancellationToken

zhenlan · 2022-01-06T22:46:20Z

src/Microsoft.FeatureManagement/IDynamicFeatureManager.cs

+        /// <param name="context">A context providing information that can be used to evaluate which variant should be used for the dynamic feature.</param>
+        /// <param name="cancellationToken">The cancellation token to cancel the operation.</param>
+        /// <returns>A typed representation of the feature variant's configuration that should be used for a given feature.</returns>
+        ValueTask<T> GetVariantAsync<T, TContext>(string feature, TContext context, CancellationToken cancellationToken);


Add default for cancellationToken

zhenlan · 2022-01-06T22:47:45Z

examples/CustomAssignmentConsoleApp/Program.cs

            using (ServiceProvider serviceProvider = services.BuildServiceProvider())
            {
-                IFeatureVariantManager variantManager = serviceProvider.GetRequiredService<IFeatureVariantManager>();
+                IDynamicFeatureManager variantManager = serviceProvider.GetRequiredService<IDynamicFeatureManager>();


variantManager

NIT: update variable name variantManager

zhenlan · 2022-01-06T22:50:18Z

examples/FeatureFlagDemo/Views/Shared/_Layout.cshtml

@@ -1,5 +1,5 @@
 @using Microsoft.FeatureManagement
-@inject IFeatureVariantManager variantManager;
+@inject IDynamicFeatureManager variantManager;


NIT: update variable name variantManager too?

zhenlan · 2022-01-06T22:51:08Z

examples/TargetingConsoleApp/Program.cs

            {
                IFeatureManager featureManager = serviceProvider.GetRequiredService<IFeatureManager>();
-                IFeatureVariantManager variantManager = serviceProvider.GetRequiredService<IFeatureVariantManager>();
+                IDynamicFeatureManager variantManager = serviceProvider.GetRequiredService<IDynamicFeatureManager>();


NIT: update variable name variantManager to dynamicFeatureManager?

zhenlan · 2022-01-06T23:48:04Z

src/Microsoft.FeatureManagement/FeatureManagerSnapshot.cs

+            {
+                var dynamicFeatureNames = new List<string>();
+
+                await foreach (string featureName in _featureManager.GetFeatureFlagNamesAsync(cancellationToken).ConfigureAwait(false))


It should be
_dynamicFeatureManager.GetDynamicFeatureNamesAsync

src/Microsoft.FeatureManagement/FeatureManagerSnapshot.cs

zhenlan · 2022-01-06T23:52:13Z

src/Microsoft.FeatureManagement/FeatureManager.cs

    /// Used to evaluate whether a feature is enabled or disabled.
    /// </summary>
-    class FeatureManager : IFeatureManager, IFeatureVariantManager
+    class FeatureManager : IFeatureManager, IDynamicFeatureManager


I'm thinking whether we should split the implementation of two interfaces into separate classes. They are pretty much independent. This could be confusing for someone new to look at this code in the future. Thoughts?

I don't see the value immediately. What is the confusion that you had in mind?

I guess the question can be asked in the other way - why do we implement both interfaces in one class? Would we do the same if we started from scratch? The functionalities of the two interfaces are parallel. It's not like one is a complement to another. In current naming, feels like the FeatureManager is really for IFeatureManager, and the IDynamicFeatureManager is a supplement (like IDisposable).

Would we do the same if we started from scratch?

I think that's a great way to look at it. I'd say we'd have them separated. Let me separate them.

In current naming, feels like the FeatureManager is really for IFeatureManager, and the IDynamicFeatureManager

For this point, it would be different if we renamed IFeatureManager to IFeatureFlagManager. But that was a breaking change that we didn't consider.

For this point, it would be different if we renamed IFeatureManager to IFeatureFlagManager. But that was a breaking change that we didn't consider.

Totally, that would make more sense. Another option is to add what's in IDynamicFeatureManager to IFeatureManager directly. I think that's what we would do if we started from scratch, but we worried it will break all existing libraries built on top of the current feature management library.

Yes, to be honest I'm not completely sold on either side. If you think it'll be more clear I'm happy to separate them. Otherwise, can keep in in FeatureManager

I think we should separate the implementation if we decide to separate the interface.

In that case I'll go ahead and separate ConfigurationFeatureDefinitionProvider and FeatureManagerSnapshot as well.

zhenlan · 2022-01-07T01:00:28Z

src/Microsoft.FeatureManagement/IContextualFeatureVariantAssigner.cs

        /// <param name="cancellationToken">The cancellation token to cancel the operation.</param>
-        /// <returns>The variant that should be assigned for a given feature.</returns>
+        /// <returns>The variant that should be assigned for a given dynamic feature.</returns>
        ValueTask<FeatureVariant> AssignVariantAsync(FeatureVariantAssignmentContext variantAssignmentContext, TContext appContext, CancellationToken cancellationToken);


cancellationToken

Should we default the cancellationToken?

Looks like we don't have cancellationToken in I*FeatureFilter, should we add it?

Looks like we don't have cancellationToken in I*FeatureFilter, should we add it?

cancellation tokens on existing interfaces were added in PRs checked in to the main branch.

Add cancellation token parameter to async feature management interfaces. #131

For that reason I didn't touch them here. This PR is going into "feature/v3" branch and when "feature/v3" merges into main those conflicts will be resolved.

Circling back to this, there were some missing default cancellation tokens on the new dynamic feature interfaces. Thanks. I'm adding them now.

zhenlan · 2022-01-07T01:05:46Z

src/Microsoft.FeatureManagement/IFeatureDefinitionProvider.cs

-        /// <returns>The feature's definition.</returns>	
-        Task<FeatureDefinition> GetFeatureDefinitionAsync(string featureName, CancellationToken cancellationToken = default);
+        /// <returns>The feature flag's definition.</returns>	
+        Task<FeatureFlagDefinition> GetFeatureFlagDefinitionAsync(string featureName, CancellationToken cancellationToken = default);


featureName

NIT: featureName -> featureFlagName

zhenlan · 2022-01-07T01:08:48Z

src/Microsoft.FeatureManagement/IFeatureDefinitionProvider.cs

    /// <summary>
    /// A provider of feature definitions.
    /// </summary>
    public interface IFeatureDefinitionProvider


Should we split it into separate interfaces for feature flags and dynamic features so users don't have to implement both if they only care about one but not the other?

We split IFeatureManager and IDynamicFeatureManager so I'm not opposed to carrying that over here.

zhenlan · 2022-01-07T01:10:53Z

src/Microsoft.FeatureManagement/IFeatureManager.cs

        /// <param name="cancellationToken">The cancellation token to cancel the operation.</param>
-        /// <returns>True if the feature is enabled, otherwise false.</returns>
+        /// <returns>True if the feature flag is enabled, otherwise false.</returns>
        Task<bool> IsEnabledAsync(string feature, CancellationToken cancellationToken = default);


feature -> featureFlag?

avanigupta · 2022-01-07T19:47:34Z

src/Microsoft.FeatureManagement/FeatureManagerSnapshot.cs

            }
        }

+        public async IAsyncEnumerable<string> GetDynamicFeatureNamesAsync([EnumeratorCancellation]CancellationToken cancellationToken = default)


cancellationToken = default

default not needed here. Cancellation token already has a default value in the interface.

avanigupta · 2022-01-07T20:31:27Z

src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs

            */

-            var enabledFor = new List<FeatureFilterConfiguration>();
+            Debug.Assert(configurationSection != null);


Debug.Assert(configurationSection != null);

Should we throw an exception instead of assert?

It's a private method so throwing here wouldn't be expected.

avanigupta · 2022-01-07T20:35:40Z

src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs

+
+        private DynamicFeatureDefinition ReadDynamicFeatureDefinition(IConfigurationSection configurationSection)
+        {
+            Debug.Assert(configurationSection != null);


Debug.Assert(configurationSection != null);

Same here. Null check may not even be necessary since the calling methods already check for null before invoking these two methods.

We often Debug.Assert use Debug.Assert in our private methods. It offers a couple benefits.

It makes it clear that the assertion can be relied on in the rest of the method execution. In this case, the reader will know the configurationSection is not null.

When we look at this method we know that if we're calling it from another method we shouldn't pass null, and vice versa we can look at the places that call this and confirm that they're doing a null check, like you mentioned, before calling.

If it is ever null for some reason, it will throw in debug mode so it can be fixed.

src/Microsoft.FeatureManagement/FeatureManagerSnapshot.cs

…ntation in FeatureManager, ConfigurationFeatureDefinitionProvider, and FeatureManagerSnapshot.. * Separated IConfigurationDefinitionProvider into IFeatureFlagDefinitionProvider and IDynamicFeatureDefinitionProvider * Added missing default cancellation tokens.

zhenlan · 2022-01-08T04:03:30Z

src/Microsoft.FeatureManagement/DynamicFeatureManager.cs

+            return assigner;
+        }
+
+        private static bool IsMatchingMetadataName(string metadataName, string desiredName, string suffix)


The comments in this function are still talking about filters. Is there a place we can put this helper function to so it can be shared?

Sure. I put it in a new helper, NameHelper.

zhenlan · 2022-01-10T18:43:33Z

using System;

Missing copyright header

Refers to: src/Microsoft.FeatureManagement/NameHelper.cs:1 in ba664ee. [](commit_id = ba664ee, deletion_comment = False)

* Add support for feature variants. * Renaming and assorted fixes. * Remove unnecessary field. * * Added additional argument validation. * Added additional dynamic feature configuration validation. * Fixed bug in audience accumulation for variant targeting * Added test for variant targeting audience accumulation. * Since filter aliases are case insensitive, update filter cache to reflect this. * * Fixed issue with filter + assigner suffix references. * Clarified error types. * Updated/added tests * Share suffix matching logic. Corrected variable name. * Removed unnecessary helper methods. Updated error codes for consistency. * Clarify filter selection logic in feature evaluation and add checks for impossible contextual evaluations. * Updated argument validation messages. Updated exception used for null child property. * Update to dynamic features. (#157) * Update to dynamic features. * Update types to respect dynamic feature name. E.g. IFeatureManager -> IDynamicFeatureManager. * Updated property/method summaries to distinguish dynamic features and feature flags. * Added IDynamicFeatureManagerSnapshot * Separate feature flags and dynamic features in configuration schema * Backward compatability support for old configuration schema. * Breaking: Update custom feature definition provider to return feature flags and dynamic features. * Added DynamicFeatureDefinition class * renamed FeatureDefinition to FeatureFlagDefinition. * Breaking: IFeatureManager.GetFeatureNamesAsync renamed to IFeatureManager.GetFeatureFlagNamesAsync * Corrected namespace for built-in feature variant assigners. * Microsoft.FeatureManagement -> Microsoft.FeatureManagement.Assigners * Added test for custom feature definition provider. * Added test for backcompat support for v1 configuration schema. * * Separated feature flag implementation from dynamic feature impelementation in FeatureManager, ConfigurationFeatureDefinitionProvider, and FeatureManagerSnapshot.. * Separated IConfigurationDefinitionProvider into IFeatureFlagDefinitionProvider and IDynamicFeatureDefinitionProvider * Added missing default cancellation tokens. * Use shared helper for filter/assigner reference matching. * Add copyright header. * Added dynamic feature documentation. (#159) * Added dynamic feature documentation. * Readme fixes. * * Added index. * Moved around sections * Removed enum usage. * Fixed mistyped interfaces. * Clarified variant resolution. * Fix links with '.' character. * Updates. * Updated index. * Default -> Name * Remove `MyFeatureFlags` references. * Updated missed "feature name" occurrences. * feature definition -> feature flag * SuggestedReadmeChanges * Updated comments from readme * Fixed targeting assignment precedence. * Remove unused variable. Update method signature. Added missing declaration in test. * Add null/empty check for feature evaluation. * Added .NET 6 as a target framework and removed .NET 5 * Update Azure App Configuration provider reference in example project to pull features using v2 schema. * Build with .NET 6 (#186) * Build with .NET 6 * Update build step name. * Add link to v2 readme. (#184) * Throw exception if feature management schemas are mixed to avoid unintentionally hiding feature flags. (#187) Co-authored-by: mrm9084 <[email protected]>

jimmyca15 requested review from avanigupta, mrm9084 and zhenlan December 14, 2021 19:05

zhenlan reviewed Jan 6, 2022

View reviewed changes

src/Microsoft.FeatureManagement/FeatureManagerSnapshot.cs Outdated Show resolved Hide resolved

zhenlan reviewed Jan 6, 2022

View reviewed changes

zhenlan reviewed Jan 7, 2022

View reviewed changes

avanigupta reviewed Jan 7, 2022

View reviewed changes

jimmyca15 commented Jan 7, 2022

View reviewed changes

src/Microsoft.FeatureManagement/FeatureManagerSnapshot.cs Outdated Show resolved Hide resolved

jimmyca15 force-pushed the user/jimmyca/schemaChange branch from 08d38af to f9b661b Compare January 7, 2022 23:42

zhenlan reviewed Jan 8, 2022

View reviewed changes

Use shared helper for filter/assigner reference matching.

ba664ee

zhenlan approved these changes Jan 10, 2022

View reviewed changes

Add copyright header.

6cac533

jimmyca15 merged commit fd51d97 into microsoft:feature/v3 Jan 10, 2022

brental mentioned this pull request Jan 15, 2022

Potentially unrequired breaking changes in v3 #160

Closed

Update to dynamic features. #157

Update to dynamic features. #157

Uh oh!

Conversation

jimmyca15 commented Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Old configuration schema example

New configuration schema example

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimmyca15 Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhenlan commented Jan 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

jimmyca15 commented Dec 14, 2021 •

edited

Loading

jimmyca15 Jan 7, 2022 •

edited

Loading