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 9 commits
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
28 changes: 27 additions & 1 deletion ....FeatureManagement.Telemetry.ApplicationInsights/ApplicationInsightsTelemetryPublisher.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ public ValueTask PublishEvent(EvaluationEvent evaluationEvent, CancellationToken

FeatureDefinition featureDefinition = evaluationEvent.FeatureDefinition;

Dictionary<string, string> properties = new Dictionary<string, string>()
var properties = new Dictionary<string, string>()
{
{ "FeatureName", featureDefinition.Name },
{ "IsEnabled", evaluationEvent.IsEnabled.ToString() }
Expand All @@ -41,6 +41,11 @@ public ValueTask PublishEvent(EvaluationEvent evaluationEvent, CancellationToken
properties["Variant"] = evaluationEvent.Variant.Name;
}

if (evaluationEvent.AssignmentReason != AssignmentReason.None)

@zhiyuanliang-ms zhiyuanliang-ms Dec 14, 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 want to call out that the ApplicationInsightsTelemetryPublisher will not send AssignmentReason telemetry if the AssignmentReason is None (which will only occur when the variant is null).

@zhenlan zhenlan Dec 14, 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.

Then it doesn't sound right. We should send the telemetry. This is a case people will want to know that the feature flag is misconfigured.

@jimmyca15 jimmyca15 Dec 14, 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.

This is a case people will want to know that the feature flag is misconfigured.

I'm not sure what you intend to be discovered here. What would you expect "AssignmentReason" to be on a basic on/off feature flag? We already don't send variant in this case. I'm not sure what meaning "AssignmentReason" would have if no variant has been assigned.

@zhenlan zhenlan Dec 14, 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.

Sync'ed up with Jimmy offline and Jimmy help clarify the scenario. I was confused by the comment for None: No variant is assigned. None actually indicates the variant evaluation never occurs so the assignment is irrelevant. The assignment reason is more like "NA" in this case. We could update the comment to make it clearer.

{
properties["AssignmentReason"] = ToString(evaluationEvent.AssignmentReason);
}

if (featureDefinition.TelemetryMetadata != null)
{
foreach (KeyValuePair<string, string> kvp in featureDefinition.TelemetryMetadata)
Expand All @@ -66,5 +71,26 @@ private void ValidateEvent(EvaluationEvent evaluationEvent)
throw new ArgumentNullException(nameof(evaluationEvent.FeatureDefinition));
}
}

private static string ToString(AssignmentReason reason)
{
Comment thread
zhiyuanliang-ms marked this conversation as resolved.
const string None = "None";
Comment thread
zhiyuanliang-ms marked this conversation as resolved.
Outdated
const string DisabledDefault = "DisabledDefault";
const string EnabledDefault = "EnabledDefault";
const string User = "User";
const string Group = "Group";
const string Percentile = "Percentile";

return reason switch
{
AssignmentReason.None => None,
AssignmentReason.DisabledDefault => DisabledDefault,
AssignmentReason.EnabledDefault => EnabledDefault,
AssignmentReason.User => User,
AssignmentReason.Group => Group,
AssignmentReason.Percentile => Percentile,
_ => throw new ArgumentException("Invalid assignment reason.", nameof(reason))
};
}
}
}
247 changes: 121 additions & 126 deletions src/Microsoft.FeatureManagement/FeatureManager.cs
View file
Open in desktop

Large diffs are not rendered by default.

