-
Notifications
You must be signed in to change notification settings - Fork 7
Telemetry Support #24
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
Changes from 1 commit
e3fd558
a9847cc
de6a442
089268f
2dbecec
43e2c04
4219544
86a50e1
545175a
eb1c8c2
61d0022
b1a2b25
8025453
06f9536
40a1275
3ec867b
7a41fa3
f839860
48a4c22
4cd65c3
0fb2000
851e6bd
3ff66b3
7297743
7cb8935
c5dd07d
765f38e
b086f3b
ca41bd5
0d76953
cead033
ade2027
96b96b2
996e730
cfcc158
f6dddbd
b42b9c6
822e4cd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -93,38 +93,42 @@ def __init__(self, configuration, **kwargs): | |
self._filters[feature_filter.name] = feature_filter | ||
|
||
@staticmethod | ||
def _check_default_disabled_variant(feature_flag, evaluation_event): | ||
def _check_default_disabled_variant(evaluation_event): | ||
""" | ||
A method called when the feature flag is disabled, to determine what the default variant should be. If there is | ||
no allocation, then None is set as the value of the variant in the EvaluationEvent. | ||
|
||
:param FeatureFlag feature_flag: Feature flag object. | ||
:param EvaluationEvent evaluation_event: Evaluation event object. | ||
""" | ||
if not feature_flag.allocation: | ||
evaluation_event.reason = VariantAssignmentReason.DEFAULT_WHEN_DISABLED | ||
if not evaluation_event.feature.allocation: | ||
evaluation_event.enabled = False | ||
return | ||
FeatureManager._check_variant_override( | ||
feature_flag.variants, feature_flag.allocation.default_when_disabled, False, evaluation_event | ||
evaluation_event.feature.variants, | ||
evaluation_event.feature.allocation.default_when_disabled, | ||
False, | ||
evaluation_event, | ||
) | ||
evaluation_event.reason = VariantAssignmentReason.DEFAULT_WHEN_DISABLED | ||
|
||
@staticmethod | ||
def _check_default_enabled_variant(feature_flag, evaluation_event): | ||
def _check_default_enabled_variant(evaluation_event): | ||
""" | ||
A method called when the feature flag is enabled, to determine what the default variant should be. If there is | ||
no allocation, then None is set as the value of the variant in the EvaluationEvent. | ||
|
||
:param FeatureFlag feature_flag: Feature flag object. | ||
:param EvaluationEvent evaluation_event: Evaluation event object. | ||
""" | ||
if not feature_flag.allocation: | ||
evaluation_event.reason = VariantAssignmentReason.DEFAULT_WHEN_ENABLED | ||
if not evaluation_event.feature.allocation: | ||
evaluation_event.enabled = True | ||
mrm9084 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
return | ||
FeatureManager._check_variant_override( | ||
feature_flag.variants, feature_flag.allocation.default_when_enabled, True, evaluation_event | ||
evaluation_event.feature.variants, | ||
evaluation_event.feature.allocation.default_when_enabled, | ||
True, | ||
evaluation_event, | ||
zhiyuanliang-ms marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) | ||
evaluation_event.reason = VariantAssignmentReason.DEFAULT_WHEN_ENABLED | ||
|
||
@staticmethod | ||
def _check_variant_override(variants, default_variant_name, status, evaluation_event): | ||
|
@@ -156,33 +160,32 @@ def _is_targeted(context_id): | |
|
||
return (context_marker / (2**32 - 1)) * 100 | ||
|
||
def _assign_variant(self, feature_flag, targeting_context, evaluation_event): | ||
def _assign_variant(self, targeting_context, evaluation_event): | ||
""" | ||
Assign a variant to the user based on the allocation. | ||
|
||
:param FeatureFlag feature_flag: Feature flag object. | ||
:param TargetingContext targeting_context: Targeting context. | ||
:param EvaluationEvent evaluation_event: Evaluation event object. | ||
:return: Variant name. | ||
""" | ||
evaluation_event.feature = feature_flag | ||
if not feature_flag.variants or not feature_flag.allocation: | ||
feature = evaluation_event.feature | ||
if not feature.variants or not feature.allocation: | ||
return None | ||
if feature_flag.allocation.user and targeting_context.user_id: | ||
for user_allocation in feature_flag.allocation.user: | ||
if feature.allocation.user and targeting_context.user_id: | ||
for user_allocation in feature.allocation.user: | ||
if targeting_context.user_id in user_allocation.users: | ||
evaluation_event.reason = VariantAssignmentReason.USER | ||
return user_allocation.variant | ||
if feature_flag.allocation.group and len(targeting_context.groups) > 0: | ||
for group_allocation in feature_flag.allocation.group: | ||
if feature.allocation.group and len(targeting_context.groups) > 0: | ||
for group_allocation in feature.allocation.group: | ||
for group in targeting_context.groups: | ||
if group in group_allocation.groups: | ||
evaluation_event.reason = VariantAssignmentReason.GROUP | ||
return group_allocation.variant | ||
if feature_flag.allocation.percentile: | ||
context_id = targeting_context.user_id + "\n" + feature_flag.allocation.seed | ||
if feature.allocation.percentile: | ||
context_id = targeting_context.user_id + "\n" + feature.allocation.seed | ||
box = self._is_targeted(context_id) | ||
for percentile_allocation in feature_flag.allocation.percentile: | ||
for percentile_allocation in feature.allocation.percentile: | ||
if box == 100 and percentile_allocation.percentile_to == 100: | ||
return percentile_allocation.variant | ||
if percentile_allocation.percentile_from <= box < percentile_allocation.percentile_to: | ||
|
@@ -277,7 +280,8 @@ def get_variant(self, feature_flag_id, *args, **kwargs): | |
self._on_feature_evaluated(result) | ||
return result.variant | ||
|
||
def _check_feature_filters(self, feature_flag, evaluation_event, targeting_context, **kwargs): | ||
def _check_feature_filters(self, evaluation_event, targeting_context, **kwargs): | ||
feature_flag = evaluation_event.feature | ||
feature_conditions = feature_flag.conditions | ||
feature_filters = feature_conditions.client_filters | ||
|
||
|
@@ -303,10 +307,11 @@ def _check_feature_filters(self, feature_flag, evaluation_event, targeting_conte | |
evaluation_event.enabled = True | ||
break | ||
|
||
def _assign_allocation(self, feature_flag, evaluation_event, targeting_context): | ||
def _assign_allocation(self, evaluation_event, targeting_context): | ||
feature_flag = evaluation_event.feature | ||
if feature_flag.allocation and feature_flag.variants: | ||
default_enabled = evaluation_event.enabled | ||
variant_name = self._assign_variant(feature_flag, targeting_context, evaluation_event) | ||
variant_name = self._assign_variant(targeting_context, evaluation_event) | ||
mrm9084 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
evaluation_event.enabled = default_enabled | ||
if variant_name: | ||
FeatureManager._check_variant_override( | ||
|
@@ -318,11 +323,11 @@ def _assign_allocation(self, feature_flag, evaluation_event, targeting_context): | |
|
||
variant_name = None | ||
if evaluation_event.enabled: | ||
FeatureManager._check_default_enabled_variant(feature_flag, evaluation_event) | ||
FeatureManager._check_default_enabled_variant(evaluation_event) | ||
if feature_flag.allocation: | ||
variant_name = feature_flag.allocation.default_when_enabled | ||
else: | ||
FeatureManager._check_default_disabled_variant(feature_flag, evaluation_event) | ||
FeatureManager._check_default_disabled_variant(evaluation_event) | ||
if feature_flag.allocation: | ||
variant_name = feature_flag.allocation.default_when_disabled | ||
evaluation_event.variant = self._variant_name_to_variant(feature_flag, variant_name) | ||
|
@@ -336,7 +341,6 @@ def _check_feature(self, feature_flag_id, targeting_context, **kwargs): | |
:return: True if the feature flag is enabled for the given context. | ||
:rtype: bool | ||
""" | ||
evaluation_event = EvaluationEvent(enabled=False) | ||
if self._copy is not self._configuration.get(FEATURE_MANAGEMENT_KEY): | ||
self._cache = {} | ||
self._copy = self._configuration.get(FEATURE_MANAGEMENT_KEY) | ||
|
@@ -347,23 +351,24 @@ def _check_feature(self, feature_flag_id, targeting_context, **kwargs): | |
else: | ||
feature_flag = self._cache.get(feature_flag_id) | ||
|
||
evaluation_event = EvaluationEvent(feature_flag) | ||
if not feature_flag: | ||
logging.warning("Feature flag %s not found", feature_flag_id) | ||
# Unknown feature flags are disabled by default | ||
return evaluation_event | ||
|
||
if not feature_flag.enabled: | ||
# Feature flags that are disabled are always disabled | ||
FeatureManager._check_default_disabled_variant(feature_flag, evaluation_event) | ||
FeatureManager._check_default_disabled_variant(evaluation_event) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. check_variant_override is called in _check_default_disabled_variant. However, status_override should not work when feature_flag.enabled == false. Current behavior is wrong. Currently, you have I suggest we have the code path as below def _check_feature():
feature_flag = _get_feature_flag()
...
evaluation_event = EvaluationEvent(feature_flag)
_check_feature_filters(evaluation_event)
_assign_allocation(evaluation_event)
if feature_flag.enabled:
_check_variant_override()
I am ok with the current change on the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rossgrambo, @zhiyuanliang-ms, doesn't the fact that this override doesn't work mean that it is more complex as in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But according to the flowcharts it should be disabled if the flag is disabled... So actually you're right. I misread the code. We have a check at the end to see if the flag is not disabled before overriding: https://github.com/microsoft/FeatureManagement-Dotnet/blob/preview/src/Microsoft.FeatureManagement/FeatureManager.cs#L354 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes but it's not complex if you just do both everytime. Dotnet just always determines both the IsEnabled result and the Variant result (which will be defaultWhenDisabled in the disabled case). Then if IsEnabled was the calling function- just returns the bool, and if GetVariant was the calling function, returns the variant. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code works the same as DotNet, which is why is does the slightly more complex operation, but it does simplify the whole logic as variants and is_enabled mostly work the same. |
||
if feature_flag.allocation: | ||
variant_name = feature_flag.allocation.default_when_disabled | ||
evaluation_event.variant = self._variant_name_to_variant(feature_flag, variant_name) | ||
evaluation_event.feature = feature_flag | ||
return evaluation_event | ||
|
||
self._check_feature_filters(feature_flag, evaluation_event, targeting_context, **kwargs) | ||
self._check_feature_filters(evaluation_event, targeting_context, **kwargs) | ||
|
||
self._assign_allocation(feature_flag, evaluation_event, targeting_context) | ||
self._assign_allocation(evaluation_event, targeting_context) | ||
return evaluation_event | ||
|
||
def list_feature_flag_names(self): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,35 +47,39 @@ def __init__(self, configuration, **kwargs): | |
self._filters[feature_filter.name] = feature_filter | ||
|
||
@staticmethod | ||
def _check_default_disabled_variant(feature_flag, evaluation_event): | ||
def _check_default_disabled_variant(evaluation_event): | ||
""" | ||
A method called when the feature flag is disabled, to determine what the default variant should be. If there is | ||
no allocation, then None is set as the value of the variant in the EvaluationEvent. | ||
|
||
:param FeatureFlag feature_flag: Feature flag object. | ||
:param EvaluationEvent evaluation_event: Evaluation event object. | ||
""" | ||
if not feature_flag.allocation: | ||
if not evaluation_event.feature.allocation: | ||
mrm9084 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
evaluation_event.enabled = False | ||
|
||
return | ||
FeatureManager._check_variant_override( | ||
feature_flag.variants, feature_flag.allocation.default_when_disabled, False, evaluation_event | ||
evaluation_event.feature.variants, | ||
evaluation_event.feature.allocation.default_when_disabled, | ||
False, | ||
evaluation_event, | ||
) | ||
|
||
@staticmethod | ||
def _check_default_enabled_variant(feature_flag, evaluation_event): | ||
def _check_default_enabled_variant(evaluation_event): | ||
""" | ||
A method called when the feature flag is enabled, to determine what the default variant should be. If there is | ||
no allocation, then None is set as the value of the variant in the EvaluationEvent. | ||
|
||
:param FeatureFlag feature_flag: Feature flag object. | ||
:param EvaluationEvent evaluation_event: Evaluation event object. | ||
""" | ||
if not feature_flag.allocation: | ||
if not evaluation_event.feature.allocation: | ||
mrm9084 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
evaluation_event.enabled = True | ||
return | ||
FeatureManager._check_variant_override( | ||
feature_flag.variants, feature_flag.allocation.default_when_enabled, True, evaluation_event | ||
evaluation_event.feature.variants, | ||
evaluation_event.feature.allocation.default_when_enabled, | ||
True, | ||
evaluation_event, | ||
) | ||
|
||
@staticmethod | ||
|
@@ -109,16 +113,15 @@ def _is_targeted(context_id): | |
|
||
return (context_marker / (2**32 - 1)) * 100 | ||
|
||
def _assign_variant(self, feature_flag, targeting_context, evaluation_event): | ||
def _assign_variant(self, targeting_context, evaluation_event): | ||
""" | ||
Assign a variant to the user based on the allocation. | ||
|
||
:param FeatureFlag feature_flag: Feature flag object. | ||
:param TargetingContext targeting_context: Targeting context. | ||
:param EvaluationEvent evaluation_event: Evaluation event object. | ||
:return: Variant name. | ||
""" | ||
evaluation_event.feature = feature_flag | ||
feature_flag = evaluation_event.feature | ||
if not feature_flag.variants or not feature_flag.allocation: | ||
return None | ||
if feature_flag.allocation.user and targeting_context.user_id: | ||
|
@@ -228,8 +231,8 @@ async def get_variant(self, feature_flag_id, *args, **kwargs): | |
result = await self._check_feature(feature_flag_id, targeting_context, **kwargs) | ||
return result.variant | ||
|
||
async def _check_feature_filters(self, feature_flag, evaluation_event, targeting_context, **kwargs): | ||
feature_conditions = feature_flag.conditions | ||
async def _check_feature_filters(self, evaluation_event, targeting_context, **kwargs): | ||
feature_conditions = evaluation_event.feature.conditions | ||
feature_filters = feature_conditions.client_filters | ||
|
||
if len(feature_filters) == 0: | ||
|
@@ -245,7 +248,7 @@ async def _check_feature_filters(self, feature_flag, evaluation_event, targeting | |
kwargs["user"] = targeting_context.user_id | ||
kwargs["groups"] = targeting_context.groups | ||
if filter_name not in self._filters: | ||
raise ValueError(f"Feature flag {feature_flag.name} has unknown filter {filter_name}") | ||
raise ValueError(f"Feature flag {evaluation_event.feature.name} has unknown filter {filter_name}") | ||
if feature_conditions.requirement_type == REQUIREMENT_TYPE_ALL: | ||
if not await self._filters[filter_name].evaluate(feature_filter, **kwargs): | ||
evaluation_event.enabled = False | ||
|
@@ -254,10 +257,11 @@ async def _check_feature_filters(self, feature_flag, evaluation_event, targeting | |
evaluation_event.enabled = True | ||
break | ||
|
||
def _assign_allocation(self, feature_flag, evaluation_event, targeting_context, **kwargs): | ||
def _assign_allocation(self, evaluation_event, targeting_context, **kwargs): | ||
feature_flag = evaluation_event.feature | ||
if feature_flag.allocation and feature_flag.variants: | ||
default_enabled = evaluation_event.enabled | ||
variant_name = self._assign_variant(feature_flag, targeting_context, evaluation_event, **kwargs) | ||
variant_name = self._assign_variant(targeting_context, evaluation_event, **kwargs) | ||
evaluation_event.enabled = default_enabled | ||
if variant_name: | ||
FeatureManager._check_variant_override( | ||
|
@@ -269,11 +273,11 @@ def _assign_allocation(self, feature_flag, evaluation_event, targeting_context, | |
|
||
variant_name = None | ||
if evaluation_event.enabled: | ||
FeatureManager._check_default_enabled_variant(feature_flag, evaluation_event) | ||
FeatureManager._check_default_enabled_variant(evaluation_event) | ||
if feature_flag.allocation: | ||
variant_name = feature_flag.allocation.default_when_enabled | ||
else: | ||
FeatureManager._check_default_disabled_variant(feature_flag, evaluation_event) | ||
FeatureManager._check_default_disabled_variant(evaluation_event) | ||
if feature_flag.allocation: | ||
variant_name = feature_flag.allocation.default_when_disabled | ||
evaluation_event.variant = self._variant_name_to_variant(feature_flag, variant_name) | ||
|
@@ -287,7 +291,6 @@ async def _check_feature(self, feature_flag_id, targeting_context, **kwargs): | |
:return: True if the feature flag is enabled for the given context. | ||
:rtype: bool | ||
""" | ||
evaluation_event = EvaluationEvent(enabled=False) | ||
if self._copy is not self._configuration.get(FEATURE_MANAGEMENT_KEY): | ||
self._cache = {} | ||
self._copy = self._configuration.get(FEATURE_MANAGEMENT_KEY) | ||
|
@@ -298,23 +301,24 @@ async def _check_feature(self, feature_flag_id, targeting_context, **kwargs): | |
else: | ||
feature_flag = self._cache.get(feature_flag_id) | ||
|
||
evaluation_event = EvaluationEvent(feature_flag) | ||
if not feature_flag: | ||
logging.warning("Feature flag %s not found", feature_flag_id) | ||
# Unknown feature flags are disabled by default | ||
return evaluation_event | ||
|
||
if not feature_flag.enabled: | ||
# Feature flags that are disabled are always disabled | ||
FeatureManager._check_default_disabled_variant(feature_flag, evaluation_event) | ||
FeatureManager._check_default_disabled_variant(evaluation_event) | ||
if feature_flag.allocation: | ||
variant_name = feature_flag.allocation.default_when_disabled | ||
evaluation_event.variant = self._variant_name_to_variant(feature_flag, variant_name) | ||
evaluation_event.feature = feature_flag | ||
return evaluation_event | ||
|
||
await self._check_feature_filters(feature_flag, evaluation_event, targeting_context, **kwargs) | ||
await self._check_feature_filters(evaluation_event, targeting_context, **kwargs) | ||
|
||
self._assign_allocation(feature_flag, evaluation_event, targeting_context, **kwargs) | ||
self._assign_allocation(evaluation_event, targeting_context, **kwargs) | ||
return evaluation_event | ||
|
||
def list_feature_flag_names(self): | ||
|
Uh oh!
There was an error while loading. Please reload this page.