-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[App Config] az appconfig create/update/feature set: Add support to enable telemetry
#32831
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
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 |
|---|---|---|
|
|
@@ -30,24 +30,54 @@ | |
| convert_keyvalue_to_configurationsetting) | ||
| from ._utils import (get_appconfig_data_client, | ||
| prep_filter_for_url_encoding, | ||
| validate_feature_flag_name) | ||
| validate_feature_flag_name, | ||
| resolve_store_metadata) | ||
| from ._featuremodels import (map_keyvalue_to_featureflag, | ||
| map_keyvalue_to_featureflagvalue, | ||
| FeatureFilter) | ||
| FeatureFilter, | ||
| FeatureTelemetry) | ||
|
|
||
|
|
||
| logger = get_logger(__name__) | ||
|
|
||
| # Feature commands # | ||
|
|
||
|
|
||
| def warn_if_app_insights_not_set(cmd, store_name): | ||
| """ | ||
| Check if Application Insights resource is set for the App Configuration store. | ||
| Emits a warning if not set or if the check cannot be completed. | ||
| """ | ||
| from ._client_factory import cf_configstore | ||
|
|
||
| try: | ||
| resource_group_name, _ = resolve_store_metadata(cmd, store_name) | ||
| configstore_client = cf_configstore(cmd.cli_ctx) | ||
| store = configstore_client.get(resource_group_name, store_name) | ||
|
|
||
| telemetry = getattr(store, "telemetry", None) | ||
| is_linked = bool(getattr(telemetry, "resource_id", None)) if telemetry else False | ||
|
|
||
| if not is_linked: | ||
| logger.warning( | ||
| "App Insights resource for the App Configuration store is not set." | ||
| "To collect telemetry, connect to an App Insights resource." | ||
| ) | ||
|
|
||
| except Exception as ex: # pylint: disable=broad-except | ||
| logger.warning( | ||
| "Unable to verify App Insights resource for the App Configuration store: %s", str(ex) | ||
|
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. This also links to my comment on line 62. What is the actual failure case her in the verify? They have a linked store that was deleted, but still linked? Are we able to give a more helpful error message on our part.
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. The exception catches failures from |
||
| ) | ||
|
|
||
|
|
||
| def set_feature(cmd, | ||
| feature=None, | ||
| key=None, | ||
| name=None, | ||
| label=None, | ||
| description=None, | ||
| requirement_type=None, | ||
| telemetry_enabled=None, | ||
| yes=False, | ||
| connection_string=None, | ||
| auth_mode="key", | ||
|
|
@@ -84,6 +114,12 @@ def set_feature(cmd, | |
| FeatureFlagConstants.CONDITIONS: default_conditions | ||
| } | ||
|
|
||
| # Add telemetry if telemetry_enabled is specified | ||
| if telemetry_enabled is not None: | ||
| default_value[FeatureFlagConstants.TELEMETRY] = {FeatureFlagConstants.ENABLED: telemetry_enabled} | ||
| if telemetry_enabled: | ||
| warn_if_app_insights_not_set(cmd, name) | ||
|
|
||
| azconfig_client = get_appconfig_data_client(cmd, name, connection_string, auth_mode, endpoint) | ||
|
|
||
| retry_times = 3 | ||
|
|
@@ -135,6 +171,13 @@ def set_feature(cmd, | |
| else FeatureFlagConstants.REQUIREMENT_TYPE_ANY | ||
| ) | ||
|
|
||
| # Update telemetry if telemetry_enabled is specified | ||
| if telemetry_enabled is not None: | ||
| if feature_flag_value.telemetry is None: | ||
| feature_flag_value.telemetry = FeatureTelemetry(enabled=telemetry_enabled) | ||
| else: | ||
| feature_flag_value.telemetry.enabled = telemetry_enabled | ||
|
|
||
| set_kv = KeyValue(key=key, | ||
| label=label, | ||
| value=json.dumps(feature_flag_value, default=lambda o: {k: v for k, v in o.__dict__.items() if v is not None}, ensure_ascii=False), | ||
|
|
||
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.
What is the logic behind this being a warning? This is actually something not supported by our portal UI.
Also, the warning is a bit wrong. If we do allow this, assuming the customer setup there application correctly, it will generate telemetry. It will just not be visible via the App Configuration Azure Portal.
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.
On Portal, we show this warning and we disable enabling the flag's telemetry if app insights is not set.

The idea is just to show a warning if app insight is not linked to the store but allow users to enable telemetry for their flags. Just to confirm we support other avenues to collect telemetry other than app insights?
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.
We support other methods, they are just not built into the libraries.
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.
Since we still allow enabling telemetry even though an app insights resource is not linked. would it be more accurate to say that telemetry will not be visible from the App Configuration portal if an app insights resource is not linked?
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.
I also remember from the email thread Christine had with us and some other folks that Zhenlan suggested we keep the portal warning but allow folks to enable/disable the telemetry toggle. We just won't show a graph if there is no linked app insights. I think this would unblock customers who might be using the other methods @mrm9084 is talking about in the event where they need to enable telemetry on portal/cli