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

Add variants for feature flags #250

Merged
merged 85 commits into from
Sep 13, 2023
Merged

Conversation

amerjusupovic
Copy link
Contributor

No description provided.

…ces to keep existing usage possible. (#133)" (#138)"

This reverts commit 8f9a7e4.
{
throw new FeatureManagementException(
FeatureManagementError.InvalidConfigurationSetting,
string.Format(parseEnumErrorString, nameof(FeatureStatus), rawFeatureStatus, configurationSection.Key));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
string.Format(parseEnumErrorString, nameof(FeatureStatus), rawFeatureStatus, configurationSection.Key));
string.Format(parseEnumErrorString, ConfigurationFields.FeatureStatus, rawFeatureStatus, configurationSection.Key));

Name = section[ConfigurationFields.NameKeyword],
ConfigurationValue = section.GetSection(ConfigurationFields.VariantDefinitionConfigurationValue),
ConfigurationReference = section[ConfigurationFields.VariantDefinitionConfigurationReference],
StatusOverride = section.GetValue<StatusOverride>(ConfigurationFields.VariantDefinitionStatusOverride)
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't this enum handled like others?

Copy link
Contributor Author

@amerjusupovic amerjusupovic Sep 6, 2023

Choose a reason for hiding this comment

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

My bad, this should definitely be checked like the previous enums. I'll add a test for invalid StatusOverride value.

@jimmyca15
Copy link
Member

Wondering if we should have a preview branch that we merge this into.

@rossgrambo
Copy link
Contributor

I think a preview branch is a good idea. We'll want to merge telemetry and variants somewhere outside of main.

@amerjusupovic amerjusupovic changed the base branch from main to preview September 7, 2023 22:34
@amerjusupovic
Copy link
Contributor Author

Created a new preview branch from main.

private readonly IConfiguration _configuration;
private readonly ConcurrentDictionary<string, FeatureDefinition> _definitions;
private IDisposable _changeSubscription;
private int _stale = 0;

const string ParseValueErrorString = "Invalid {0} with value '{1}' for feature '{2}'.";
Copy link
Member

Choose a reason for hiding this comment

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

How come {0} isn't in single quotes ? And shouldn't it be "Invalid settting '{0}' with value..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just based the format string off of the old error message, but since we're using a variable now I understand adding the single quotes.


