Feature/status resolution

[Степан (Stepami)]

Why this PR?

Closes #604

Visible Changes

• IFeatureServiceProvider<TService> interface to get the implementation based on the feature status, maybe using additional context
• WithFeatureService<TService, TEnabled, TDisabled> extension method to setup feature service provider in di
• FeatureServiceProviderOptions class that specifies the keys used by a feature service provider to resolve an implementation based on the feature flag status when keyed di is available.

[Степан (Stepami)]

Hello Zhiyuan Liang (@zhiyuanliang-ms) ! It's been a little while. Could you take a look at my proposal of the FeatureServiceProvider API please?

[Zhiyuan Liang (zhiyuanliang-ms)]

Hey, Степан (@Stepami) Sorry for the late response. I actually discussed this with my colleague. We came up with the following design:

Introduce a new overload WithVariantService<TService, TEnabled, TDisabled>("FeatureFlagName")

The TEnabled and TDisabled are just used to specify the fallback implementation when there is no variant (in this case, the VariantServiceProvider will use feature flag status to retrieve service implementation from DI).

This is very similar to your proposal. But the benefit is that we don't need to introduce a new API with a new name and people can keep using IVaraintServiceProvider.

What do you think?

[Степан (Stepami)]

Hey, Степан (@Stepami) Sorry for the late response. I actually discussed this with my colleague. We came up with the following design:

Introduce a new overload WithVariantService<TService, TEnabled, TDisabled>("FeatureFlagName")

The TEnabled and TDisabled are just used to specify the fallback implementation when there is no variant (in this case, the VariantServiceProvider will use feature flag status to retrieve service implementation from DI).

This is very similar to your proposal. But the benefit is that we don't need to introduce a new API with a new name and people can keep using IVaraintServiceProvider.

What do you think?

Zhiyuan Liang (@zhiyuanliang-ms)

IVariantServiceProvider does not support context propagation for feature filters.

[Jimmy Campbell (jimmyca15)]

Степан (@Stepami)

Are you referring to the lack of a way to pass the filter context in as a parameter to resolve the service? That does seem like a deficiency of IVariantServiceProvider. We can consider how to address it.

[Zhiyuan Liang (zhiyuanliang-ms)]

Hey, Степан (@Stepami)

IVariantServiceProvider does not support context propagation for feature filters.

This is a good call. We missed this for variant evaluation as well. ValueTask GetVariantAsync(string feature, ITargetingContext context, CancellationToken cancellationToken = default)

This makes thing tricky. It will be clean if we can add a new GetService method that takes a context parameter to IVariantServiceProvider. However, this will be a breaking change.

My current thought is that we can introduce a new IContextualVariantServiceProvider:

public interface IContextualVariantServiceProvider<TService> where TService : class
{
        ValueTask<TService> GetServiceAsync<TContext>(TContext context, CancellationToken cancellationToken);
}

We will have the current VariantServiceProvider to implement it as well. And the WithVariantService will then also register IContextualVariantServiceProvider to DI.

[Степан (Stepami)]

Jimmy Campbell (@jimmyca15) yes i am! Zhiyuan Liang (@zhiyuanliang-ms) did a great job of explaining my point.

However, i still do not understand why status based resolution should be bound to variant evaluation

[Zhiyuan Liang (zhiyuanliang-ms)]

However, i still do not understand why status based resolution should be bound to variant evaluation

Hey, Степан (@Stepami)
Variant feature flag is the super set of simple feature flag. For the design simplicity, we want to unify them into variant. If something supports variant, it should also support toggle feature flag.

[Степан (Stepami)]

Zhiyuan Liang (@zhiyuanliang-ms) thanks, i got your point.

Why is addding a new GetService method that takes a context parameter to IVariantServiceProvider a breaking change?

[Zhiyuan Liang (zhiyuanliang-ms)]

Zhiyuan Liang (@zhiyuanliang-ms) thanks, i got your point.

Why is addding a new GetService method that takes a context parameter to IVariantServiceProvider a breaking change?

Hey, Степан (@Stepami)

IVariantServiceProvider is an public interface. If someone is implementing it, then it will be broken. But I am wondering we can avoid this by adding default implementation for the new method. I need to do some investigation.

[Степан (Stepami)]

hey, Zhiyuan Liang (@zhiyuanliang-ms)

IVariantServiceProvider is an public interface. If someone is implementing it, then it will be broken.

That's a good call. Well, there are not many options left. If your goal is to have as little new API as possible, then IContextualVariantServiceProvider is the way to go.

The next question i would ask is whether this interface extends IVariantServiceProvider or not.

But I am wondering we can avoid this by adding default implementation for the new method. I need to do some investigation.

IMHO the only way it works is opposite - non-generic method calls GetServiceAsync<object>(context: null, ct). It's already there in the IFeatureManager impl.

[Zhiyuan Liang (zhiyuanliang-ms)]

Hey, Степан (@Stepami)

The next question i would ask is whether this interface extends IVariantServiceProvider or not.

Yes, IContextualVariantServiceProvider should extend IVariantServiceProvider

But I am wondering we can avoid this by adding default implementation for the new method

I mean: adding a new method for an existing interface with a default implementation to avoid break change.

But since we have aligned with introducing IContextualVariantServiceProvider, this doesn't matter anymore. ;)

non-generic method calls GetServiceAsync(context: null, ct). It's already there in the IFeatureManager impl

I agree.

[Zhiyuan Liang (zhiyuanliang-ms)]

Hey, Степан (@Stepami)

I sent out PR #611 to implement the proposed design.

cc Jimmy Campbell (@jimmyca15) linglingye001