41 changes: 41 additions & 0 deletions src/Microsoft.FeatureManagement/Telemetry/AssignmentReason.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.
//
namespace Microsoft.FeatureManagement.Telemetry
{
/// <summary>
/// The reason the variant was assigned during the evaluation of a feature.
/// </summary>
public enum AssignmentReason

@zhenlan zhenlan Dec 14, 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.

Not sure if it's just me feeling this confusing. No where in this enum the variant is mentioned. For example, AssignmentReason.None sounds like it's saying "something is assigned with no reason". Don't ask me to guess what AssignmentReason.DisabledDefault means :) The list is great, but can we come up with something that is more intuitive?

@zhiyuanliang-ms zhiyuanliang-ms Dec 14, 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 can change the enum name to VariantAssignmentReason. Is that enough?

AssignmentReason.None is the default value of the enum. When there is no variant assigned, the reason will be None. Maybe, we can change it to NotAssigned?

@zhiyuanliang-ms zhiyuanliang-ms Dec 14, 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.

DisabledDefault is in our feature flag schema. Only when you config the DisabledDefault section, you can recieve this reason field in the telemetry. I don't think people may feel confused when seeing "AssignmentReason": "DisabledDefault" in telemetry.

{
/// <summary>
/// No variant is assigned.
/// </summary>
None,

/// <summary>
/// Variant is assigned by default when the feature flag is disabled.
/// </summary>
DisabledDefault,

/// <summary>
/// Variant is assigned by default after processing the user/group/percentile allocation, when the feature flag is enabled.
/// </summary>
EnabledDefault,

/// <summary>
/// Variant is assigned because of the user allocation.
/// </summary>
User,

/// <summary>
/// Variant is assigned because of the group allocation.
/// </summary>
Group,

/// <summary>
/// Variant is assigned because of the percentile allocation.
/// </summary>
Percentile
}
}
7 changes: 5 additions & 2 deletions src/Microsoft.FeatureManagement/Telemetry/EvaluationEvent.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.
//
using Microsoft.FeatureManagement.FeatureFilters;

namespace Microsoft.FeatureManagement.Telemetry
{
/// <summary>
Expand All @@ -24,5 +22,10 @@ public class EvaluationEvent
/// The variant given after evaluation.
/// </summary>
public Variant Variant { get; set; }

/// <summary>
/// The reason the variant was assigned.
/// </summary>
public AssignmentReason AssignmentReason { get; set; }
}
}
115 changes: 69 additions & 46 deletions tests/Tests.FeatureManagement/FeatureManagement.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using Microsoft.Extensions.DependencyInjection;
using Microsoft.FeatureManagement;
using Microsoft.FeatureManagement.FeatureFilters;
using Microsoft.FeatureManagement.Telemetry;
using Microsoft.FeatureManagement.Tests;
using System;
using System.Collections.Generic;
Expand Down Expand Up @@ -355,7 +356,7 @@ public async Task Percentage()
}
}

Assert.True(enabledCount >= 0 && enabledCount < 10);
Assert.True(enabledCount >= 0 && enabledCount <= 10);
}

[Fact]
Expand Down Expand Up @@ -1010,11 +1011,11 @@ public async Task TelemetryPublishing()

var services = new ServiceCollection();

services
var targetingContextAccessor = new OnDemandTargetingContextAccessor();
services.AddSingleton<ITargetingContextAccessor>(targetingContextAccessor)
.AddSingleton(config)
.AddFeatureManagement()
.AddTelemetryPublisher<TestTelemetryPublisher>()
.AddFeatureFilter<TimeWindowFilter>();
.AddTelemetryPublisher<TestTelemetryPublisher>();

ServiceProvider serviceProvider = services.BuildServiceProvider();

Expand All @@ -1028,55 +1029,80 @@ public async Task TelemetryPublishing()
Assert.Null(testPublisher.evaluationEventCache);

// Test telemetry cases
const string onFeature = "AlwaysOnTestFeature";

result = await featureManager.IsEnabledAsync(onFeature, CancellationToken.None);
result = await featureManager.IsEnabledAsync(Features.AlwaysOnTestFeature, CancellationToken.None);

Assert.True(result);
Assert.Equal(onFeature, testPublisher.evaluationEventCache.FeatureDefinition.Name);
Assert.Equal(Features.AlwaysOnTestFeature, testPublisher.evaluationEventCache.FeatureDefinition.Name);
Assert.Equal(result, testPublisher.evaluationEventCache.IsEnabled);
Assert.Equal("EtagValue", testPublisher.evaluationEventCache.FeatureDefinition.TelemetryMetadata["Etag"]);
Assert.Equal("LabelValue", testPublisher.evaluationEventCache.FeatureDefinition.TelemetryMetadata["Label"]);
Assert.Equal("Tag1Value", testPublisher.evaluationEventCache.FeatureDefinition.TelemetryMetadata["Tags.Tag1"]);
Assert.Null(testPublisher.evaluationEventCache.Variant);
Assert.Equal(AssignmentReason.None, testPublisher.evaluationEventCache.AssignmentReason);