if (from > to)
{
throw new ArgumentException($"Double {nameof(from)} cannot be larger than double {nameof(to)}.");
Copy link
Member

Choose a reason for hiding this comment

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

Both usage of "double" should be replaced with "value of"

{
if (string.IsNullOrEmpty(percentile.Variant))
{
_logger.LogWarning($"Missing variant name for {nameof(featureDefinition.Allocation.Percentile)} allocation in feature {featureDefinition.Name}");
Copy link
Member

@jimmyca15 jimmyca15 Sep 8, 2023

Choose a reason for hiding this comment

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

The way this is used it seems like it should just be hardcoded literal "percentile"

percentile.From,
percentile.To,
_assignerOptions.IgnoreCase,
featureDefinition.Allocation.Seed))
Copy link
Contributor

Choose a reason for hiding this comment

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

If seed is null, we should use feature name.

Copy link
Contributor

Choose a reason for hiding this comment

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

This would ensure that flags have different seeds by default. This ensures whoever gets mapped to 1% isn't thrown into every experiment always.

Copy link
Contributor

Choose a reason for hiding this comment

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

Although, there is an argument that Allocation might not want to default to the same % mapping that targeting uses. Because a targeting filter of 50% with an allocation of 50% means 100% of the 50% would be allocated.

So perhaps we should default to feature name, but add "allocation" at the end or something. @jimmyca15 what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

@rossgrambo makes sense. Although I would put any addition before the flag name since it's unchanging and acts like prefix in my eyes.

Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>
@amerjusupovic amerjusupovic merged commit b1b1562 into preview Sep 13, 2023
2 checks passed
if (string.Equals(featureFilterConfiguration.Name, "AlwaysOn", StringComparison.OrdinalIgnoreCase))
// Handle AlwaysOn and On filters
if (string.Equals(featureFilterConfiguration.Name, "AlwaysOn", StringComparison.OrdinalIgnoreCase) ||
string.Equals(featureFilterConfiguration.Name, "On", StringComparison.OrdinalIgnoreCase))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we mention that the filter name "On" is also occupied and it is a built-in filter in the document.

rossgrambo added a commit that referenced this pull request Sep 12, 2024
* Add variants for feature flags (#250)

* in progress, add new classes for variants and define methods in featuremanager

* Revert "Revert "Add cancellation token parameter to async feature management interfaces. (#131)" (#139)"

This reverts commit e531863.

* Revert "Revert "Added default value for cancellation token in interfaces to keep existing usage possible. (#133)" (#138)"

This reverts commit 8f9a7e4.

* fix any conflicts left from adding cancellationToken back

* add in progress changes to allocation and featuredefinitionprovider

* add examples for testing

* fix adding new featuredefinition properties from featuremanagement definition

* progress adding getvariant logic classes

* continued

* remove repeated code in contextual targeting

* fix version of contextual filter

* more progress on getting the contextual allocator to work

* about to test getvariant

* add example to test

* add snapshot changes

* variant can be detected and retrieved from getvariantasync

* progress on allocation logic, add comments where consideration needed

* add use of optionsresolver for reference, todo work on isenabledasync between customer use and variant use

* All working except couple TODOs, need to add unit tests

* remove some comments, add null check where needed

* update todo comments

* fix line eols

* add unit test, in progress

* TODOs in progress, need to restructure featurevariantassigner design

* fix seed logic

* update comments, status logic

* remove unnecessary files for custom assigners, fix featuremanager methods and interfaces to match

* fix naming from allocator to assigner for classes and files

* cleanup extra methods, todo config section logic

* in progress adding configurationsection returned when using configurationvalue

* continuation of last commit

* working return for configvalue

* move logic to featuremanager for assigning

* remove unused assigner classes

* add new configurationsection to handle return for variant

* null error, in progress new configurationsection class

* fix old bug

* progress on unit tests

* more null check changes, test fixes

* reset examples changes

* Revert "Revert "Revert "Added default value for cancellation token in interfaces to keep existing usage possible. (#133)" (#138)""

This reverts commit d087e7b.

* Revert "Revert "Revert "Add cancellation token parameter to async feature management interfaces. (#131)" (#139)""

This reverts commit c1451d3.

* add comments for new classes

* fix comments for public classes again

* update comments, default values

* fix variantconfigurationsection, comments in definitionprovider

* fix using statements, null checks

* fix unit test failures with servicecollectionextensions

* add revisions: fix namepaces, add exceptions tests, combine percentage logic, fix comments, add cancellationtoken to new interface

* change context accessor logic

* fix comments for default variants

* PR revisions

* change class names, PR fixes

* fix edge case percentage targeting

* rename allocation classes, remove exceptions and add warning logs, prioritize inline value for variant config, more revisions

* refactor isenabled to remove boolean param

* change configurationvalue to IConfigurationSection instead of string

* fix enabledwithvariants logic

* PR revisions, fix logic in new methods from last commit

* set session managers last in flow

* make false explicit for status disabled or missing definition

* fix constructor default params, move session managers logic, pr revisions

* fix comment

* fix resolvedefaultvariant, isexternalinit error

* add back 3.1

* Apply suggestions from code review

Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>

* isexternalinit comments, remove resolvedefault helper

* remove binding, fix featuredefinitionprovider issues

* change to Debug.Assert from Assert

* update method name

* remove parseenum, add ConfigurationFields class

* test failing, fixed PR revisions

* fix invalid scenarios test

* simplify context in test

* remove unused using

* remove unused param

* Clarify how From and To bounds work in PercentileAllocation

Co-authored-by: Ross Grambo <rossgrambo@microsoft.com>

* fix error messages

* add feature name as default seed with allocation prefix

* Update src/Microsoft.FeatureManagement/FeatureManager.cs

Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>

---------

Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>
Co-authored-by: Ross Grambo <rossgrambo@microsoft.com>

* Adds GetFeatureNamesAsync to IVariantFeatureManager and uses the interface in FeatureManagerSnapshot (#270)

* Adds telemetry pipeline (#259)

* Adds telemetry pipeline. Adds evaluationevent, publisher interface, and a publisher instance for app insights.

* Use await insead of .result

* Reverting unused dependency removal

* Update src/Microsoft.FeatureManagement/FeatureDefinition.cs

Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>

* Update src/Microsoft.FeatureManagement/FeatureDefinition.cs

Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>

* Update src/Microsoft.FeatureManagement.AppInsightsTelemetryPublisher/TelemetryPublisherAppInsights.cs

Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>

* Removed factory pattern for publisher DI

* Update src/Microsoft.FeatureManagement/FeatureManager.cs

Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>

* Resolve misc. comments

* Adjusts descriptions for FeatureDefinition properties

* Rename AppInsights to ApplicationInsights

* Resolves project reference in sln

* Adjusts ServiceCollectionExtensions to use TryAddSingleton

* Removes langversion from .csproj

* Temp

* Adds explicit null checkts before publishing fields to app insights

* Addressing misc. comments

* Removing project reference for example project

* Syncing telemetry and variants

* Adds null check for feature definitions

* Adjusted TelemetryPublishers to no longer be inserted into DI

* Resolving comments

* Removing Application Insights project

* Converts some variables to inline

* Resolving misc. comments

* Moves AddTelemetryPublisher to be an extension method

* Removing IVariantFeatureManager GetFeatureNamesAsync definition, as it should it it's own PR

* Remove unused dependency

* Resolving comments

* Remove invisible character changes

* Remove Dependency Injection package

* Update src/Microsoft.FeatureManagement/FeatureManagerSnapshot.cs

* Update src/Microsoft.FeatureManagement/IVariantFeatureManager.cs

* Persists cancellation token to PublishEvent and adds null check for null publisher collection

* Adds TargetingContext to the EvaluationResult

* Resolving comments

* Removes TargetingContext from EvaluationEvent for now

* Moves tags, etag, and label under 'TelemetryMetadata'

* Adjusts telemetry metadata to already be a flattened dictionary

* Removes bind in favor of ToDictionary. Removes unused using

---------

Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>

* Application Insights Publisher (#281)

* Adds Application Insights publisher

* Change readonly to const

* Update src/Microsoft.FeatureManagement.Telemetry.ApplicationInsights/ApplicationInsightsTelemetryPublisher.cs

Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>

* Update src/Microsoft.FeatureManagement.Telemetry.ApplicationInsights/ApplicationInsightsTelemetryPublisher.cs

Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>

---------

Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>

* Adds example library for Application Insights

* Adds readme to example

* Updates readme and persists telemetry metadata by placing it in the correct field

* Update README file for variants (#264)

* update readme with variants, first draft

* small summary revisions

* fix wording of summary

* fix code description

* Apply suggestions from code review

Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>

* revisions

* PR revisions, describe Allocation properties, fix descriptions and titles

* clarify configurationvalue possible values

* clarify seed description

* remove all mentions of On filter

* fix example for override, clarify allowing no configurationvalue or reference

* change statusoverride example

* Apply suggestions from code review

Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>

* revisions to summaries, fix allocation table

---------

Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>

* Edits to README for variants  (#289)

* add edits for variants

* fix wording

* Simplified javascript and other slight modifications

* Adjusts IVarantManager to use ValueTask (#293)

* remove conflict mark

* add missing comment & [Fact] attribute

* Updates.

* Updates.

* Improvements.

* Add missing project in sln file.

* Fix various solution issues
* missing project inclusion in solution file
* Project declared nullable support but wasn't written with it in mind
* pack.ps1 script didn't reference newest project.

* Remove nullable usage. Other updates.

* remove & add empty line

* resolve conflicts

* Fix project added twice.

* use const string and feature flag referece

* Add Telemetry section

* Made section casing consistent.

* Add section for enabling telemetry on flags.

* Update TOC.

* Use relative links.

* should -> must

* Updates based on feedback. Reordered telemetry sections.

* Add note.

* Update README.md

* Update telemetry schema in feature flags.

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

* Adds Reason field and adjusts evaluation logic

* use enum AssignmentReason

* add more testcases & remove some interal methods

* fix typo

* update comments

* resolve comments

* update comment for DisabledDefault

* fix typo

* update

* update telemetry publisher

* add reason when no allocation section

* update

* remove nullable

* resolve comments

* update comments

---------

Co-authored-by: zhiyuanliang <zhiyuanliang@microsoft.com>

* Undo whitespace changes from  merge.

* Readme clarifications. Added argument exception if feature definition telemetry is null when passed into app insights telemetry publisher.

* Update property summary.

* Prepare packages for 4.0.0-preview release.

* Add preview label. Update telemetry package description.

* Bumps Application Insights version (#342)

* Updates example application insights version (#344)

* Mention TargetingContext and fix wording in Variants section of readme (#330)

* mention itargetingcontextaccessor for variant allocation

* update changes

* add reference to ITargetingContextAccessor to readme for allocation

* update with new discussed changes, rename sections and introduce context sooner

* fix wording of default

* fix wording to avoid implying variants are assigned per feature

* use the term assign when referring to variants returned for a user

* use assign term in more places

* small changes

* Update README.md

Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>

* update section titles

* resolve comments

* fix typo

* update link to allocating variants section

---------

Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>

* Adds TargetingContext to EvaluationEvent & Emits TargetingId in the App Insights Publisher (#347)

* Adds TargetingId to EvaluationEvent

* Adjusts EvaluationEvent to use TargetingContext instead of ITargetingContext

* move public method to the front of private method (#349)

* Adds extension methods for TrackEvent and TrackMetric (#348)

* Adds extension methods for TrackEvent and TrackMetric

* Adds copyright, crefs, and uses []= instead of .Add for properties

* Update src/Microsoft.FeatureManagement.Telemetry.ApplicationInsights/ApplicationInsightsTelemetryExtensions.cs

Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>

* Added null check, moved shared logic to helper functions

* Removed TrackMetric extension for now

---------

Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>

* Adds missing cref (#352)

* Adjusts private methods from Task to ValueTask (#353)

* Adds targeting middleware and targeting initializer (#350)

* Adds targeting middleware and targeting initializer

* Added logger and added prefix to targetingId

* Apply suggestions from code review

Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>

* Adjustments from comments

---------

Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>

* unify the parse methods

* update

* Adjusts example to use middleware and initializer for targeting id (#357)

* Adjusts example to use middleware and initializer for targeting id

* Update examples/EvaluationDataToApplicationInsights/README.md

* remove unused package

* Feature-based Injection (#335)

* init

* draft

* use ValueTask

* support factory method

* add example

* update

* Update

* Update

* update example

* match variant name or configuration value

* update to the latest design

* merge with preview branch

* resolve comments

* rename to VariantService

* update & add comments

* remove POC example

* add testcases & use method name GetServiceAsync

* update comments

* add variant service cache

* resolve comments

* throw exception for duplicated registration

* add testcase

* remove unused package

* update comment

* set feature name in constructor

* adjust-tab-size (#366)

* Merge main to preview (#373)

* Add a constructor without FeatureManagementOption parameter for FeatureManager (#363)

* add new constructor

* update

* Target on .NET 8.0 (#365)

* target on .NET 8.0

* remove file

* add net8.0 for Microsoft.FeatureManagement.AspNetCore

* update package version

* update

* Add feature management schema for main branch (#362)

* schema file added

* README update

* update

* update

* update README & remove the Microsoft schema file

* update readme

* update

* Updates test schema and adjusts title (#372)

* Support Microsoft Feature Management schema for main branch (#370)

* use snake case

* do not support root fall back for MS schema & remove EnsureInit

* update

* rename variable

* update .NET FM schema

* re-add schema link

* add whitespace (#374)

* update

---------

Co-authored-by: Ross Grambo <rossgrambo@microsoft.com>

* add missing param comment (#380)

* Version bump

* Adds .Telemetry.ApplicationInsights.AspNetCore to build packages

* Support Microsoft Feature Management schema for preview branch (#375)

* support microsoft feature flag schema v2

* add more testcases

* add empty lines

* add schema & update README

* update schema

* update (#398)

* Optional Cancellation Token (#395)

* make cancelltation optional for feature manager & add cancellation token for session manager

* revert breaking change in ISessionManager

* remove unused package

* Update README to mention the usage of feature-based injection (variant service) (#388)

* update README

* update

* update

* update

* update

* fix typo

* update

* update

* Avoid redundant validation for cached TargetingFilterSettings (#387)

* do validation during binding parameters

* adjust method order

* Update target framework for telemetry packages (#413)

* update target framework

* update language version

* Adjusts namespace to simply ~.Telemetry

* Adjusts ApplicationInsights extension methods namespace

* Adjusts class name of TelemetryClientExceptions

* Removed changes to FM.Telemetry.ApplicationInsights.AspNetCore namespace

* Ensure the consistency of targeting evaluation across CPU architectures. (#405)

* check system endianness

* remove period

* update reverse method

* update comment

* adjust comment

* version bump (#420)

* Move warning deeper into variant code

* Adjusted warning message

* Boolean zen

* Updating from comments

* Adjusted wording

* Formatting update

* Promote Microsoft FM schema in README (#414)

* replace examples using Microsoft schema

* update

* correct section name

* update

* update schema reference link

* Remove variant & telemetry support for .NET Feature Management schema (#421)

* reorg test suite

* remove variant and telemetry & reorg test suite

* update

* update constructor

* update appsettings.json of EvaluationDataToApplicationInsights example project

* update appsetting.json for all examples

* add dependency for telemetry aspnetcore package (#439)

* Support variant for FeatureTagHelper (#407)

* add variant for feature tag helper

* update

* Negate also applies for variant

* correct comment

* update comment

* update README

* update README

* throw exception for Requirement All

* update readme

* update

* Moves telemetry publishing to Activity

* Update src/Microsoft.FeatureManagement/FeatureManager.cs

Co-authored-by: Juliano Leal Goncalves <julealgon@gmail.com>

* Update src/Microsoft.FeatureManagement/FeatureManager.cs

Co-authored-by: Juliano Leal Goncalves <julealgon@gmail.com>

* Resolving comments

* avoid null reference (#457)

* Some formatting updates

* Adjusted exception & exception message

* Update src/Microsoft.FeatureManagement/FeatureManager.cs

Co-authored-by: Juliano Leal Goncalves <julealgon@gmail.com>

* Adjusts hosted service to use the ServiceProvider instead of manually creating the publisher

* Update src/Microsoft.FeatureManagement/FeatureManager.cs

Co-authored-by: Zhiyuan Liang <141655842+zhiyuanliang-ms@users.noreply.github.com>

* Resolving comments

* Moved conditional checks

* Switched to standard null checks

* Adjusted to use activity parent instead of thread check and adjusted null or empty event field check

* Updated string null check

* Updates activity event to be PascalCase

* update example name

* Update Variant Example Application (#437)

* add example for variant based injection

* readme added

* not use third-party api

* update

* update README

* fix typo

* use latest api

* Switches to Activity for storing TargetingId and removes Microsoft.FeatureManagement.Telemetry.AspNetCore project

* Removes Microsoft.FeatureManagement.Telemetry.ApplicationInsights.AspNetCore

* Adds null check in middleware

* Added debug logs for unexpected nulls

* Respect .NET and Microsoft feature management schemas in configuration (#470)

* respect all feature management schemas

* add testcase

* add testcase

* correct test case

* simplify methods

* remove unused package

* unify newline usage

* remove useless code & rename variables

* reuse ParseEnum

* rename variable

* Switching log from debug to warning (#471)

* Switching log from debug to warning

* Adjusted warning logs

* adjust spacing

* version bump 4.0.0-preview4 (#476)

* fix typo in comment (#480)

* Adjusts builder extension methods to be friendlier (#487)

* Adds and adjusts builder extension methods

* Removed UseFeatureManagement and adjusted name of AddAppInsightsTelemetryPublisher

* Adjusts examples

* Use ITargetingContext when calling GetVariantAsync (#484)

* add variant feature manager extensions

* use TContext to avoid future breaking change

* TargetingContext in EvaluationEvent

* update comments & not use ambient context in contextual case

* update comment

* use ITargetingContext & check runtime context type

* use TargetingCotnext for private method

* use var instead of type

* revert change on ContextualTestFilter

* update comment

* update comment

* remove configuration reference from variant feature flag (#488)

* Renames and updates EvaluationDataToAppInsights example (#490)

* Renames project and cleans up startup code + auth

* Update examples/VariantAndTelemetryDemo/Program.cs

Co-authored-by: Zhiyuan Liang <141655842+zhiyuanliang-ms@users.noreply.github.com>

---------

Co-authored-by: Zhiyuan Liang <141655842+zhiyuanliang-ms@users.noreply.github.com>

* For examples and tests- fixes warnings, updates packages, and updates… (#491)

* Renames project and cleans up startup code + auth

* For examples and tests- fixes warnings, updates packages, and updates .net versions

* Switched to var

* Reverted construtor away from primary constructor style

* Missing semicolon

* use TimeProvider (#452) (#492)

Co-authored-by: Zhiyuan Liang <141655842+zhiyuanliang-ms@users.noreply.github.com>

* Removes preview (#493)

* Removes unused using statements and cleans up spacing. (#496)

---------

Co-authored-by: Amer Jusupovic <32405726+amerjusupovic@users.noreply.github.com>
Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>
Co-authored-by: zhiyuanliang <zhiyuanliang@microsoft.com>
Co-authored-by: Zhiyuan Liang <141655842+zhiyuanliang-ms@users.noreply.github.com>
Co-authored-by: Juliano Leal Goncalves <julealgon@gmail.com>
Co-authored-by: AMER JUSUPOVIC <ajusupovic@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants