support feature variant #13

Eskibear · 2024-07-30T08:56:15Z

No description provided.

zhiyuanliang-ms · 2024-08-05T08:20:28Z

.NET FM has TargetingEvaluationOptions which allows user to ignore case while targeting (both targeting filter and varaint allocation). This is missing in JS FM.

Python FM allows to ignore case while using targeting filter. here But it doesn't support it for variant allocation.

src/common/targetingEvaluator.ts

Eskibear · 2024-08-05T08:38:18Z

.NET FM has TargetingEvaluationOptions which allows user to ignore case while targeting (both targeting filter and varaint allocation). This is missing in JS FM.

Python FM allows to ignore case while using targeting filter. here But it doesn't support it for variant allocation.

I personally prefer not to support this option ignoreCase now until there's valid customer requests coming for simplicity. We'll see issues if it does block customers from conveniently using it. It's relatively easy to add the support if later we decide to do that, and won't breaking anything.

zhiyuanliang-ms · 2024-08-05T08:52:21Z

src/featureManager.ts

+                if (isTargetedUser(context.userId, userAllocation.users)) {
+                    const variant = featureFlag.variants?.find(v => v.name == userAllocation.variant);
+                    if (variant !== undefined) {
+                        return { variant, reason: VariantAssignmentReason.User };


Should it be { variant: variant, reason: VariantAssignmentReason.User }?

{variant} is short for {variant: variant}. A syntax sugar for simplicity, I can make it clearer if it looks confusing to you now.

I am ok with it. But personally, I prefer consistency. Is there linting rule for this?

Of course there is corresponding linting rule, it's called object-shorthand.
This shorthand syntax makes the code more readable and is widely adopted in ES6 and later versions of JavaScript. See the example of this rule, usually we force to use object shorthand in linting rules instead of forbidding.

src/featureManager.ts

src/variant/IVariantFeatureManager.ts

src/common/targetingEvaluator.ts

src/featureManager.ts

zhiyuanliang-ms · 2024-08-14T03:12:36Z

src/IFeatureManager.ts

+
+export interface IFeatureManager {
+    /**
+     * Get the list of feature names.


Somewhere you used the third person singular, while in others, you did not.

I use Get... instead of `Gets..." here. But for the properties of options, I use "It gets/specifies ..." if starting with an "It".

Those copied from dotnet are using third person singular for verbs I guess. ; ) Yes, they should be updated to be consistent.

src/common/targetingEvaluator.ts

zhiyuanliang-ms · 2024-08-14T08:42:23Z

src/featureManager.ts

+                    // no default specified
+                    variantDef = undefined;
+                    reason = VariantAssignmentReason.DefaultWhenDisabled;
+                }


reason = VariantAssignmentReason.DefaultWhenDisabled; should be put here outside the if-else

I decide to keep it in the if-else block together with variantDef assignment for better clarity. ;)

src/featureManager.ts

zhiyuanliang-ms · 2024-08-14T08:53:10Z

@Eskibear Except for the NIT in comments and the logic of variant assignment reason, this PR looks good to me.

rossgrambo · 2024-08-14T22:43:57Z

src/IFeatureManager.ts

+     * @param featureName name of the feature.
+     * @param context a targeting context object used to evaluate which variant the user will be assigned.
+     */
+    getVariant(featureName: string, context: ITargetingContext): Promise<Variant | undefined>;


We're actively debating this now- but I think context here should be unknown. https://github.com/microsoft/FeatureManagement-Dotnet/pull/484/files#r1717629098

Given the possibility that we calculate audience context id based on any other property rather than userid, I agree that the context is unknown (or to limit it to an object?). A good thing is, here all properties in ITargetingContext are optional, meaning it’s compatible with an arbitrary object type.

rossgrambo · 2024-08-14T22:47:09Z

src/common/targetingEvaluator.ts

+import { createHash } from "crypto";
+
+/**
+ * Determines if the user is part of the audience, based on the user id and the percentage range.


Suggested change

* Determines if the user is part of the audience, based on the user id and the percentage range.

* Determines if the user is part of the audience, based on the user id and the percentile range.

I think percentile is slightly more explicit here. I'd also use it in the from and to param comments.

@zhiyuanliang-ms please help to follow up with the wording in comments, thanks!

zhiyuanliang-ms · 2024-08-17T15:03:28Z

src/featureManager.ts

+    Percentile
+}
+
+class EvaluationResult {


Should this also be exported, since developers will need it to send telemetry.

Eskibear added 3 commits July 30, 2024 16:55

support feature variant

504b6e0

add test cases

e51b959

refactor isEnabled to support status_override

3ed3d75

Eskibear marked this pull request as ready for review August 2, 2024 08:03

Eskibear requested review from mrm9084, avanigupta, juniwang and zhiyuanliang-ms August 2, 2024 08:04

zhiyuanliang-ms requested review from amerjusupovic and rossgrambo August 5, 2024 03:06

zhiyuanliang-ms reviewed Aug 5, 2024

View reviewed changes

src/common/targetingEvaluator.ts Outdated Show resolved Hide resolved

zhiyuanliang-ms reviewed Aug 5, 2024

View reviewed changes

src/featureManager.ts Outdated Show resolved Hide resolved

rossgrambo reviewed Aug 5, 2024

View reviewed changes

src/variant/IVariantFeatureManager.ts Outdated Show resolved Hide resolved

rossgrambo reviewed Aug 5, 2024

View reviewed changes

src/common/targetingEvaluator.ts Outdated Show resolved Hide resolved

rossgrambo reviewed Aug 5, 2024

View reviewed changes

src/featureManager.ts Show resolved Hide resolved

rossgrambo reviewed Aug 5, 2024

View reviewed changes

src/featureManager.ts Show resolved Hide resolved

Eskibear added 4 commits August 6, 2024 10:35

add missing return statement

71fe5f3

address comments: simplify code

bf5b0e1

fix range validation: align with .NET

a7f0472

use a single interface for the public APIs

ca66f77

rossgrambo reviewed Aug 7, 2024

View reviewed changes

src/featureManager.ts Outdated Show resolved Hide resolved

extract common code as getVariantAssignment

63cdd43

zhiyuanliang-ms reviewed Aug 14, 2024

View reviewed changes

src/common/targetingEvaluator.ts Outdated Show resolved Hide resolved

zhiyuanliang-ms reviewed Aug 14, 2024

View reviewed changes

src/featureManager.ts Show resolved Hide resolved

zhiyuanliang-ms approved these changes Aug 14, 2024

View reviewed changes

address comments

422b562

Eskibear merged commit d8e7d54 into preview Aug 15, 2024
1 check passed

Eskibear deleted the yanzh/variant branch August 15, 2024 03:03

rossgrambo reviewed Aug 15, 2024

View reviewed changes

Eskibear mentioned this pull request Aug 16, 2024

getVariant/TargetingFilter might accept any context type #20

Open

zhiyuanliang-ms reviewed Aug 17, 2024

View reviewed changes

	* Determines if the user is part of the audience, based on the user id and the percentage range.
	* Determines if the user is part of the audience, based on the user id and the percentile range.

support feature variant #13

support feature variant #13

Uh oh!

Conversation

Eskibear commented Jul 30, 2024

Uh oh!

zhiyuanliang-ms commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Eskibear commented Aug 5, 2024

Uh oh!

zhiyuanliang-ms Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zhiyuanliang-ms commented Aug 14, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zhiyuanliang-ms commented Aug 5, 2024 •

edited

Loading

zhiyuanliang-ms Aug 5, 2024 •

edited

Loading