Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
e3fd558
telemetry support
mrm9084 Apr 4, 2024
a9847cc
updating from feedback
mrm9084 Apr 15, 2024
de6a442
Changes from design meeting
mrm9084 Apr 17, 2024
089268f
formatting, updating samples
mrm9084 Apr 17, 2024
2dbecec
Merge branch 'main' into Telemetry
mrm9084 Apr 17, 2024
43e2c04
fixing merge issue
mrm9084 Apr 17, 2024
4219544
typing issue
mrm9084 Apr 17, 2024
86a50e1
Merge branch 'main' into Telemetry
mrm9084 May 3, 2024
545175a
fixing test validations from changes
mrm9084 May 3, 2024
eb1c8c2
Update test_json_validations.py
mrm9084 May 10, 2024
61d0022
Apply suggestions from code review
mrm9084 May 13, 2024
b1a2b25
Updating doc strings
mrm9084 May 13, 2024
8025453
Spelling
mrm9084 May 13, 2024
06f9536
Updating async name.
mrm9084 May 13, 2024
40a1275
Adding missing eval reason. Fixed formatting.
mrm9084 May 13, 2024
3ec867b
Merge branch 'main' into Telemetry
mrm9084 Jul 1, 2024
7a41fa3
Fixing Merge issue
mrm9084 Jul 3, 2024
f839860
fixing merge
mrm9084 Jul 3, 2024
48a4c22
fixing merge
mrm9084 Jul 8, 2024
4cd65c3
review comments
mrm9084 Jul 9, 2024
0fb2000
Updating evaluation event usage
mrm9084 Jul 10, 2024
851e6bd
Updated feature flag usage
mrm9084 Jul 10, 2024
3ff66b3
updating assign allocation logic
mrm9084 Jul 10, 2024
7297743
formatting
mrm9084 Jul 10, 2024
7cb8935
Adding Just Open Telemetry Support
mrm9084 Jul 12, 2024
c5dd07d
Update _send_telemetry.py
mrm9084 Jul 12, 2024
765f38e
review items
mrm9084 Jul 15, 2024
b086f3b
Update test_send_telemetry_appinsights.py
mrm9084 Jul 15, 2024
ca41bd5
review comments
mrm9084 Jul 16, 2024
0d76953
fixing default
mrm9084 Jul 16, 2024
cead033
Removing Just Open Telemetry
mrm9084 Jul 17, 2024
ade2027
Removing open telemetry test
mrm9084 Jul 18, 2024
96b96b2
rename to azuremonitor
mrm9084 Jul 18, 2024
996e730
Update test_send_telemetry_appinsights.py
mrm9084 Jul 18, 2024
cfcc158
Update feature_variant_sample_with_telemetry.py
mrm9084 Jul 18, 2024
f6dddbd
Fixing Enabled = False usage with status override
mrm9084 Jul 19, 2024
b42b9c6
Removed extra false
mrm9084 Jul 23, 2024
822e4cd
Removed extra enabled
mrm9084 Jul 23, 2024
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
2 changes: 2 additions & 0 deletions dev_requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,5 @@ sphinx_rtd_theme
sphinx-toolbox
myst_parser
azure-appconfiguration-provider
azure-monitor-opentelemetry
azure-monitor-events-extension
4 changes: 3 additions & 1 deletion featuremanagement/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from ._featuremanager import FeatureManager
from ._featurefilters import FeatureFilter
from ._defaultfilters import TimeWindowFilter, TargetingFilter
from ._models import FeatureFlag, Variant, TargetingContext
from ._models import FeatureFlag, Variant, EvaluationEvent, VariantAssignmentReason, TargetingContext

from ._version import VERSION

Expand All @@ -18,5 +18,7 @@
"FeatureFilter",
"FeatureFlag",
"Variant",
"EvaluationEvent",
"VariantAssignmentReason",
"TargetingContext",
]
165 changes: 97 additions & 68 deletions featuremanagement/_featuremanager.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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):
Expand All @@ -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:
Expand All @@ -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,
)

