Adds 'Settings' to Context and uses it as a cache of parameters

[rossgrambo]

Rewrite of #180

This PR adds a filter parameters binder interface to allow feature manager to cache bound feature filter and feature variant assignment parameters for performance optimization.

Why

Feature filters are given filter parameters in the form of IConfiguration that instruct them how to perform feature evaluation. Often these parameters are bound to a strongly typed settings object before use. Observed cost of binding IConfiguration to perform feature evaluation is high. By caching the result of the binding, a noticeable performance increase can be achieved. This PR gives Implementers of IFeatureFilter and IFeatureVariantAssigner the option to cache these settings with the help of the feature management system. This is possible through the IFilterParametersBinder and IAssignmentParametersBinder interfaces.

/// <summary>
/// An interface used by the feature management system to pre-bind feature filter parameters to a settings type.
/// <see cref="IFeatureFilter"/>s can implement this interface to take advantage of caching of settings by the feature management system.
/// </summary>
public interface IFilterParametersBinder
{
    /// <summary>
    /// Binds a set of feature filter parameters to a settings object.
    /// </summary>
    /// <param name="parameters">The configuration representing filter parameters to bind to a settings object</param>
    /// <returns>A settings object that is understood by the implementer of <see cref="IFilterParametersBinder"/>.</returns>
    object BindParameters(IConfiguration parameters);
}

Example Usage

Filters offered in this library will benefit from this caching change for anyone using this version moving forward. For custom filters, the class must implement the IFilterParametersBinder interface. Below is an example.

class MyFilter : IFeatureFilter, IFilterParametersBinder
{
    public object BindParameters(IConfiguration filterParameters)
    {
        //
        // Called before evaluation occurs, only if settings are not already in cache.
        return filterParameters.Get<FilterSettings>();
    }

    public Task<bool> EvaluateAsync(FeatureFilterEvaluationContext context, CancellationToken cancellationToken)
    {
        //
        // Get pre-bound/cached settings
        FilterSettings settings = (FilterSettings)context.Settings;

        //
        // Use them for evaluation
        ...
    }
}

Breaking Change

This PR introduces a possible breaking change for custom feature definition providers that implement IFeatureFlagDefinitionProvider. To ensure that any filters that implement IFilterParametersBinder see updates to feature filter parameters when they are changed, definition providers should ensure a new object reference is used for the parameters in the feature definition. Otherwise, the filters will continue to use the same settings from the cache.

Perf

Rough performance assessment showed improvements of ~100 times at least for complex settings objects.

[jimmyca15]

This is our first option that can be invalid (ttl < 0). Do you mind to add some validation in FeatureManager to validate that cache ttl is greater or equal to 0.

[rossgrambo]

Added validation for TTL. Added an error type and a validation method. 95abd68

Let me know if you prefer a different structure or a different error name.

[jimmyca15]

Added validation for TTL. Added an error type and a validation method. 95abd68

Let me know if you prefer a different structure or a different error name.

For constructor error I'd expect simple ArgumentException to suffice.

[rossgrambo]

I just realized that this cacheing doesn't work well with multiple filters. Essentially, we cache FeatureName -> Filter.Parameters.

If I have a 2 Targeting Filters on a single Feature. The parameters of the first one get cached, and then the second filter busts the cache (because the key is the same, but "Parameters" are not the same ref) and it writes a new cache item for those parameters.

A solution here could be to cache parameters for each Filter, not for the Feature. Something like "Feature.1" pointing to the first filter? That should be safe because we know the ref's will be different on each Parameters object, so even if the order of Filters gets moved around, the cache would bust accordingly.

What do y'all think?

[rossgrambo]

This is ready for review again.

Solved the multiple filters cache problem by adding a "FilterIndex" and appending it to the key of the cache.

Also added an internal interface that allows us to cache params only from only our providers. Had to expose this internal interface to the Tests project via InternalsVisibleTo and sign the tests project as well.

[jimmyca15]

I'd like if we have some comment explaining that IFeatureDefinitionProviderCacheable is around purely for satisfying test consumption.

[jimmyca15]

Perhaps also in that types file as well.

[jimmyca15]

Looks like this is purely for our caching needs. The filter really has no use for it. I'd recommend leaving it out and passing the index to BindSettings directly.

[rossgrambo]

I was thinking it would be relevant for telemetry as well (as we'll mark which filter returned true). But I no longer think that we actually need the index for those changes. I'll go ahead with your suggestion.

[jimmyca15]

What do you think about having a separate short circuit like

if (!(_featureDefinitionProvider is IFeatureDefinitionProviderCacheable))
{
    context.Settings = binder.BindParameters(context.Parameters);

    return;
}

Suggesting it because any interaction with cache key or cache doesn't make sense if the definition provider isn't cachable.

[rossgrambo]

Good idea, makes the cache if statement a little cleaner as a result.