Telemetry Support #24
Conversation
|
@mrm9084 The variant assignment reason should be VariantAssignmentReason.NONE only when there is no Variant section in feature flag configuration. Discussion can be found here BTW, I also didn't find the VariantAssignmentReason.DEFAULT_WHEN_ENABLED and DEFAULT_WHEN_DISABLED are used anywhere. Please refer to the .NET feature management. |
|
The overall variant assign reason should be: Only when there is no If if allocation is not none and we hit some allocation rule and get a variant, the reason should be that allocation rule name. if no allocation rule is hit and we get null/none variant, the reason should be "DefaultWhenEnabled" or "DefaultWhenDisabled" The status_override of variant will not affect the variant assignment reason, because it takes place after the variant is assigned. @zhenlan @jimmyca15 Please confirm whether this is correct. |
Co-authored-by: Zhiyuan Liang <141655842+zhiyuanliang-ms@users.noreply.github.com>
So, basically the default value of reason should be |
Sorry, I just updated my previous comments. I didn't explain it clearly. Only when there is no . |
We should try and be consistent in what we do between the libraries. As customers can access the By default, the value is But if the feature flag has any Implementation wise. Both Python and Dot Net use the |
| event[VARIANT] = evaluation_event.variant.name | ||
| event[REASON] = evaluation_event.reason.value | ||
|
|
||
| event["ETag"] = evaluation_event.feature.telemetry.metadata.get("etag", "") |
There was a problem hiding this comment.
We should publish all fields in feature.telemetry.metadata like this
There was a problem hiding this comment.
If python provider sets "etag", "feature_flag_reference", "feature_flag_id" fields to the telemetry metadata, I think we should have another PR to update them to pascal case names.
There was a problem hiding this comment.
@rossgrambo has this changed? When I wrote this originally, I copied the say outputs as DotNet.
There was a problem hiding this comment.
There was a problem hiding this comment.
@rossgrambo has this changed? When I wrote this originally, I copied the say outputs as DotNet.
You might be thinking of the provider. FeatureManagement never knew what an ETag was- it just grabs everything in metadata and blindly adds it.
There was a problem hiding this comment.
This should be correct now.
There was a problem hiding this comment.
The telemetry metadata field name is inconsistent with .NET provider.
| evaluation_event.variant = self._variant_name_to_variant(feature_flag, variant_name) | ||
| evaluation_event.feature = feature_flag | ||
| return evaluation_event | ||
| variant_name = self._assign_variant(targeting_context, evaluation_event) |
There was a problem hiding this comment.
Inconsistent with another feature manager. I am ok with another version.
There was a problem hiding this comment.
This should be fixed. I went through an made sure the sync and async code matches. After I finish this PR I really need to make a base class both sync and async extend that should remove a good amount of duplicate code.
There was a problem hiding this comment.
After I finish this PR I really need to make a base class both sync and async extend that should remove a good amount of duplicate code.
YESSSS!
| 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.
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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| feature_flag.variants, feature_flag.allocation.default_when_disabled, False | ||
| evaluation_event.reason = VariantAssignmentReason.DEFAULT_WHEN_DISABLED | ||
| if not evaluation_event.feature.allocation: | ||
| evaluation_event.enabled = False |
There was a problem hiding this comment.
In line 286,
if not evaluation_event.enabled:
FeatureManager._check_default_disabled_variant(evaluation_event)Setting evaluation_event.enabled = False in _check_default_disabled_variant is redundant.
In line 327,
if not feature_flag.enabled:
# Feature flags that are disabled are always disabled
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
# If a feature flag is disabled and override can't enable it
evaluation_event.enabled = False
return evaluation_eventAfter calling _check_default_disabled_variant, you set evaluation_event.enabled = False again. I understand this is handing the corner case for status_override.
Why not extract the logic of handling status_override and put it at the end of `_check_feature"?
You have short cut when if not feature_flag.enabled. Except for this case, the code path will always fall into the status override check.
I don't think it's necessary to add this layer of abstraction for both _check_default_disabled_variant and _check_default_enabled_variant methods. The logic within these two functions isn't complex, and they aren't used frequently. Writing the logic inline won't make the code harder to read.
There was a problem hiding this comment.
For the second part, I still want to set the variant info for is_enabled as it will be provided to telemetry either way. And I believe that is how DotNet also provides things.
| event[REASON] = evaluation_event.reason.value | ||
|
|
||
| for metadata_key, metadata_value in evaluation_event.feature.telemetry.metadata.items(): | ||
| if metadata_key not in event: |
There was a problem hiding this comment.
.NET Provider will put these fields in feature flag telemetry metadata.
public const string ETag = "ETag";
public const string FeatureFlagId = "FeatureFlagId";
public const string FeatureFlagReference = "FeatureFlagReference";However,in this PR
python provider will put these fields
ETAG_KEY = "etag"
FEATURE_FLAG_REFERENCE_KEY = "feature_flag_reference"
FEATURE_FLAG_ID_KEY = "feature_flag_id"There was a problem hiding this comment.
Will be updated in the Provider PR once Auto Failover has been completed.
Description
Adds Telemetry support + changes from design meeting