const string offFeature = "OffTimeTestFeature";

result = await featureManager.IsEnabledAsync(offFeature, CancellationToken.None);
result = await featureManager.IsEnabledAsync(Features.OffTimeTestFeature, CancellationToken.None);

Assert.False(result);
Assert.Equal(offFeature, testPublisher.evaluationEventCache.FeatureDefinition.Name);
Assert.Equal(Features.OffTimeTestFeature, testPublisher.evaluationEventCache.FeatureDefinition.Name);
Assert.Equal(result, testPublisher.evaluationEventCache.IsEnabled);
Assert.Equal(AssignmentReason.None, testPublisher.evaluationEventCache.AssignmentReason);

// Test variant cases
const string variantDefaultEnabledFeature = "VariantFeatureDefaultEnabled";

result = await featureManager.IsEnabledAsync(variantDefaultEnabledFeature, CancellationToken.None);
result = await featureManager.IsEnabledAsync(Features.VariantFeatureDefaultEnabled, CancellationToken.None);

Assert.True(result);
Assert.Equal(variantDefaultEnabledFeature, testPublisher.evaluationEventCache.FeatureDefinition.Name);
Assert.Equal(Features.VariantFeatureDefaultEnabled, testPublisher.evaluationEventCache.FeatureDefinition.Name);
Assert.Equal(result, testPublisher.evaluationEventCache.IsEnabled);
Assert.Equal("Medium", testPublisher.evaluationEventCache.Variant.Name);

Variant variantResult = await featureManager.GetVariantAsync(variantDefaultEnabledFeature, CancellationToken.None);
Variant variantResult = await featureManager.GetVariantAsync(Features.VariantFeatureDefaultEnabled, CancellationToken.None);

Assert.True(testPublisher.evaluationEventCache.IsEnabled);
Assert.Equal(variantDefaultEnabledFeature, testPublisher.evaluationEventCache.FeatureDefinition.Name);
Assert.Equal(Features.VariantFeatureDefaultEnabled, testPublisher.evaluationEventCache.FeatureDefinition.Name);
Assert.Equal(variantResult.Name, testPublisher.evaluationEventCache.Variant.Name);
Assert.Equal(AssignmentReason.EnabledDefault, testPublisher.evaluationEventCache.AssignmentReason);

string variantFeatureStatusDisabled = "VariantFeatureStatusDisabled";

result = await featureManager.IsEnabledAsync(variantFeatureStatusDisabled, CancellationToken.None);
result = await featureManager.IsEnabledAsync(Features.VariantFeatureStatusDisabled, CancellationToken.None);

Assert.False(result);
Assert.Equal(variantFeatureStatusDisabled, testPublisher.evaluationEventCache.FeatureDefinition.Name);
Assert.Equal(Features.VariantFeatureStatusDisabled, testPublisher.evaluationEventCache.FeatureDefinition.Name);
Assert.Equal(result, testPublisher.evaluationEventCache.IsEnabled);
Assert.Equal("Small", testPublisher.evaluationEventCache.Variant.Name);
Assert.Equal(AssignmentReason.DisabledDefault, testPublisher.evaluationEventCache.AssignmentReason);

variantResult = await featureManager.GetVariantAsync(variantFeatureStatusDisabled, CancellationToken.None);
variantResult = await featureManager.GetVariantAsync(Features.VariantFeatureStatusDisabled, CancellationToken.None);

Assert.False(testPublisher.evaluationEventCache.IsEnabled);
Assert.Equal(variantFeatureStatusDisabled, testPublisher.evaluationEventCache.FeatureDefinition.Name);
Assert.Equal(Features.VariantFeatureStatusDisabled, testPublisher.evaluationEventCache.FeatureDefinition.Name);
Assert.Equal(variantResult.Name, testPublisher.evaluationEventCache.Variant.Name);
Assert.Equal(AssignmentReason.DisabledDefault, testPublisher.evaluationEventCache.AssignmentReason);

