Skip to content

Merges variants and isenabled paths. Adds variant reason field. #290

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.

Sign up for GitHub

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

Merged
zhiyuanliang-ms merged 16 commits into from
Dec 20, 2023
Merged

Merges variants and isenabled paths. Adds variant reason field. #290

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
87b34df
Adds Reason field and adjusts evaluation logic
rossgrambo Oct 26, 2023
e3b2392
merge latest preview branch
zhiyuanliang-ms Dec 11, 2023
6a24277
use enum AssignmentReason
zhiyuanliang-ms Dec 12, 2023
602f32f
add more testcases & remove some interal methods
zhiyuanliang-ms Dec 12, 2023
8e6c14d
fix typo
zhiyuanliang-ms Dec 12, 2023
c1094df
update comments
zhiyuanliang-ms Dec 12, 2023
391fef0
resolve comments
zhiyuanliang-ms Dec 13, 2023
263e284
update comment for DisabledDefault
zhiyuanliang-ms Dec 14, 2023
6ff2a4d
fix typo
zhiyuanliang-ms Dec 14, 2023
ff0b240
update
zhiyuanliang-ms Dec 18, 2023
6660d09
update telemetry publisher
zhiyuanliang-ms Dec 19, 2023
cd1dc64
add reason when no allocation section
zhiyuanliang-ms Dec 19, 2023
b26628a
update
zhiyuanliang-ms Dec 19, 2023
4a3f7cf
remove nullable
zhiyuanliang-ms Dec 19, 2023
3007179
resolve comments
zhiyuanliang-ms Dec 20, 2023
3649fd0
update comments
zhiyuanliang-ms Dec 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
227 changes: 112 additions & 115 deletions src/Microsoft.FeatureManagement/FeatureManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,92 +71,148 @@ public FeatureManager(

public Task<bool> IsEnabledAsync(string feature)
{
return IsEnabledWithVariantsAsync<object>(feature, appContext: null, useAppContext: false, CancellationToken.None).AsTask();
return IsEnabledEvaluation<object>(feature, appContext: null, useAppContext: false, CancellationToken.None).AsTask();
}

public Task<bool> IsEnabledAsync<TContext>(string feature, TContext appContext)
{
return IsEnabledWithVariantsAsync(feature, appContext, useAppContext: true, CancellationToken.None).AsTask();
return IsEnabledEvaluation(feature, appContext, useAppContext: true, CancellationToken.None).AsTask();
}

public ValueTask<bool> IsEnabledAsync(string feature, CancellationToken cancellationToken)
{
return IsEnabledWithVariantsAsync<object>(feature, appContext: null, useAppContext: false, cancellationToken);
return IsEnabledEvaluation<object>(feature, appContext: null, useAppContext: false, cancellationToken);
}

public ValueTask<bool> IsEnabledAsync<TContext>(string feature, TContext appContext, CancellationToken cancellationToken)
{
return IsEnabledWithVariantsAsync(feature, appContext, useAppContext: true, cancellationToken);
return IsEnabledEvaluation(feature, appContext, useAppContext: true, cancellationToken);
}

private async ValueTask<bool> IsEnabledWithVariantsAsync<TContext>(string feature, TContext appContext, bool useAppContext, CancellationToken cancellationToken)
private async ValueTask<bool> IsEnabledEvaluation<TContext>(string feature, TContext appContext, bool useAppContext, CancellationToken cancellationToken)

@zhiyuanliang-ms zhiyuanliang-ms Dec 11, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

According to https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/names-of-type-members#names-of-methods
The method name should be verb. Is the name "IsEnabledEvaluation" a noun?

I suggest "GetIsEnabledEvaluation"

@jimmyca15 jimmyca15 Dec 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We may want to avoid this change given IFeatureManager's main method is IsEnabled. That's not a verb. I don't want to stray to far from that with the internal methods.

@jimmyca15 jimmyca15 Dec 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You raise a good point, perhaps we could have named IsEnabled something like GetXyz in the beginning.

{
bool isFeatureEnabled = false;
EvaluationEvent evaluationEvent = new EvaluationEvent
{
FeatureDefinition = await GetFeatureDefinition(feature).ConfigureAwait(false)
};

FeatureDefinition featureDefinition = await GetFeatureDefinition(feature).ConfigureAwait(false);
await EvaluateFeature(evaluationEvent, appContext, useAppContext, cancellationToken);

VariantDefinition variantDefinition = null;
return evaluationEvent.IsEnabled;
}

if (featureDefinition != null)
public ValueTask<Variant> GetVariantAsync(string feature, CancellationToken cancellationToken)
{
if (string.IsNullOrEmpty(feature))
{
isFeatureEnabled = await IsEnabledAsync(featureDefinition, appContext, useAppContext, cancellationToken).ConfigureAwait(false);
throw new ArgumentNullException(nameof(feature));
}

return GetVariantEvaluation(feature, context: null, useContext: false, cancellationToken);
}

public ValueTask<Variant> GetVariantAsync(string feature, TargetingContext context, CancellationToken cancellationToken)
{
if (string.IsNullOrEmpty(feature))
{
throw new ArgumentNullException(nameof(feature));
}

if (context == null)
{
throw new ArgumentNullException(nameof(context));
}

return GetVariantEvaluation(feature, context, useContext: true, cancellationToken);
}

if (featureDefinition.Variants != null && featureDefinition.Variants.Any() && featureDefinition.Allocation != null)
private async ValueTask<Variant> GetVariantEvaluation(string feature, TargetingContext context, bool useContext, CancellationToken cancellationToken)
{
EvaluationEvent evaluationEvent = new EvaluationEvent
{
FeatureDefinition = await GetFeatureDefinition(feature).ConfigureAwait(false)
};

await EvaluateFeature(evaluationEvent, context, useContext, cancellationToken);
Comment thread
zhiyuanliang-ms marked this conversation as resolved.
Outdated

return evaluationEvent.Variant;
}

private async Task<EvaluationEvent> EvaluateFeature<TContext>(EvaluationEvent evaluationEvent, TContext context, bool useContext, CancellationToken cancellationToken)
{
if (evaluationEvent.FeatureDefinition != null)
{
//
// Determine IsEnabled
evaluationEvent.IsEnabled = await IsEnabledAsync(evaluationEvent.FeatureDefinition, context, useContext, cancellationToken).ConfigureAwait(false);

//
// Determine Variant
VariantDefinition variantDefinition;

if (evaluationEvent.FeatureDefinition.Allocation == null || (!evaluationEvent.FeatureDefinition.Variants?.Any() ?? false))
{
variantDefinition = null;

evaluationEvent.VariantReason = "No Allocation or Variants";
Comment thread
zhiyuanliang-ms marked this conversation as resolved.
Outdated
}
else
{
if (!isFeatureEnabled)
if (!evaluationEvent.IsEnabled)
{
variantDefinition = featureDefinition.Variants.FirstOrDefault((variant) => variant.Name == featureDefinition.Allocation.DefaultWhenDisabled);
variantDefinition = evaluationEvent.FeatureDefinition.Variants.FirstOrDefault((variant) => variant.Name == evaluationEvent.FeatureDefinition.Allocation.DefaultWhenDisabled);

evaluationEvent.VariantReason = "Disabled Default";
}
else
{
TargetingContext targetingContext;

if (useAppContext)
if (useContext)
{
targetingContext = appContext as TargetingContext;
targetingContext = context as TargetingContext;
}
else
{
targetingContext = await ResolveTargetingContextAsync(cancellationToken).ConfigureAwait(false);
}

variantDefinition = await GetAssignedVariantAsync(
featureDefinition,
evaluationEvent,
targetingContext,
cancellationToken)
.ConfigureAwait(false);
}

if (variantDefinition != null && featureDefinition.Status != FeatureStatus.Disabled)
evaluationEvent.Variant = variantDefinition != null ? GetVariantFromVariantDefinition(variantDefinition) : null;

//
// Override IsEnabled if variant has an override
if (variantDefinition != null && evaluationEvent.FeatureDefinition.Status != FeatureStatus.Disabled)
{
if (variantDefinition.StatusOverride == StatusOverride.Enabled)
{
isFeatureEnabled = true;
evaluationEvent.IsEnabled = true;
}
else if (variantDefinition.StatusOverride == StatusOverride.Disabled)
{
isFeatureEnabled = false;
evaluationEvent.IsEnabled = false;
}
}
}
}

foreach (ISessionManager sessionManager in _sessionManagers)
{
await sessionManager.SetAsync(feature, isFeatureEnabled).ConfigureAwait(false);
await sessionManager.SetAsync(evaluationEvent.FeatureDefinition.Name, evaluationEvent.IsEnabled).ConfigureAwait(false);
Comment thread
zhiyuanliang-ms marked this conversation as resolved.
Outdated
}

if (featureDefinition.TelemetryEnabled)
if (evaluationEvent.FeatureDefinition.TelemetryEnabled)
{
PublishTelemetry(new EvaluationEvent
{
FeatureDefinition = featureDefinition,
IsEnabled = isFeatureEnabled,
Variant = variantDefinition != null ? GetVariantFromVariantDefinition(variantDefinition) : null
}, cancellationToken);
PublishTelemetry(evaluationEvent, cancellationToken);
}

return isFeatureEnabled;
return evaluationEvent;
}

public IAsyncEnumerable<string> GetFeatureNamesAsync()
Expand Down Expand Up @@ -304,73 +360,6 @@ await contextualFilter.EvaluateAsync(context, appContext).ConfigureAwait(false)
return enabled;
}

public ValueTask<Variant> GetVariantAsync(string feature, CancellationToken cancellationToken)
{
if (string.IsNullOrEmpty(feature))
{
throw new ArgumentNullException(nameof(feature));
}

return GetVariantAsync(feature, context: null, useContext: false, cancellationToken);
}

public ValueTask<Variant> GetVariantAsync(string feature, TargetingContext context, CancellationToken cancellationToken)
{
if (string.IsNullOrEmpty(feature))
{
throw new ArgumentNullException(nameof(feature));
}

if (context == null)
{
throw new ArgumentNullException(nameof(context));
}

return GetVariantAsync(feature, context, useContext: true, cancellationToken);
}

private async ValueTask<Variant> GetVariantAsync(string feature, TargetingContext context, bool useContext, CancellationToken cancellationToken)
{
FeatureDefinition featureDefinition = await GetFeatureDefinition(feature).ConfigureAwait(false);

if (featureDefinition == null || featureDefinition.Allocation == null || (!featureDefinition.Variants?.Any() ?? false))
{
return null;
}

VariantDefinition variantDefinition = null;

bool isFeatureEnabled = await IsEnabledAsync(featureDefinition, context, useContext, cancellationToken).ConfigureAwait(false);

if (!isFeatureEnabled)
{
variantDefinition = featureDefinition.Variants.FirstOrDefault((variant) => variant.Name == featureDefinition.Allocation.DefaultWhenDisabled);
}
else
{
if (!useContext)
{
context = await ResolveTargetingContextAsync(cancellationToken).ConfigureAwait(false);
}

variantDefinition = await GetAssignedVariantAsync(featureDefinition, context, cancellationToken).ConfigureAwait(false);
}

Variant variant = variantDefinition != null ? GetVariantFromVariantDefinition(variantDefinition) : null;

if (featureDefinition.TelemetryEnabled)
{
PublishTelemetry(new EvaluationEvent
{
FeatureDefinition = featureDefinition,
IsEnabled = isFeatureEnabled,
Variant = variant
}, cancellationToken);
}

return variant;
}

private async ValueTask<FeatureDefinition> GetFeatureDefinition(string feature)
{
FeatureDefinition featureDefinition = await _featureDefinitionProvider
Expand Down Expand Up @@ -415,95 +404,103 @@ private async ValueTask<TargetingContext> ResolveTargetingContextAsync(Cancellat
return context;
}

private async ValueTask<VariantDefinition> GetAssignedVariantAsync(FeatureDefinition featureDefinition, TargetingContext context, CancellationToken cancellationToken)
private async ValueTask<VariantDefinition> GetAssignedVariantAsync(EvaluationEvent evaluationEvent, TargetingContext context, CancellationToken cancellationToken)
{
VariantDefinition variantDefinition = null;

if (context != null)
{
variantDefinition = await AssignVariantAsync(featureDefinition, context, cancellationToken).ConfigureAwait(false);
variantDefinition = await AssignVariantAsync(evaluationEvent, context, cancellationToken).ConfigureAwait(false);
}

if (variantDefinition == null)
{
variantDefinition = featureDefinition.Variants.FirstOrDefault((variant) => variant.Name == featureDefinition.Allocation.DefaultWhenEnabled);
variantDefinition = evaluationEvent.FeatureDefinition.Variants.FirstOrDefault((variant) => variant.Name == evaluationEvent.FeatureDefinition.Allocation.DefaultWhenEnabled);

evaluationEvent.VariantReason = "Enabled Default";
}

return variantDefinition;
}

private ValueTask<VariantDefinition> AssignVariantAsync(FeatureDefinition featureDefinition, TargetingContext targetingContext, CancellationToken cancellationToken)
private ValueTask<VariantDefinition> AssignVariantAsync(EvaluationEvent evaluationEvent, TargetingContext targetingContext, CancellationToken cancellationToken)
{
VariantDefinition variant = null;

@jimmyca15 jimmyca15 Dec 12, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if targetingContext is null, don't call this method.

if (featureDefinition.Allocation.User != null)
if (evaluationEvent.FeatureDefinition.Allocation.User != null)
{
foreach (UserAllocation user in featureDefinition.Allocation.User)
foreach (UserAllocation user in evaluationEvent.FeatureDefinition.Allocation.User)
{
if (TargetingEvaluator.IsTargeted(targetingContext.UserId, user.Users, _assignerOptions.IgnoreCase))
{
if (string.IsNullOrEmpty(user.Variant))
{
_logger.LogWarning($"Missing variant name for user allocation in feature {featureDefinition.Name}");
_logger.LogWarning($"Missing variant name for user allocation in feature {evaluationEvent.FeatureDefinition.Name}");

return new ValueTask<VariantDefinition>((VariantDefinition)null);
}

Debug.Assert(featureDefinition.Variants != null);
Debug.Assert(evaluationEvent.FeatureDefinition.Variants != null);

evaluationEvent.VariantReason = "User Allocated";

return new ValueTask<VariantDefinition>(
featureDefinition
evaluationEvent.FeatureDefinition
.Variants
.FirstOrDefault((variant) => variant.Name == user.Variant));
}
}
}

if (featureDefinition.Allocation.Group != null)
if (evaluationEvent.FeatureDefinition.Allocation.Group != null)
{
foreach (GroupAllocation group in featureDefinition.Allocation.Group)
foreach (GroupAllocation group in evaluationEvent.FeatureDefinition.Allocation.Group)
{
if (TargetingEvaluator.IsTargeted(targetingContext.Groups, group.Groups, _assignerOptions.IgnoreCase))
{
if (string.IsNullOrEmpty(group.Variant))
{
_logger.LogWarning($"Missing variant name for group allocation in feature {featureDefinition.Name}");
_logger.LogWarning($"Missing variant name for group allocation in feature {evaluationEvent.FeatureDefinition.Name}");

return new ValueTask<VariantDefinition>((VariantDefinition)null);
}

Debug.Assert(featureDefinition.Variants != null);
Debug.Assert(evaluationEvent.FeatureDefinition.Variants != null);

evaluationEvent.VariantReason = "Group Allocated";

return new ValueTask<VariantDefinition>(
featureDefinition
evaluationEvent.FeatureDefinition
.Variants
.FirstOrDefault((variant) => variant.Name == group.Variant));
}
}
}

if (featureDefinition.Allocation.Percentile != null)
if (evaluationEvent.FeatureDefinition.Allocation.Percentile != null)
{
foreach (PercentileAllocation percentile in featureDefinition.Allocation.Percentile)
foreach (PercentileAllocation percentile in evaluationEvent.FeatureDefinition.Allocation.Percentile)
{
if (TargetingEvaluator.IsTargeted(
targetingContext,
percentile.From,
percentile.To,
_assignerOptions.IgnoreCase,
featureDefinition.Allocation.Seed ?? $"allocation\n{featureDefinition.Name}"))
evaluationEvent.FeatureDefinition.Allocation.Seed ?? $"allocation\n{evaluationEvent.FeatureDefinition.Name}"))
{
if (string.IsNullOrEmpty(percentile.Variant))
{
_logger.LogWarning($"Missing variant name for percentile allocation in feature {featureDefinition.Name}");
_logger.LogWarning($"Missing variant name for percentile allocation in feature {evaluationEvent.FeatureDefinition.Name}");

return new ValueTask<VariantDefinition>((VariantDefinition)null);
}

Debug.Assert(featureDefinition.Variants != null);
Debug.Assert(evaluationEvent.FeatureDefinition.Variants != null);

evaluationEvent.VariantReason = "Percentile Allocated";

return new ValueTask<VariantDefinition>(
featureDefinition
evaluationEvent.FeatureDefinition
.Variants
.FirstOrDefault((variant) => variant.Name == percentile.Variant));
}
Expand Down
5 changes: 5 additions & 0 deletions src/Microsoft.FeatureManagement/Telemetry/EvaluationEvent.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,10 @@ public class EvaluationEvent
/// The variant given after evaluation.
/// </summary>
public Variant Variant { get; set; }

/// <summary>
/// The reason the variant was given.
/// </summary>
public string VariantReason { get; set; }

@jimmyca15 jimmyca15 Dec 7, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think enum would be more fitting here.

@jimmyca15 jimmyca15 Dec 8, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/// <summary>
/// The reason the variant was assigned.
/// </summary>
public AssignmentReason AssignmentReason

public enum AssignmentReason 
{
    None,
    DisabledDefault,
    EnabledDefault,
    User,
    Group,
    Percentile
}

}
}
Loading