Conversation
️✔️AzureCLI-FullTest
|
|
Hi @ChristineWanjau, |
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| appconfig create | cmd appconfig create added parameter appinsights_resource |
||
| appconfig feature set | cmd appconfig feature set added parameter telemetry_enabled |
||
| appconfig update | cmd appconfig update added parameter appinsights_resource |
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
microsoft-github-policy-service
bot
requested review from
avanigupta,
jsntcy,
yanzhudd,
yonzhan and
zhoxing-ms
microsoft-github-policy-service
bot
added
App Configuration
Auto-Assign
There was a problem hiding this comment.
Pull request overview
This PR adds support for linking Application Insights resources to Azure App Configuration stores and enables telemetry collection for feature flags. The changes introduce the ability to track and monitor feature flag usage through Application Insights.
Changes:
- Updated
azure-mgmt-appconfigurationdependency from 5.0.0 to 6.0.0b1 to support new telemetry features - Added
--appinsights-resourceparameter toappconfig createandappconfig updatecommands for linking/unlinking Application Insights - Added
--telemetry-enabledparameter toappconfig feature setcommand to enable/disable telemetry on feature flags - Implemented warning system to notify users when telemetry is enabled without Application Insights linked
- Added comprehensive test coverage for both management plane (App Insights linking) and data plane (feature telemetry) operations
Reviewed changes
Copilot reviewed 11 out of 19 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/azure-cli/setup.py | Updated azure-mgmt-appconfiguration dependency to 6.0.0b1 |
| src/azure-cli/requirements.py3.windows.txt | Updated azure-mgmt-appconfiguration dependency to 6.0.0b1 for Windows |
| src/azure-cli/requirements.py3.Linux.txt | Updated azure-mgmt-appconfiguration dependency to 6.0.0b1 for Linux |
| src/azure-cli/requirements.py3.Darwin.txt | Updated azure-mgmt-appconfiguration dependency to 6.0.0b1 for macOS |
| src/azure-cli/azure/cli/command_modules/appconfig/tests/latest/test_appconfig_mgmt_commands.py | Added test for linking/unlinking Application Insights to App Configuration store |
| src/azure-cli/azure/cli/command_modules/appconfig/tests/latest/test_appconfig_feature_commands.py | Added test for feature flag telemetry functionality with warning validation |
| src/azure-cli/azure/cli/command_modules/appconfig/tests/latest/recordings/test_azconfig_appinsights.yaml | Test recording file for App Insights linking scenario |
| src/azure-cli/azure/cli/command_modules/appconfig/feature.py | Implemented telemetry support in feature set command with App Insights linkage warning |
| src/azure-cli/azure/cli/command_modules/appconfig/custom.py | Added appinsights_resource parameter handling in create and update store operations |
| src/azure-cli/azure/cli/command_modules/appconfig/_params.py | Added appinsights_resource and telemetry_enabled parameter definitions |
| src/azure-cli/azure/cli/command_modules/appconfig/_help.py | Added help documentation and examples for telemetry features |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| is_linked = bool(getattr(telemetry, "resource_id", None)) if telemetry else False | ||
|
|
||
| if not is_linked: | ||
| logger.warning( |
There was a problem hiding this comment.
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.
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.
We support other methods, they are just not built into the libraries.
There was a problem hiding this comment.
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.
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
|
|
||
| except Exception as ex: # pylint: disable=broad-except | ||
| logger.warning( | ||
| "Unable to verify App Insights resource for the App Configuration store: %s", str(ex) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The exception catches failures from resolve_metadata, which can fail if the logged-in user lacks sufficient permissions to list stores in the subscription or if the store name doesn't match any existing store. The specific error message from resolve_metadata should propagate and be caught by this exception.
| feature_flag_value.telemetry.enabled = telemetry_enabled | ||
|
|
||
| if telemetry_enabled: | ||
| warn_if_app_insights_not_set(cmd, name) |
There was a problem hiding this comment.
I might be missing something, but this check will run twice if this is hit as it will have already been checked on line 121.
There was a problem hiding this comment.
Yes it will run twice. Updated to just have line 121. Thanks
|
Please fix CI issues |
yanzhudd
changed the title
az appconfig create/update/feature set: Add support to enable telemetryaz appconfig create/update/feature set: Add support to enable telemetry
Related command
Description
This PR adds support to link an app insight resource to an app configuration store and enable telemetry to a feature flag.
Testing Guide
History Notes
[AppConfig]
az appconfig create/update: Enable linking app insights resource to an app configuration store[AppConfig]
az appconfig feature set: Enable telemetry for a feature flagThis checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.