Load feature flags from the dedicated feature flag resource endpoint#48222
Load feature flags from the dedicated feature flag resource endpoint#48222yuanqu72 wants to merge 1 commit into
Conversation
…in the provider Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: 10 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
/azp run |
|
You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list. |
|
/azp run appconfiguration |
|
No pipelines are associated with this pull request. |
|
/azp run python - appconfiguration |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
| return False | ||
|
|
||
| @distributed_trace | ||
| @distributed_trace |
| loaded_feature_flags: List[FeatureFlag] = [] | ||
| page_etags: List[List[str]] = [] | ||
| # Needs to be removed unknown keyword argument for the feature flag client | ||
| kwargs.pop("sentinel_keys", None) |
There was a problem hiding this comment.
I know this is in the other methods, but we shouldn't need it anymore. We have a utils method that validates the header.
| return loaded_feature_flags, [[] for _ in feature_flag_selectors] | ||
| for select in feature_flag_selectors: | ||
| selector_etags: List[str] = [] | ||
| if select.snapshot_name is not None: |
There was a problem hiding this comment.
This seems weird, I know that we use the same selects as "key value feature flags".
| selector_etags: List[str] = [] | ||
| if select.snapshot_name is not None: | ||
| # Snapshots are not supported by the feature flag resource endpoint | ||
| page_etags.append(selector_etags) |
There was a problem hiding this comment.
Is there a way to not have this, it seems weird having an empty selector for something that wasn't loaded. Typically we do this to support selecting an empty set, and that having the item appear live.
| async for page in iterator: | ||
| async for ff in page: | ||
| loaded_feature_flags.append(ff) | ||
| selector_etags.append(iterator.etag) # type: ignore[attr-defined] |
There was a problem hiding this comment.
The fact that we ignore this seems wrong. Either we have something wrong above, or the sdk isn't correct.
| if self._feature_flag_client is None: | ||
| return False | ||
| for i, select in enumerate(feature_flag_selectors): | ||
| if select.snapshot_name is not None: |
There was a problem hiding this comment.
I'm wondering if instead of skipping the snapshots every time, we just filter them out at startup. We can then remove all of the snapshot logic from the 'new feature flags`.
| retry_backoff_max=retry_backoff_max, | ||
| **kwargs, | ||
| ), | ||
| FeatureFlagClient( |
There was a problem hiding this comment.
We should probably only create a feature flag client if feature flag loading is enabled.
| # Per-selector collection ETags for feature flags loaded from the feature flag resource endpoint. This is | ||
| # independent of the key-value based feature_flag_page_etags, since the resource endpoint is a separate | ||
| # resource type with its own change-detection mechanism. | ||
| self._feature_flag_resource_etags: List[List[str]] = [] | ||
| # Feature flags are loaded from two independent sources: the classic key-value store, and the newer | ||
| # dedicated feature flag resource endpoint. Each source's processed output is cached separately so that a | ||
| # refresh of one source does not require re-processing or discarding the other source's data. The two are | ||
| # merged (resource-based feature flags take precedence on identifier collision) whenever either changes. |
There was a problem hiding this comment.
These and a few others sort of seem like copilot comments about the properties. This seems more like copilot direction than property descriptions.
| # Key-value based feature flags store the variant value under "configuration_value". Feature | ||
| # flags loaded from the feature flag resource endpoint store it under "value" instead. |
There was a problem hiding this comment.
This is a problem https://github.com/microsoft/FeatureManagement/blob/01d585c91d128457e49634f773813e9530c6d82b/Schema/FeatureFlag.v2.0.0.schema.json#L149
@jimmyca15, was this intentional? I didn't realize this till now.
| if "configuration_value" in v: | ||
| allocation_id += ( | ||
| f"{json.dumps(v.get('configuration_value', ''), separators=(',', ':'), sort_keys=True)}" | ||
| ) | ||
| elif "value" in v: | ||
| allocation_id += f"{json.dumps(v.get('value', ''), separators=(',', ':'), sort_keys=True)}" |
There was a problem hiding this comment.
I'm wondering if we want to swap this around. FF endpoint feature flags have a server-side enforced schema, while ff value feature flags don't.
Either way we should use reference_path_segment so we don't guess.
| ALLOCATION_ID_KEY = "AllocationId" | ||
| ETAG_KEY = "ETag" | ||
|
|
||
| # Identifier field used by feature flags loaded from the classic key-value store. |
There was a problem hiding this comment.
Update everywhere with the new terms. "Feature Flags" and "Enhanced Feature Flags"
| FEATURE_FLAG_NAME_FIELD: feature_flag.name, | ||
| "enabled": feature_flag.enabled, | ||
| } | ||
| if feature_flag.label and not feature_flag.label.isspace(): |
There was a problem hiding this comment.
I don't think we do this isspace check anywhere else. I just checked it isn't against our test api, but it isn't allowed in the portal.
| # Feature flag value is not a valid JSON | ||
| return {} | ||
|
|
||
| def _process_feature_flag_resource(self, feature_flag: FeatureFlag) -> Dict[str, Any]: |
There was a problem hiding this comment.
If we can't make all kv feature flags FeatureFlag maybe make a helper method for this. I'd rather not have two copies of this logic.
| merged[identifier] = ff | ||
| for ff in resource_feature_flags: | ||
| identifier = ff.get(FEATURE_FLAG_NAME_FIELD) | ||
| merged[identifier] = ff |
There was a problem hiding this comment.
I just realized this is also going to be an issue. We need to convert these to JSON, then change the name field to be 'id' to match the schema.
Can you validate that this new provider works with the feature flag library with zero changes on the feature flag libraries part.
| * **Configuration not refreshing** — Make sure you are calling `config.refresh()` periodically (e.g., before each request in a web app). The provider does not auto-refresh in the background. | ||
| * **Startup failures** — If the store is unreachable during startup, the provider will retry until `startup_timeout` (default 100 seconds) is exceeded. Increase this value if your store is expected to have high latency. | ||
|
|
||
| ## Testing |
There was a problem hiding this comment.
I think we should move this to it's own file, or a readme inside the testing folder.
There was a problem hiding this comment.
Pull request overview
Adds dedicated feature-flag resource loading to the App Configuration provider, alongside existing key-value feature flags.
Changes:
- Adds synchronous and asynchronous loading, merging, refresh, and telemetry support.
- Adds integration/unit tests and samples.
- Updates dependency, version, changelog, documentation, and recordings.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
tests/testcase.py |
Adds resource test helpers. |
tests/test_provider_feature_flag_resources.py |
Tests synchronous resource loading. |
tests/test_configuration_client_manager.py |
Tests resource paging and ETags. |
tests/test_azureappconfigurationproviderbase.py |
Tests conversion and merging. |
tests/asynctestcase.py |
Adds asynchronous test helpers. |
tests/aio/test_async_provider_feature_flag_resources.py |
Tests asynchronous resource loading. |
setup.py |
Raises the App Configuration dependency. |
samples/README.md |
Lists the new samples. |
samples/feature_flag_resource_sample.py |
Demonstrates synchronous usage. |
samples/async_feature_flag_resource_sample.py |
Demonstrates asynchronous usage. |
README.md |
Documents resource flags and testing. |
CHANGELOG.md |
Records the feature and dependency update. |
aio/_azureappconfigurationproviderasync.py |
Integrates asynchronous loading and refresh. |
aio/_async_client_manager.py |
Adds asynchronous resource-client support. |
_version.py |
Sets version 2.6.0b1. |
_request_tracing_context.py |
Tracks filters by name. |
_constants.py |
Adds identifier and reference constants. |
_client_manager.py |
Adds synchronous resource-client support. |
_azureappconfigurationproviderbase.py |
Converts, caches, and merges resource flags. |
_azureappconfigurationprovider.py |
Integrates synchronous loading and refresh. |
assets.json |
Updates test recording assets. |
Comments suppressed due to low confidence (1)
sdk/appconfiguration/azure-appconfiguration-provider/azure/appconfiguration/provider/_azureappconfigurationproviderbase.py:455
- An empty list is a valid refreshed result when the last resource flag is deleted, but this truthiness check leaves
_processed_resource_feature_flagsunchanged; the subsequent merge therefore keeps serving the deleted flag. TreatNoneas “not refreshed” and an empty list as “refreshed to empty” for both sources and for the merge condition.
if feature_flag_resources:
| if feature_flags or feature_flag_resources: | ||
| # Reset feature flag usage | ||
| self._tracing_context.reset_feature_filter_usage() |
| feature_flag_value: Dict[str, Any] = { | ||
| FEATURE_FLAG_NAME_FIELD: feature_flag.name, | ||
| "enabled": feature_flag.enabled, |
| @distributed_trace | ||
| @distributed_trace |
| if (settings_refreshed or feature_flags or feature_flag_resources) and self._on_refresh_success: | ||
| self._on_refresh_success() |
| if (settings_refreshed or feature_flags or feature_flag_resources) and self._on_refresh_success: | ||
| self._on_refresh_success() |
| if not self._feature_flag_resource_etags or client.check_feature_flag_resource_etags( | ||
| self._feature_flag_selectors, self._feature_flag_resource_etags, headers=headers, **kwargs | ||
| ): | ||
| feature_flag_resources, feature_flag_resource_etags = client.load_feature_flag_resources( |
Summary
Loads feature flags from the new dedicated Feature Flag resource endpoint (
FeatureFlagClient/FeatureFlag) inazure-appconfiguration-provider, in addition to the existing key-value based feature flags.feature_flag_enabled=True, feature flags created via the feature flag resource endpoint are now loaded automatically alongside key-value based feature flags.feature_management.feature_flagslist. When a resource-based feature flag and a key-value based feature flag share the same name, the resource-based one takes precedence.load()options are required to opt in; existingfeature_flag_selectorsfilter both kinds.azure-appconfigurationto>=1.10.0b1forFeatureFlagClient/FeatureFlagsupport.Dependency on #47620
AppConfig-FFEndpointbranch), which introduces theFeatureFlagClientAPIs inazure-appconfigurationthat this change depends on. Because of that, this PR targetsAppConfig-FFEndpointinstead ofmain— the diff here is scoped only tosdk/appconfiguration/azure-appconfiguration-provider.This PR should be reviewed/merged after (or together with) #47620, once that lands in
main.Changes
_azureappconfigurationprovider.py/aio/_azureappconfigurationproviderasync.py: load and merge feature flags from the feature flag resource endpoint_client_manager.py/aio/_async_client_manager.py: new client manager support for feature flag resource clients_constants.py,_request_tracing_context.py: new constants and request tracing updates for the feature flag resource pathsamples/feature_flag_resource_sample.py,samples/async_feature_flag_resource_sample.pytests/test_provider_feature_flag_resources.py,tests/test_async_provider_feature_flag_resources.py,tests/test_azureappconfigurationproviderbase.py,tests/test_configuration_client_manager.pyCHANGELOG.md,README.md,samples/README.mdupdatedTesting