-
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 all commits
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
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,7 @@ | |
from typing import overload | ||
from ._defaultfilters import TimeWindowFilter, TargetingFilter | ||
from ._featurefilters import FeatureFilter | ||
from ._models import FeatureFlag, Variant, EvaluationEvent, TargetingContext | ||
from ._models import FeatureFlag, Variant, EvaluationEvent, VariantAssignmentReason, TargetingContext | ||
|
||
|
||
FEATURE_MANAGEMENT_KEY = "feature_management" | ||
|
@@ -73,6 +73,8 @@ class FeatureManager: | |
|
||
:param Mapping configuration: Configuration object. | ||
:keyword list[FeatureFilter] feature_filters: Custom filters to be used for evaluating feature flags. | ||
:keyword Callable[EvaluationEvent] on_feature_evaluated: Callback function to be called when a feature flag is | ||
evaluated. | ||
""" | ||
|
||
def __init__(self, configuration, **kwargs): | ||
|
@@ -82,6 +84,7 @@ def __init__(self, configuration, **kwargs): | |
self._configuration = configuration | ||
self._cache = {} | ||
self._copy = configuration.get(FEATURE_MANAGEMENT_KEY) | ||
self._on_feature_evaluated = kwargs.pop("on_feature_evaluated", None) | ||
filters = [TimeWindowFilter(), TargetingFilter()] + kwargs.pop(PROVIDED_FEATURE_FILTERS, []) | ||
|
||
for feature_filter in filters: | ||
|
@@ -90,54 +93,63 @@ def __init__(self, configuration, **kwargs): | |
self._filters[feature_filter.name] = feature_filter | ||
|
||
@staticmethod | ||
def _check_default_disabled_variant(feature_flag): | ||
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. | ||
:return: EvaluationEvent | ||
:param EvaluationEvent evaluation_event: Evaluation event object. | ||
""" | ||
if not feature_flag.allocation: | ||
return EvaluationEvent(enabled=False) | ||
return FeatureManager._check_variant_override( | ||
feature_flag.variants, feature_flag.allocation.default_when_disabled, False | ||
evaluation_event.reason = VariantAssignmentReason.DEFAULT_WHEN_DISABLED | ||
if not evaluation_event.feature.allocation: | ||
return | ||
FeatureManager._check_variant_override( | ||
evaluation_event.feature.variants, | ||
evaluation_event.feature.allocation.default_when_disabled, | ||
False, | ||
evaluation_event, | ||
) | ||
|
||
@staticmethod | ||
def _check_default_enabled_variant(feature_flag): | ||
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. | ||
:return: EvaluationEvent | ||
:param EvaluationEvent evaluation_event: Evaluation event object. | ||
""" | ||
if not feature_flag.allocation: | ||
return EvaluationEvent(enabled=True) | ||
return FeatureManager._check_variant_override( | ||
feature_flag.variants, feature_flag.allocation.default_when_enabled, True | ||
evaluation_event.reason = VariantAssignmentReason.DEFAULT_WHEN_ENABLED | ||
if not evaluation_event.feature.allocation: | ||
return | ||
FeatureManager._check_variant_override( | ||
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
|
||
) | ||
|
||
@staticmethod | ||
def _check_variant_override(variants, default_variant_name, status): | ||
def _check_variant_override(variants, default_variant_name, status, evaluation_event): | ||
""" | ||
A method to check if a variant is overridden to be enabled or disabled by the variant. | ||
|
||
:param list[Variant] variants: List of variants. | ||
:param str default_variant_name: Name of the default variant. | ||
:param bool status: Status of the feature flag. | ||
:return: EvaluationEvent | ||
:param EvaluationEvent evaluation_event: Evaluation event object. | ||
""" | ||
if not variants or not default_variant_name: | ||
return EvaluationEvent(enabled=status) | ||
evaluation_event.enabled = status | ||
return | ||
for variant in variants: | ||
if variant.name == default_variant_name: | ||
if variant.status_override == "Enabled": | ||
return EvaluationEvent(enabled=True) | ||
evaluation_event.enabled = True | ||
return | ||
if variant.status_override == "Disabled": | ||
return EvaluationEvent(enabled=False) | ||
return EvaluationEvent(enabled=status) | ||
evaluation_event.enabled = False | ||
return | ||
evaluation_event.enabled = status | ||
|
||
@staticmethod | ||
def _is_targeted(context_id): | ||
|
@@ -147,34 +159,45 @@ def _is_targeted(context_id): | |
|
||
return (context_marker / (2**32 - 1)) * 100 | ||
|
||
def _assign_variant(self, feature_flag, targeting_context): | ||
def _assign_variant(self, feature_flag, 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. | ||
:return: Variant name. | ||
:param EvaluationEvent evaluation_event: Evaluation event object. | ||
""" | ||
if not feature_flag.variants or not feature_flag.allocation: | ||
return None | ||
if feature_flag.allocation.user and targeting_context.user_id: | ||
for user_allocation in feature_flag.allocation.user: | ||
feature = evaluation_event.feature | ||
variant_name = None | ||
if not feature.variants or not feature.allocation: | ||
return | ||
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: | ||
return user_allocation.variant | ||
if feature_flag.allocation.group and len(targeting_context.groups) > 0: | ||
for group_allocation in feature_flag.allocation.group: | ||
evaluation_event.reason = VariantAssignmentReason.USER | ||
variant_name = user_allocation.variant | ||
elif 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: | ||
return group_allocation.variant | ||
if feature_flag.allocation.percentile: | ||
context_id = targeting_context.user_id + "\n" + feature_flag.allocation.seed | ||
evaluation_event.reason = VariantAssignmentReason.GROUP | ||
variant_name = group_allocation.variant | ||
elif 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 | ||
variant_name = percentile_allocation.variant | ||
if percentile_allocation.percentile_from <= box < percentile_allocation.percentile_to: | ||
return percentile_allocation.variant | ||
return None | ||
evaluation_event.reason = VariantAssignmentReason.PERCENTILE | ||
variant_name = percentile_allocation.variant | ||
if not variant_name: | ||
FeatureManager._check_default_enabled_variant(evaluation_event) | ||
evaluation_event.variant = self._variant_name_to_variant( | ||
feature_flag, feature_flag.allocation.default_when_enabled | ||
) | ||
return | ||
evaluation_event.variant = self._variant_name_to_variant(feature_flag, variant_name) | ||
FeatureManager._check_variant_override(feature_flag.variants, variant_name, True, evaluation_event) | ||
|
||
def _variant_name_to_variant(self, feature_flag, variant_name): | ||
""" | ||
|
@@ -186,6 +209,8 @@ def _variant_name_to_variant(self, feature_flag, variant_name): | |
""" | ||
if not feature_flag.variants: | ||
return None | ||
if not variant_name: | ||
return None | ||
for variant_reference in feature_flag.variants: | ||
if variant_reference.name == variant_name: | ||
configuration = variant_reference.configuration_value | ||
|
@@ -230,6 +255,9 @@ def is_enabled(self, feature_flag_id, *args, **kwargs): | |
targeting_context = self._build_targeting_context(args) | ||
|
||
result = self._check_feature(feature_flag_id, targeting_context, **kwargs) | ||
if self._on_feature_evaluated and result.feature.telemetry.enabled: | ||
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. Maybe consider to change this condition to the following only, and leave the
Reason:
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. line 250 is checking if 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. I didn't mean that. I mean line 250 should not check 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 I do agree that the naming sounds like this shouldn't be limited to telemetry. I think we should either:
I prefer option 2, and we add the telemetry check at the start of 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 how does the DotNet Feature Management library work for this? 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. When dotnet had the callback- we did not call the callback, but it was specifically a "TelemetryPublisher" that we were not calling. Today, if telemetry is disabled- dotnet won't create an activity or event- so there is no non-telemetry callback behavior. But- the reason why we didn't emit it anyway is because we didn't want to add too much to their telemetry, which would happen if OpenTelemetry ingested it. That wouldn't happen here. So either:
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, I think we should default to parity and if DotNet changes we can too. 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.
Then I think we should rename the callback to be "telemetry_callback" or something to make it clear that Personally- I prefer breaking from .net in regards to this callback- and placing the |
||
result.user = targeting_context.user_id | ||
self._on_feature_evaluated(result) | ||
return result.enabled | ||
|
||
@overload | ||
|
@@ -255,12 +283,15 @@ def get_variant(self, feature_flag_id, *args, **kwargs): | |
targeting_context = self._build_targeting_context(args) | ||
|
||
result = self._check_feature(feature_flag_id, targeting_context, **kwargs) | ||
if self._on_feature_evaluated and result.feature.telemetry.enabled: | ||
result.user = targeting_context.user_id | ||
self._on_feature_evaluated(result) | ||
return result.variant | ||
|
||
def _check_feature_filters(self, feature_flag, 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 | ||
evaluation_event = EvaluationEvent(enabled=False) | ||
|
||
if len(feature_filters) == 0: | ||
# Feature flags without any filters return evaluate | ||
|
@@ -283,31 +314,24 @@ def _check_feature_filters(self, feature_flag, targeting_context, **kwargs): | |
elif self._filters[filter_name].evaluate(feature_filter, **kwargs): | ||
evaluation_event.enabled = True | ||
break | ||
return evaluation_event | ||
|
||
def _assign_allocation(self, feature_flag, evaluation_event, targeting_context): | ||
if feature_flag.allocation and feature_flag.variants: | ||
variant_name = self._assign_variant(feature_flag, targeting_context) | ||
if variant_name: | ||
evaluation_event.enabled = FeatureManager._check_variant_override( | ||
feature_flag.variants, variant_name, evaluation_event.enabled | ||
).enabled | ||
evaluation_event.variant = self._variant_name_to_variant(feature_flag, variant_name) | ||
evaluation_event.feature = feature_flag | ||
return evaluation_event | ||
|
||
variant_name = None | ||
if evaluation_event.enabled: | ||
evaluation_event = FeatureManager._check_default_enabled_variant(feature_flag) | ||
if feature_flag.allocation: | ||
variant_name = feature_flag.allocation.default_when_enabled | ||
else: | ||
evaluation_event = FeatureManager._check_default_disabled_variant(feature_flag) | ||
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 | ||
def _assign_allocation(self, evaluation_event, targeting_context): | ||
feature_flag = evaluation_event.feature | ||
if feature_flag.variants: | ||
if not feature_flag.allocation: | ||
if evaluation_event.enabled: | ||
evaluation_event.reason = VariantAssignmentReason.DEFAULT_WHEN_ENABLED | ||
return | ||
evaluation_event.reason = VariantAssignmentReason.DEFAULT_WHEN_DISABLED | ||
return | ||
if not evaluation_event.enabled: | ||
FeatureManager._check_default_disabled_variant(evaluation_event) | ||
evaluation_event.variant = self._variant_name_to_variant( | ||
mrm9084 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
feature_flag, feature_flag.allocation.default_when_disabled | ||
) | ||
return | ||
|
||
self._assign_variant(feature_flag, targeting_context, evaluation_event) | ||
|
||
def _check_feature(self, feature_flag_id, targeting_context, **kwargs): | ||
""" | ||
|
@@ -327,23 +351,28 @@ 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 EvaluationEvent(enabled=False) | ||
return evaluation_event | ||
|
||
if not feature_flag.enabled: | ||
# Feature flags that are disabled are always disabled | ||
evaluation_event = FeatureManager._check_default_disabled_variant(feature_flag) | ||
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 | ||
|
||
# If a feature flag is disabled and override can't enable it | ||
evaluation_event.enabled = False | ||
return evaluation_event | ||
|
||
evaluation_event = self._check_feature_filters(feature_flag, targeting_context, **kwargs) | ||
self._check_feature_filters(evaluation_event, targeting_context, **kwargs) | ||
|
||
return 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 |
---|---|---|
@@ -0,0 +1,19 @@ | ||
# ------------------------------------------------------------------------ | ||
# Copyright (c) Microsoft Corporation. All rights reserved. | ||
# Licensed under the MIT License. See License.txt in the project root for | ||
# license information. | ||
# ------------------------------------------------------------------------- | ||
from enum import Enum | ||
|
||
|
||
class VariantAssignmentReason(Enum): | ||
""" | ||
Represents an assignment reason. | ||
""" | ||
|
||
NONE = "None" | ||
DEFAULT_WHEN_DISABLED = "DefaultWhenDisabled" | ||
DEFAULT_WHEN_ENABLED = "DefaultWhenEnabled" | ||
USER = "User" | ||
GROUP = "Group" | ||
PERCENTILE = "Percentile" |
Uh oh!
There was an error while loading. Please reload this page.