-
Notifications
You must be signed in to change notification settings - Fork 7
New telemetry values #41
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
021723a
6b3e2b9
c32fa20
8384228
799a29e
43e5a79
1e7fd28
ebe7dad
3bb4fff
bcbc9e6
aebbe4e
dd22111
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 |
|---|---|---|
|
|
@@ -25,6 +25,8 @@ | |
|
|
||
| EVENT_NAME = "FeatureEvaluation" | ||
|
|
||
| EVALUATION_EVENT_VERSION = "1.0.0" | ||
|
|
||
|
|
||
| def track_event(event_name: str, user: str, event_properties: Optional[Dict[str, Optional[str]]] = None) -> None: | ||
| """ | ||
|
|
@@ -51,17 +53,49 @@ def publish_telemetry(evaluation_event: EvaluationEvent) -> None: | |
| """ | ||
| if not HAS_AZURE_MONITOR_EVENTS_EXTENSION: | ||
| return | ||
| event = {} | ||
| if evaluation_event.feature: | ||
| event[FEATURE_NAME] = evaluation_event.feature.name | ||
| event: Dict[str, Optional[str]] = {} | ||
| if not evaluation_event.feature: | ||
| return | ||
| event[FEATURE_NAME] = evaluation_event.feature.name | ||
| event[ENABLED] = str(evaluation_event.enabled) | ||
| event["Version"] = EVALUATION_EVENT_VERSION | ||
|
|
||
| # VariantAllocationPercentage | ||
| if evaluation_event.reason and evaluation_event.reason != VariantAssignmentReason.NONE: | ||
| if evaluation_event.variant: | ||
| event[VARIANT] = evaluation_event.variant.name | ||
| event[REASON] = evaluation_event.reason.value | ||
|
|
||
| if evaluation_event.feature and evaluation_event.feature.telemetry: | ||
| if evaluation_event.reason == VariantAssignmentReason.DEFAULT_WHEN_ENABLED: | ||
| allocation_percentage = 0 | ||
|
|
||
| if evaluation_event.feature.allocation and evaluation_event.feature.allocation.percentile: | ||
| for allocation in evaluation_event.feature.allocation.percentile: | ||
| if ( | ||
| evaluation_event.variant | ||
| and allocation.variant == evaluation_event.variant.name | ||
| and allocation.percentile_to | ||
zhiyuanliang-ms marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ): | ||
| allocation_percentage += allocation.percentile_to - allocation.percentile_from | ||
|
|
||
| event["VariantAssignmentPercentage"] = str(100 - allocation_percentage) | ||
zhiyuanliang-ms marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| elif evaluation_event.reason == VariantAssignmentReason.PERCENTILE: | ||
| if evaluation_event.feature.allocation and evaluation_event.feature.allocation.percentile: | ||
| allocation_percentage = 0 | ||
|
Member
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. Can you move this line before the above if statement to make it consistent with line 70
Contributor
Author
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. See my latest PR. I already made these changes. |
||
| for allocation in evaluation_event.feature.allocation.percentile: | ||
| if ( | ||
| evaluation_event.variant | ||
|
Member
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. It would be better to check whether evaluation_event.variant is None in advance so that we can shortcircuit before the for loop.
Member
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. Is it a possible case that variantAssignmentReason is not None but variant is None? I cannot imagine it will happen.
Contributor
Author
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. @zhiyuanliang-ms, I've made some changes already to this in my already existing PR that has some updates. #45 |
||
| and allocation.variant == evaluation_event.variant.name | ||
| and allocation.percentile_to | ||
| ): | ||
| allocation_percentage += allocation.percentile_to - allocation.percentile_from | ||
| event["VariantAssignmentPercentage"] = str(allocation_percentage) | ||
|
|
||
| # DefaultWhenEnabled | ||
| if evaluation_event.feature.allocation and evaluation_event.feature.allocation.default_when_enabled: | ||
| event["DefaultWhenEnabled"] = evaluation_event.feature.allocation.default_when_enabled | ||
|
Member
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. Just want to call out that it is possible that variant assignment reason is defaultWhenEnabled but the assigned variant is empty/none. The reason of this design is that if we have empty allocation configuration, (in .NET) we can consider it as: {
"allocation": {
"default_when_enabled" : ""
}
}
Member
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 don't whether it is a better choice to have event["DefaultWhenEnabled"] = "" when there is no allocation.default_when_enabled. But it could be another option for us.
Contributor
Author
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. |
||
|
|
||
| if evaluation_event.feature.telemetry: | ||
| for metadata_key, metadata_value in evaluation_event.feature.telemetry.metadata.items(): | ||
| if metadata_key not in event: | ||
| event[metadata_key] = metadata_value | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rossgrambo @mrm9084 Can you confirm whether this line is by design please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect- it's updated in https://github.com/microsoft/FeatureManagement-Python/pull/45/files#diff-e599fecd01f487185b6b19104e39d7a20d310657896ddeee742af944b129945a