Improve non-DI usage of built-in feature filters
Closed this issue · 8 comments
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:
- Built-in feature filter constructors require an
ILoggerFactory
parameter which cannot be null. FeatureManager has a publicILogger
property which can be null. - Currently, the constructor of FeatureManager has a default value for FeatureManagementOptions #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.
@jimmyca15 @rossgrambo @zhenlan What do you think?
- 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)
.
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);
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.
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.
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?
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.
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.