Skip to content
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

Use ITargetingContext when calling GetVariantAsync #484

Merged
merged 11 commits into from
Aug 26, 2024
10 changes: 5 additions & 5 deletions src/Microsoft.FeatureManagement/FeatureManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -225,10 +225,10 @@ public async ValueTask<Variant> GetVariantAsync(string feature, CancellationToke
/// Gets the assigned variant for a specific feature.
zhiyuanliang-ms marked this conversation as resolved.
Show resolved Hide resolved
/// </summary>
/// <param name="feature">The name of the feature to evaluate.</param>
/// <param name="context">An instance of <see cref="TargetingContext"/> used to evaluate which variant the user will be assigned.</param>
/// <param name="context">A context providing information that can used to evaluate which variant the user will be assigned.</param>
zhiyuanliang-ms marked this conversation as resolved.
Show resolved Hide resolved
/// <param name="cancellationToken">The cancellation token to cancel the operation.</param>
/// <returns>A variant assigned to the user based on the feature's configured allocation.</returns>
public async ValueTask<Variant> GetVariantAsync(string feature, TargetingContext context, CancellationToken cancellationToken = default)
public async ValueTask<Variant> GetVariantAsync<TContext>(string feature, TContext context, CancellationToken cancellationToken = default)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it required for TContext to implement ITargetingContext for this API to work? If so, what's the point to have TContext?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While GetVariantAsync, the first step is to evaluate the feature flag state according to the feature filter.

If the user uses a contextual feature filter which requires TContext for filter evaluation.
TContext is not necessarily ITargetingContext. The feature manager will still use it to evaluate whether feature flag is enabled or disabled.
And he can also use two variants through DefaultWhenEnabled and DefaultWhenDisabled which don't require TargetingContext to do the variant allocation.

Previously, we only allow user to pass TargetingContext which will make it impossible to use contextual filter.

How about this?

If the TContext doesn't implement ITargetingContext, the feature manager will still give it another chance to get targeting context from the targeting context accessor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are talking about an edge case where ITargetingContext may not be necessary, but general speaking, ITargetingContext is required for variant assignment. We should enforce the context to implement ITargetingContext.

If the TContext doesn't implement ITargetingContext, the feature manager will still give it another chance to get targeting context from the targeting context accessor.

No, we shouldn't use ambient context in contextual feature evaluation.

Copy link
Contributor Author

@zhiyuanliang-ms zhiyuanliang-ms Aug 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ITargetingContext is required for variant assignment. We should enforce the context to implement ITargetingContext.

ITargetingContext is a public interface, we cannot modify it to avoid breaking changes.
If in the future, we want to add new fields in targeting context, we may create a new interface called IAttributeTargetingContext. If we enforce the context to implement ITargetingContext here, we will constrain ourselves.

TargetingContext should be sealed. We had an oversight on it. I created an issue #486 Currently, we are in an very uncomfortable spot that we cannot modify anything related to targeting context to avoid potential breaking changes.

Copy link
Contributor

@rossgrambo rossgrambo Aug 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't think Variants need to be fully tied to targeting. We don't support it yet- but it seems quite reasonable to want variants without wanting user based allocation.

Making it TContext also aligns it with IsEnabled, which is more consistent. IsEnabled is also able to use Allocation in the same way that GetVariant is. So whatever we decide on- the two interfaces should use the same.

{
if (string.IsNullOrEmpty(feature))
{
Expand Down Expand Up @@ -262,11 +262,11 @@ private async ValueTask<EvaluationEvent> EvaluateFeature<TContext>(string featur

//
// Determine Targeting Context
TargetingContext targetingContext;
ITargetingContext targetingContext;

if (useContext)
{
targetingContext = context as TargetingContext;
targetingContext = context as ITargetingContext;
}
else
{
Expand Down Expand Up @@ -601,7 +601,7 @@ private async ValueTask<TargetingContext> ResolveTargetingContextAsync(Cancellat
return context;
}

private ValueTask<VariantDefinition> AssignVariantAsync(EvaluationEvent evaluationEvent, TargetingContext targetingContext, CancellationToken cancellationToken)
private ValueTask<VariantDefinition> AssignVariantAsync(EvaluationEvent evaluationEvent, ITargetingContext targetingContext, CancellationToken cancellationToken)
{
Debug.Assert(evaluationEvent != null);

Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.FeatureManagement/FeatureManagerSnapshot.cs
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public async ValueTask<Variant> GetVariantAsync(string feature, CancellationToke
return variant;
}

public async ValueTask<Variant> GetVariantAsync(string feature, TargetingContext context, CancellationToken cancellationToken)
public async ValueTask<Variant> GetVariantAsync<TContext>(string feature, TContext context, CancellationToken cancellationToken)
{
string cacheKey = GetVariantCacheKey(feature);

Expand Down
4 changes: 2 additions & 2 deletions src/Microsoft.FeatureManagement/IVariantFeatureManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,9 @@ public interface IVariantFeatureManager
/// Gets the assigned variant for a specific feature.
/// </summary>
/// <param name="feature">The name of the feature to evaluate.</param>
/// <param name="context">An instance of <see cref="TargetingContext"/> used to evaluate which variant the user will be assigned.</param>
/// <param name="context">A context providing information that can used to evaluate which variant the user will be assigned.</param>
zhiyuanliang-ms marked this conversation as resolved.
Show resolved Hide resolved
/// <param name="cancellationToken">The cancellation token to cancel the operation.</param>
/// <returns>A variant assigned to the user based on the feature's configured allocation.</returns>
ValueTask<Variant> GetVariantAsync(string feature, TargetingContext context, CancellationToken cancellationToken = default);
ValueTask<Variant> GetVariantAsync<TContext>(string feature, TContext context, CancellationToken cancellationToken = default);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ public class EvaluationEvent
/// <summary>
/// The targeting context used to evaluate the feature.
/// </summary>
public TargetingContext TargetingContext { get; set; }
public ITargetingContext TargetingContext { get; set; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to keep this as TargetingContext to enable easy expansion. We should be able to create a new instance and copy over in FeatureManager.


/// <summary>
/// The enabled state of the feature after evaluation.
Expand Down
28 changes: 28 additions & 0 deletions src/Microsoft.FeatureManagement/VariantFeatureManagerExtensions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.
//
using Microsoft.FeatureManagement.FeatureFilters;
using System.Threading;
using System.Threading.Tasks;

namespace Microsoft.FeatureManagement
{
/// <summary>
/// Extensions used to provide an overload for <see cref="IVariantFeatureManager.GetVariantAsync"/> that accepts a <see cref="TargetingContext"/> type directly.
/// </summary>
public static class VariantFeatureManagerExtensions
{
/// <summary>
/// Gets the assigned variant for a specific feature.
/// </summary>
/// <param name="variantFeatureManager">The <see cref="IVariantFeatureManager"/> instance.</param>
/// <param name="feature">The name of the feature to evaluate.</param>
/// <param name="context">An instance of <see cref="TargetingContext"/> used to evaluate which variant the user will be assigned.</param>
/// <param name="cancellationToken">The cancellation token to cancel the operation.</param>
/// <returns>A variant assigned to the user based on the feature's configured allocation.</returns>
public static ValueTask<Variant> GetVariantAsync(this IVariantFeatureManager variantFeatureManager, string feature, TargetingContext context, CancellationToken cancellationToken = default)
{
return variantFeatureManager.GetVariantAsync(feature, context, cancellationToken);
}
}
}
Loading