targetingContextAccessor.Current = new TargetingContext
{
UserId = "Marsha",
Groups = new List<string> { "Group1" }
};

variantResult = await featureManager.GetVariantAsync(Features.VariantFeaturePercentileOn, CancellationToken.None);
Assert.Equal("Big", variantResult.Name);
Assert.Equal("Big", testPublisher.evaluationEventCache.Variant.Name);
Assert.Equal(AssignmentReason.Percentile, testPublisher.evaluationEventCache.AssignmentReason);

variantResult = await featureManager.GetVariantAsync(Features.VariantFeaturePercentileOff, CancellationToken.None);
Assert.Null(variantResult);
Assert.Null(testPublisher.evaluationEventCache.Variant);
Assert.Equal(AssignmentReason.None, testPublisher.evaluationEventCache.AssignmentReason);

variantResult = await featureManager.GetVariantAsync(Features.VariantFeatureUser, CancellationToken.None);
Assert.Equal("Small", variantResult.Name);
Assert.Equal("Small", testPublisher.evaluationEventCache.Variant.Name);
Assert.Equal(AssignmentReason.User, testPublisher.evaluationEventCache.AssignmentReason);

variantResult = await featureManager.GetVariantAsync(Features.VariantFeatureGroup, CancellationToken.None);
Assert.Equal("Small", variantResult.Name);
Assert.Equal("Small", testPublisher.evaluationEventCache.Variant.Name);
Assert.Equal(AssignmentReason.Group, testPublisher.evaluationEventCache.AssignmentReason);

}

[Fact]
Expand All @@ -1088,17 +1114,14 @@ public async Task TelemetryPublishingNullPublisher()

services
.AddSingleton(config)
.AddFeatureManagement()
.AddFeatureFilter<TimeWindowFilter>();
.AddFeatureManagement();

ServiceProvider serviceProvider = services.BuildServiceProvider();

FeatureManager featureManager = (FeatureManager)serviceProvider.GetRequiredService<IVariantFeatureManager>();

// Test telemetry enabled feature with no telemetry publisher
string onFeature = "AlwaysOnTestFeature";

bool result = await featureManager.IsEnabledAsync(onFeature, CancellationToken.None);
bool result = await featureManager.IsEnabledAsync(Features.AlwaysOnTestFeature, CancellationToken.None);

Assert.True(result);
}
Expand Down Expand Up @@ -1127,44 +1150,44 @@ public async Task UsesVariants()
};

// Test StatusOverride and Percentile with Seed
Variant variant = await featureManager.GetVariantAsync("VariantFeaturePercentileOn", cancellationToken);
Variant variant = await featureManager.GetVariantAsync(Features.VariantFeaturePercentileOn, cancellationToken);

Assert.Equal("Big", variant.Name);
Assert.Equal("green", variant.Configuration["Color"]);
Assert.False(await featureManager.IsEnabledAsync("VariantFeaturePercentileOn", cancellationToken));
Assert.False(await featureManager.IsEnabledAsync(Features.VariantFeaturePercentileOn, cancellationToken));

variant = await featureManager.GetVariantAsync("VariantFeaturePercentileOff", cancellationToken);
variant = await featureManager.GetVariantAsync(Features.VariantFeaturePercentileOff, cancellationToken);

Assert.Null(variant);
Assert.True(await featureManager.IsEnabledAsync("VariantFeaturePercentileOff", cancellationToken));
Assert.True(await featureManager.IsEnabledAsync(Features.VariantFeaturePercentileOff, cancellationToken));

// Test Status = Disabled
variant = await featureManager.GetVariantAsync("VariantFeatureStatusDisabled", cancellationToken);
variant = await featureManager.GetVariantAsync(Features.VariantFeatureStatusDisabled, cancellationToken);

