Improve non-DI usage of built-in feature filters

[zhiyuanliang-ms]

After Microsoft.FeatureManagement 3.1.0, we exposed FeatureManager and ConfigurationFeatureDefintionProvider classes to public and the non-DI usage of Feature Management is supported:

IConfiguration configuration = new ConfigurationBuilder()
    .AddJsonFile("appsettings.json")
    .Build();

var featureDefintionProvider = new ConfigurationFeatureDefinitionProvider(configuration);

var featureManager = new FeatureManager(featureDefintionProvider)();

bool res = await featureManager.IsEnabledAsync("MyFeature");

However, the built-in feature filters are designed for DI usage originally. If users want to use feature filters in non-DI Feature Management. The usage will be like:

IConfiguration configuration = new ConfigurationBuilder()
    .AddJsonFile("appsettings.json")
    .Build();

var featureDefintionProvider = new ConfigurationFeatureDefinitionProvider(configuration);

ILoggerFactory loggerFactory = new LoggerFactory();

var targetingFilter = new ContextualTargetingFilter(Options.Create(new TargetingEvaluationOptions()), loggerFactory);

var timeWindowFilter = new TimeWindowFilter(loggerFactory);

var featureManager = new FeatureManager(featureDefintionProvider)
{
    FeatureFilters = new List<IFeatureFilterMetadata>() { targetingFilter, timeWindowFilter },
    Logger = loggerFactory.CreateLogger<FeatureManager>()
};

bool res = await featureManager.IsEnabledAsync("MyFeature");

The current non-DI usage looks kind of messy.

Notice that the current constructor of built-in filters are:

public PercentageFilter(ILoggerFactory loggerFactory)
{
    _logger = loggerFactory.CreateLogger<PercentageFilter>();
}

public TimeWindowFilter(ILoggerFactory loggerFactory)
{
    _logger = loggerFactory?.CreateLogger<TimeWindowFilter>() ?? throw new ArgumentNullException(nameof(loggerFactory));
}

public ContextualTargetingFilter(IOptions<TargetingEvaluationOptions> options, ILoggerFactory loggerFactory)
{
    _options = options?.Value ?? throw new ArgumentNullException(nameof(options));
    _logger = loggerFactory?.CreateLogger<ContextualTargetingFilter>() ?? throw new ArgumentNullException(nameof(loggerFactory));
}

public TargetingFilter(IOptions<TargetingEvaluationOptions> options, ITargetingContextAccessor contextAccessor, ILoggerFactory loggerFactory)
{
    _contextAccessor = contextAccessor ?? throw new ArgumentNullException(nameof(contextAccessor));
    _contextualFilter = new ContextualTargetingFilter(options, loggerFactory);
    _logger = loggerFactory?.CreateLogger<TargetingFilter>() ?? throw new ArgumentNullException(nameof(loggerFactory));
}

There are two inconsistent things:

1. Built-in feature filter constructors require an ILoggerFactory parameter which cannot be null. FeatureManager has a public ILogger property which can be null.
2. Currently, the constructor of FeatureManager has a default value for FeatureManagementOptions Add a constructor without FeatureManagementOption parameter for FeatureManager #363. Targeting filters still require an IOptions parameter which does not have a default value.

I propose that we should at least support such non-DI usage for built-in feature filters:

IConfiguration configuration = new ConfigurationBuilder()
    .AddJsonFile("appsettings.json")
    .Build();

var featureDefintionProvider = new ConfigurationFeatureDefinitionProvider(configuration);

var targetingFilter = new ContextualTargetingFilter();

var timeWindowFilter = new TimeWindowFilter();

var featureManager = new FeatureManager(featureDefintionProvider)
{
    FeatureFilters = new List<IFeatureFilterMetadata>() { targetingFilter, timeWindowFilter }
};

bool res = await featureManager.IsEnabledAsync("MyFeature");

by enabling logger of built-in filters to be null and giving a default value for TargetingEvaluationOptions.

This change will make non-DI usage simpler. Another reason we decided to expose FeatureManager class is to allow third-party DI containers to use FeatureManagement. The current usage of FeatureManagement with third party DI containers can be found here. With this change, we can also make it easier for third party DI containers to use FeatureManagement.

My further thought is to make built-in feature filters have the same pattern as FeatureManager where Logger and TargetingEvaluationOptions are public properties instead of parameters of constructor.

[zhiyuanliang-ms]

@jimmyca15 @rossgrambo @zhenlan What do you think?

[zhenlan]

• It's reasonable for the logger parameter to be optional.
• Should we add built-in filters automatically?
• It will be nice to have featureManager.FeatureFilters.Add(IFeatureFilter).

[zhiyuanliang-ms]

@zhenlan

It's reasonable for the logger parameter to be optional.

Agree.

Should we add built-in filters automatically?

For DI usage, built-in filters have already been added automatically. Did you mean we should add them automatically when feature manager is constructed?

It will be nice to have featureManager.FeatureFilters.Add(IFeatureFilter).

Do you mean that it would be better to have the usage look like:

var timeWindowFilter = new TimeWindowFilter();

var featureManager = new FeatureManager(featureDefintionProvider);

featureManager.FeatureFilters.Add(timeWindowFilter);

[zhenlan]

Should we add built-in filters automatically?

For DI usage, built-in filters have already been added automatically. Did you mean we should add them automatically when feature manager is constructed?

Right, for non-DI usage.

It will be nice to have featureManager.FeatureFilters.Add(IFeatureFilter).

Do you mean that it would be better to have the usage look like:

var timeWindowFilter = new TimeWindowFilter();

var featureManager = new FeatureManager(featureDefintionProvider);

featureManager.FeatureFilters.Add(timeWindowFilter);

I wish people couldn't set featureManager.FeatureFilters but add items.

[zhiyuanliang-ms]

After discussed with Jimmy, we thought we should keep the current usage:

var featureManager = new FeatureManager(featureDefintionProvider)
{
    FeatureFilters = new List<IFeatureFilterMetadata>() { targetingFilter, timeWindowFilter }
};

Currently, FeatureFilters is a public property with the init accessor. The reason of this design is the idea that we want feature filters to be static after FeatureManager is constructed.

var featureManager = new FeatureManager(featureDefintionProvider);

featureManager.FeatureFilters.Add(timeWindowFilter);

This usage will break this idea.

@jimmyca15 @zhenlan

[zhenlan]

I understand featureManager.FeatureFilters cannot be set to another List object once it's initialized, but can't people call Add on the existing list object anyway?

[jimmyca15]

@zhenlan

but can't people call Add on the existing list object anyway

FeatureManager.FeatureFilters is an IEnumerable, no filters can be added. By design so that the feature manager is constructed as intended from the onset.

[zhenlan]

I see. Thanks. I don't have any better ideas, but kind of a bummer users have to know IFeatureFilterMetadata even just to use built-in filters.