-
Notifications
You must be signed in to change notification settings - Fork 114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds 'Settings' to Context and uses it as a cache of parameters #229
Conversation
48d58a2
to
91617b2
Compare
src/Microsoft.FeatureManagement/FeatureFilters/IFilterParametersBinder.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.FeatureManagement/FeatureFilterEvaluationContext.cs
Outdated
Show resolved
Hide resolved
…mmary. Added explicit check for TimeSpan > zero to skip the cache.
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. |
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. |
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? |
…in and a absolute of 1 day, adds an interface that determines if the parameter cache should be used
…om/microsoft/FeatureManagement-Dotnet into rossgrambo/cache-filter-parameters
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 |
…t exposes it to the tests project. Also now signs the test project.
7e27353
to
d2209ae
Compare
ad6ab10
to
c18384c
Compare
@@ -15,7 +15,7 @@ namespace Microsoft.FeatureManagement | |||
/// <summary> | |||
/// A feature definition provider that pulls feature definitions from the .NET Core <see cref="IConfiguration"/> system. | |||
/// </summary> | |||
sealed class ConfigurationFeatureDefinitionProvider : IFeatureDefinitionProvider, IDisposable | |||
sealed class ConfigurationFeatureDefinitionProvider : IFeatureDefinitionProvider, IDisposable, IFeatureDefinitionProviderCacheable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like if we have some comment explaining that IFeatureDefinitionProviderCacheable
is around purely for satisfying test consumption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps also in that types file as well.
@@ -135,6 +160,7 @@ private async Task<bool> IsEnabledAsync<TContext>(string feature, TContext appCo | |||
var context = new FeatureFilterEvaluationContext() | |||
{ | |||
FeatureName = feature, | |||
FilterIndex = filterIndex, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
{ | ||
IFilterParametersBinder binder = filter as IFilterParametersBinder; | ||
|
||
if (binder == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, makes the cache if statement a little cleaner as a result.
…eans up FeatureManager.BindSettings logic
…om/microsoft/FeatureManagement-Dotnet into rossgrambo/cache-filter-parameters
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 bindingIConfiguration
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 theIFilterParametersBinder
andIAssignmentParametersBinder
interfaces.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.
Breaking Change
This PR introduces a possible breaking change for custom feature definition providers that implement
IFeatureFlagDefinitionProvider
. To ensure that any filters that implementIFilterParametersBinder
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.