Assert.Equal("Small", variant.Name);
Assert.Equal("300px", variant.Configuration.Value);
Assert.False(await featureManager.IsEnabledAsync("VariantFeatureStatusDisabled", cancellationToken));
Assert.False(await featureManager.IsEnabledAsync(Features.VariantFeatureStatusDisabled, cancellationToken));

// Test DefaultWhenEnabled and ConfigurationValue with inline IConfigurationSection
variant = await featureManager.GetVariantAsync("VariantFeatureDefaultEnabled", cancellationToken);
variant = await featureManager.GetVariantAsync(Features.VariantFeatureDefaultEnabled, cancellationToken);

Assert.Equal("Medium", variant.Name);
Assert.Equal("450px", variant.Configuration["Size"]);
Assert.True(await featureManager.IsEnabledAsync("VariantFeatureDefaultEnabled", cancellationToken));
Assert.True(await featureManager.IsEnabledAsync(Features.VariantFeatureDefaultEnabled, cancellationToken));

// Test User allocation
variant = await featureManager.GetVariantAsync("VariantFeatureUser", cancellationToken);
variant = await featureManager.GetVariantAsync(Features.VariantFeatureUser, cancellationToken);

Assert.Equal("Small", variant.Name);
Assert.Equal("300px", variant.Configuration.Value);
Assert.True(await featureManager.IsEnabledAsync("VariantFeatureUser", cancellationToken));
Assert.True(await featureManager.IsEnabledAsync(Features.VariantFeatureUser, cancellationToken));

// Test Group allocation
variant = await featureManager.GetVariantAsync("VariantFeatureGroup", cancellationToken);
variant = await featureManager.GetVariantAsync(Features.VariantFeatureGroup, cancellationToken);

Assert.Equal("Small", variant.Name);
Assert.Equal("300px", variant.Configuration.Value);
Assert.True(await featureManager.IsEnabledAsync("VariantFeatureGroup", cancellationToken));
Assert.True(await featureManager.IsEnabledAsync(Features.VariantFeatureGroup, cancellationToken));
}

[Fact]
Expand All @@ -1190,24 +1213,24 @@ public async Task VariantsInvalidScenarios()
CancellationToken cancellationToken = CancellationToken.None;

// Verify null variant returned if no variants are specified
Variant variant = await featureManager.GetVariantAsync("VariantFeatureNoVariants", cancellationToken);
Variant variant = await featureManager.GetVariantAsync(Features.VariantFeatureNoVariants, cancellationToken);

Assert.Null(variant);

// Verify null variant returned if no allocation is specified
variant = await featureManager.GetVariantAsync("VariantFeatureNoAllocation", cancellationToken);
variant = await featureManager.GetVariantAsync(Features.VariantFeatureNoAllocation, cancellationToken);

Assert.Null(variant);

// Verify that ConfigurationValue has priority over ConfigurationReference
variant = await featureManager.GetVariantAsync("VariantFeatureBothConfigurations", cancellationToken);
variant = await featureManager.GetVariantAsync(Features.VariantFeatureBothConfigurations, cancellationToken);

Assert.Equal("600px", variant.Configuration.Value);

// Verify that an exception is thrown for invalid StatusOverride value
FeatureManagementException e = await Assert.ThrowsAsync<FeatureManagementException>(async () =>
{
variant = await featureManager.GetVariantAsync("VariantFeatureInvalidStatusOverride", cancellationToken);
variant = await featureManager.GetVariantAsync(Features.VariantFeatureInvalidStatusOverride, cancellationToken);
});

Assert.Equal(FeatureManagementError.InvalidConfigurationSetting, e.Error);
Expand All @@ -1216,7 +1239,7 @@ public async Task VariantsInvalidScenarios()
// Verify that an exception is thrown for invalid doubles From and To in the Percentile section
e = await Assert.ThrowsAsync<FeatureManagementException>(async () =>
{
variant = await featureManager.GetVariantAsync("VariantFeatureInvalidFromTo", cancellationToken);
variant = await featureManager.GetVariantAsync(Features.VariantFeatureInvalidFromTo, cancellationToken);
});

Assert.Equal(FeatureManagementError.InvalidConfigurationSetting, e.Error);
Expand Down
Loading