@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):
Expand All @@ -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):
"""
Expand All @@ -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
Expand Down Expand Up @@ -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:
Copy link
Member

Choose a reason for hiding this comment

The 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 feature.telemetry.enabled check into the impl of push_telemetry?

if self._on_feature_evaluated

Reason:

  • on_feature_evaluated is a callback function when ff is evaluated. It has nothing to do with what's inside a feature flag, and it doesn't have to be publishing telemetry. So there is no reason we check the evaluation result to determine whether to execute the callback or not.
  • publish_telemetry is dedicated for telemetry. And it's reasonable that if feature.telemetry.enabled is false we don't send telemetry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

line 250 is checking if _on_feature_evaluated is not None this isn't actually calling the function.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't mean that. I mean line 250 should not check result.feature.telemetry.enabled because the following block is to call _on_feature_evaluated, which doesn't have to be something related to "telemetry". (line 252)

Copy link
Member

Choose a reason for hiding this comment

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

_on_feature_evaluated is the callback customers setup for the purpose of telemetry. So the idea from the design was that _on_feature_evaluated should not be called on flags that don't have telemetry enabled.

But I do agree that the naming sounds like this shouldn't be limited to telemetry. I think we should either:

  1. Rename the callback to something along the lines of "telemetry_callback"
  2. Always call the callback- and let that function handle the .telemetry.enabled check.

I prefer option 2, and we add the telemetry check at the start of def publish_telemetry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rossgrambo how does the DotNet Feature Management library work for this?

Copy link
Member

@rossgrambo rossgrambo Jul 16, 2024

Choose a reason for hiding this comment

The 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:

  1. To be in pairity with dotnet- don't call the callback at all
  2. Commit to the idea that this callback doesn't have to be telemetry related- and do the check only on the telemetry specific things.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

@rossgrambo rossgrambo Jul 18, 2024

Choose a reason for hiding this comment

The 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.

Then I think we should rename the callback to be "telemetry_callback" or something to make it clear that telemetry.enabled = false means it won't be called.

Personally- I prefer breaking from .net in regards to this callback- and placing the telemetry.enabled check inside of publish_telemetry.

result.user = targeting_context.user_id
self._on_feature_evaluated(result)
return result.enabled

@overload
Expand All @@ -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
Expand All @@ -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(
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):
"""
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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 check_variant_override called in _check_default_disabled_variant, _check_default_enabled_variant and _assign_variant. The code path is complex.

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()

_assign_allocation follows
#24 (comment)

I am ok with the current change on the _assign_variant method where default_when_enabled is also handled there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 is_enabled case we need to return false always if the feature flag isn't enabled, but the correct override variant in the case of get_variant?

Copy link
Member

@rossgrambo rossgrambo Jul 19, 2024

Choose a reason for hiding this comment

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

However, status_override should not work when feature_flag.enabled == false. Current behavior is wrong.

I don't think this is correct. We run both IsEnabled and Variant allocation on both .IsEnabled and .GetVariant calls, regardless of if the flag is enabled or disabled- then apply the override https://github.com/microsoft/FeatureManagement-Dotnet/blob/preview/src/Microsoft.FeatureManagement/FeatureManager.cs#L282

Copy link
Member

@rossgrambo rossgrambo Jul 19, 2024

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

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

doesn't the fact that this override doesn't work mean that it is more complex as in the is_enabled case we need to return false always if the feature flag isn't enabled, but the correct override variant in the case of get_variant?

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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):
"""
Expand Down
3 changes: 2 additions & 1 deletion featuremanagement/_models/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@
from ._feature_flag import FeatureFlag
from ._variant import Variant
from ._evaluation_event import EvaluationEvent
from ._variant_assignment_reason import VariantAssignmentReason
from ._targeting_context import TargetingContext

__path__ = __import__("pkgutil").extend_path(__path__, __name__) # type: ignore

__all__ = ["FeatureFlag", "Variant", "EvaluationEvent", "TargetingContext"]
__all__ = ["FeatureFlag", "Variant", "EvaluationEvent", "VariantAssignmentReason", "TargetingContext"]
4 changes: 2 additions & 2 deletions featuremanagement/_models/_evaluation_event.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@ class EvaluationEvent:
Represents a feature flag evaluation event.
"""

def __init__(self, *, enabled=False, feature_flag=None):
def __init__(self, feature_flag):
"""
Initialize the EvaluationEvent.
"""
self.feature = feature_flag
self.user = ""
self.enabled = enabled
self.enabled = False
self.variant = None
self.reason = None
19 changes: 19 additions & 0 deletions featuremanagement/_models/_variant_assignment_reason.py
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"
2 changes: 1 addition & 1 deletion featuremanagement/_version.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@
# license information.
# -------------------------------------------------------------------------

VERSION = "1.0.0"
VERSION = "2.0.0b1"
Loading