microsoft / FeatureManagement-Dotnet

Microsoft.FeatureManagement provides standardized APIs for enabling feature flags within applications. Utilize this library to secure a consistent experience when developing applications that use patterns such as beta access, rollout, dark deployments, and more.
MIT License
1.02k stars 111 forks source link

Nuget "Microsoft.FeatureManagement.AspNetCore" Version 3.3.0 - Multiple feature filters match the configured filter named 'Microsoft.TimeWindow' #447

Closed davidstarkcab closed 3 months ago

davidstarkcab commented 3 months ago

Hello!

After we updated the NuGet package Microsoft.FeatureManagement.AspNetCore to version 3.3.0, we started receiving the following exception: "Multiple feature filters match the configured filter named 'Microsoft.TimeWindow'" whenever we have multiple feature toggles activated with Microsoft.TimeWindow.

This is how we configure it:

builder.Services.AddFeatureManagement().AddFeatureFilter<ContextualTargetingFilter>().AddFeatureFilter<TimeWindowFilter>().AddFeatureFilter<PercentageFilter>();

 builder.Configuration.AddAzureAppConfiguration(options =>
 {
     options.Connect(url, credentials).UseFeatureFlags(options =>
     {
         options.Select("*", environment);
         options.CacheExpirationInterval = TimeSpan.FromSeconds(30);
     });

     configurationRefresher = options.GetRefresher();
 });

Here is how we encounter the error:

await _configurationRefresher.TryRefreshAsync();

TargetingContext targetingContext = new TargetingContext
{
    Groups = Groups,
    UserId = String.Empty
};

var features = new List<FeatureToggle>();

await foreach (var featureName in _featureManagerSnapshot.GetFeatureNamesAsync())
{
    var feature = new FeatureToggle
    {
        Feature = featureName,
        IsEnabled = await _featureManagerSnapshot.IsEnabledAsync(featureName, targetingContext)
    };
    features.Add(feature);
}
jimmyca15 commented 3 months ago

What version did you upgrade from, and is the same setup working with the old version?

zhiyuanliang-ms commented 3 months ago

Hi, @davidstarkcab

I have reproduced the bug. This bug is introduced by 3.3.0. Sorry for the inconvenience.

The reason of this bug is that in v3.3.0, we enhance the TimeWindowFilter to support recurrence settings. To optimize the recurring time window evaluation, we used a memory cache in TimeWindowFilter, which cause the registeration of TimeWindowFilter is slightly different from other built-in filters. We used a new overload of AddFeatureFilter method. This caused TimeWindowFilter being registered multiple times and the feature manager will get duplicated filter metadata of it. Then, AmbiguousFeatureFilter exception is thrown.

To resolve your problem, please downgrade to 3.2.0 or you can change this line

 builder.Services.AddFeatureManagement().AddFeatureFilter<ContextualTargetingFilter>().AddFeatureFilter<TimeWindowFilter>().AddFeatureFilter<PercentageFilter>();

to

 builder.Services.AddFeatureManagement();

Starting from 3.0.0, ContextualTargetingFilter, TimeWindowFilter and PercentageFilter are registered within the AddFeatureManagement call. For more details, see here So you no longer need to register them by yourself. We mention this in the public doc and also in the README.

In the README, we said

There a few feature filters that come with the Microsoft.FeatureManagement package: PercentageFilter, TimeWindowFilter, ContextualTargetingFilter and TargetingFilter. All filters, except for the TargetingFilter, are added automatically when feature management is registered.

This is a mistake, I should bold this to catch more eyes. I will update the README and fix the bug soon.

davidstarkcab commented 3 months ago

What version did you upgrade from, and is the same setup working with the old version?

3.2.0 and its the same setup

davidstarkcab commented 3 months ago

@zhiyuanliang-ms Thanks!

zhiyuanliang-ms commented 3 months ago

@jimmyca15

Our previous way to avoid duplicated registeration is to check the implementationType of existing ServiceDescriptor in the service collection. Now, time window filter is registered through implementationFactory, its descriptor's implementationType is null.

ServiceDescriptor doesn't allow us to set implementationType while using implementationFactory. https://learn.microsoft.com/en-us/dotnet/api/microsoft.extensions.dependencyinjection.servicedescriptor?view=net-6.0

I didn't come up a graceful solution. Currently, I have two was in my mind:

  1. Breaking change (We may adopt this in v4)

Remove the public Cache property of TimeWindowFilter and put it into the constructor Remove the overload of AddFeatureFilter(Func)

  1. Work around

Ignore TimeWindowFilter in AddFeatureFilter()

The FeatureManagementBuilder is internal, so our implementation of AddFeatureFilter() will only be called after calling AddFeatureManagement() where the TimeWindowFilter should have been registered

zhiyuanliang-ms commented 3 months ago

Hi, @davidstarkcab

Have you tried removing all AddFeatureFilter calls to register built-in feature filters? Did it work for you?

davidstarkcab commented 3 months ago

@zhiyuanliang-ms Yes, now it works perfectly! Many thanks!

zhiyuanliang-ms commented 3 months ago

Microsoft.FeatureManagement 3.3.1 has been released.