Add `RegisterAll` API to enable monitoring collections of key-values for refresh #574

amerjusupovic · 2024-07-15T22:10:27Z

This PR adds the new RegisterAll API to monitor any key-values added with AzureAppConfigurationOptions.Select for refresh as a collection. If a refresh is triggered and any of these registered key-values have changed, all key-values and feature flags will be reloaded. This PR will also begin monitoring feature flags as a collection instead of on an individual basis. Consequently, feature flag changes will no longer be logged on an individual basis. This solves an existing issue.

Example usage

configurationBuilder.AddAzureAppConfiguration(options =>
{
    options.Select("TestApp*");
    options.Select("Settings*");

    options.ConfigureRefresh(refresh =>
    {
        // Calling RegisterAll will monitor all key-values beginning with TestApp or Settings for refresh.
        refresh.RegisterAll();
    });
}

…otnetProvider into ajusupovic/collection-monitoring

jimmyca15 · 2025-01-15T18:39:44Z

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs


-                        foreach (KeyValueWatcher changeWatcher in refreshableWatchers.Concat(refreshableMultiKeyWatchers))
+                        // Invalidate all the cached KeyVault secrets
+                        foreach (IKeyValueAdapter adapter in _options.Adapters)


Where do deleted flags get removed?

discussed offline, lost this along the way when restructuring the refresh methods. I'll try checking the existing data and using that to decide whether the setting for that flag's key should be updated. Might need a new data structure/method for this

…ove flags on refresh

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs

jimmyca15 · 2025-01-18T00:25:06Z

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/Constants/LoggingConstants.cs

@@ -22,12 +22,14 @@ internal class LoggingConstants
        public const string RefreshKeyVaultSecretRead = "Secret read from Key Vault for key-value.";
        public const string RefreshFeatureFlagRead = "Feature flag read from App Configuration.";


Is this still used?

No, will remove

jimmyca15 · 2025-01-18T00:27:05Z

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs

+                                ffKeys,
+                                cancellationToken).ConfigureAwait(false);
+
+                            logInfoBuilder.Append(LogHelper.BuildFeatureFlagsUpdatedMessage());


I don't see anywhere that we logged feature flag updates as info before. Am I missing it.

Also, why

We had a LogFeatureFlagUpdated message when refreshing individual flags in the ConfigurationClientExtensions file, so I thought it might make sense to log the collection being updated since it seems like whenever we update a value (kv, flag, secret) we would log it as info.

I see. In that case, I see for configuration we log "Configuration reloaded.", but for flags it is currently "Feature flags updated." It seems to me we should be consistent and log "Feature flags reloaded."

Right, they both do a full reload so it makes sense

jimmyca15 · 2025-01-18T00:40:21Z

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs

+                if (haveCollectionsChanged)
+                {
+                    return haveCollectionsChanged;
+                }


Suggested change

if (haveCollectionsChanged)

{

return haveCollectionsChanged;

}

if (haveCollectionsChanged)

{

return true;

}

jimmyca15 · 2025-01-18T00:44:38Z

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationOptions.cs

@@ -376,18 +406,40 @@ public AzureAppConfigurationOptions ConfigureClientOptions(Action<ConfigurationC
        /// <param name="configure">A callback used to configure Azure App Configuration refresh options.</param>
        public AzureAppConfigurationOptions ConfigureRefresh(Action<AzureAppConfigurationRefreshOptions> configure)
        {
+            if (RegisterAllEnabled)
+            {
+                throw new ArgumentException($"{nameof(ConfigureRefresh)}() cannot be invoked multiple times when {nameof(AzureAppConfigurationRefreshOptions.RegisterAll)} has been invoked.");


Should be an invalid operation exception. There aren't any invalid arguments.

jimmyca15 · 2025-01-18T00:51:38Z

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationOptions.cs

+
+            if (!isRegisterCalled && !RegisterAllEnabled)
+            {
+                throw new ArgumentException($"{nameof(ConfigureRefresh)}() must register at least one key-value for refresh or enable refresh of all selected key-values.");


Does this mean that one must either call Register or RegisterAll, if so then I think it would be helpful to mention that explicitly.

Yes, since we required calling Register before, so now at least one should be called. I can update it to mention the specific APIs instead of this description in the exception

jimmyca15 · 2025-01-21T19:46:07Z

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationOptions.cs

            {
-                throw new ArgumentException($"{nameof(ConfigureRefresh)}() must have at least one key-value registered for refresh.");
+                throw new ArgumentException($"Cannot call both {nameof(AzureAppConfigurationRefreshOptions.RegisterAll)} and "


InvalidOperationException.

jimmyca15 · 2025-01-21T19:46:35Z

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationOptions.cs

+
+            if (!isRegisterCalled && !RegisterAllEnabled)
+            {
+                throw new ArgumentException($"{nameof(ConfigureRefresh)}() must call either {nameof(AzureAppConfigurationRefreshOptions.Register)}()" +


This should also be an invalid operation exception since there are no invalid arguments.

…tants to match behavior

avanigupta · 2025-01-22T01:35:05Z

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/Constants/ErrorMessages.cs

@@ -10,5 +10,6 @@ internal class ErrorMessages
        public const string FeatureFlagInvalidJsonProperty = "Invalid property '{0}' for feature flag. Key: '{1}'. Found type: '{2}'. Expected type: '{3}'.";
        public const string FeatureFlagInvalidFormat = "Invalid json format for feature flag. Key: '{0}'.";
        public const string InvalidKeyVaultReference = "Invalid Key Vault reference.";
+        public const string InvalidConfigurationSettingPage = "Invalid page while loading configuration settings.";


Not used. File changes can be reverted.

jimmyca15 · 2025-01-30T15:51:02Z

Can you provide a usage snippet in the PR description.

…-values for refresh (#574)" This reverts commit 5e6a012.

…for refresh (#574) * WIP * WIP testing out client extensions methods * WIP added selectors to multikeywatchers * remove unused property * WIP check for registerall changes to change refreshall * WIP * WIP fixing types and reslving errors * WIP fixing client extensions class * WIP * WIP update feature flag logic * WIP client extensions * WIP reload all flags on change * WIP * WIP fixing tests to return response for getconfigurationsettingsasync * WIP etag for tests * fix watchedcollections null * WIP tests, working for examples * remove unused variables * update to newest sdk version, remove unused * WIP fixing tests * WIP reworking testing to work with new etag approach * tests passing, fix mockasyncpageable * update sdk package version * fix loghelper, tests * WIP fixing aspages tests * revert watchesfeatureflags test * update test again * WIP * fixing watchconditions * separate selected key value collections from feature flag collections, separate selectors, add new methods to support new logic * comment and naming updates * fixing unit tests, namespace of defining/calling code needs to be same * fixing tests using AsPages * fix tests with pageablemanager * format * fix tests * fix tests * remove unused extension test class * fix comment, capitalization * check etag on 200, fix tests * add registerall test, fix refresh tests * fix condition for pages and old match conditions * WIP fixing PR comments, tests * check status after advancing existing etag enumerator * move around refresh logic * null check page etag, revert break to existing keys check in getrefreshedcollections * fix loadselected, replace selectedkvwatchers with registerall refresh time * fix comment in options * clean up tests * PR comments * PR comments * don't allow both registerall and register * fix check for calls to both register methods * PR comments for rename/small changes * fix compile error * simplify refreshasync path, fix naming from comments * remove redundant if check * simplify logic for minrefreshinterval * fix smaller comments * call loadselected when refreshing collection, separate data for individual refresh * in progress change to registerall include ff * fix load order * fix comments, rename logging constants to match new behavior * pr comments, refactor refreshasync * clean up etags dictionary creation * PR comments * add uncommitted changes to testhelper * update tests for registerall with feature flags, check ff keys to remove flags on refresh * PR comments * PR comments * use invalidoperationexception in configurerefresh, update loggingconstants to match behavior * remove unused changes

* Adding allocation id * serialize with sorted keys * use string empty * nit * rename ff id to TelemetryVariantPercentile * add more values * dotnet format * Version bump * Add `RegisterAll` API to enable monitoring collections of key-values for refresh (#574) * WIP * WIP testing out client extensions methods * WIP added selectors to multikeywatchers * remove unused property * WIP check for registerall changes to change refreshall * WIP * WIP fixing types and reslving errors * WIP fixing client extensions class * WIP * WIP update feature flag logic * WIP client extensions * WIP reload all flags on change * WIP * WIP fixing tests to return response for getconfigurationsettingsasync * WIP etag for tests * fix watchedcollections null * WIP tests, working for examples * remove unused variables * update to newest sdk version, remove unused * WIP fixing tests * WIP reworking testing to work with new etag approach * tests passing, fix mockasyncpageable * update sdk package version * fix loghelper, tests * WIP fixing aspages tests * revert watchesfeatureflags test * update test again * WIP * fixing watchconditions * separate selected key value collections from feature flag collections, separate selectors, add new methods to support new logic * comment and naming updates * fixing unit tests, namespace of defining/calling code needs to be same * fixing tests using AsPages * fix tests with pageablemanager * format * fix tests * fix tests * remove unused extension test class * fix comment, capitalization * check etag on 200, fix tests * add registerall test, fix refresh tests * fix condition for pages and old match conditions * WIP fixing PR comments, tests * check status after advancing existing etag enumerator * move around refresh logic * null check page etag, revert break to existing keys check in getrefreshedcollections * fix loadselected, replace selectedkvwatchers with registerall refresh time * fix comment in options * clean up tests * PR comments * PR comments * don't allow both registerall and register * fix check for calls to both register methods * PR comments for rename/small changes * fix compile error * simplify refreshasync path, fix naming from comments * remove redundant if check * simplify logic for minrefreshinterval * fix smaller comments * call loadselected when refreshing collection, separate data for individual refresh * in progress change to registerall include ff * fix load order * fix comments, rename logging constants to match new behavior * pr comments, refactor refreshasync * clean up etags dictionary creation * PR comments * add uncommitted changes to testhelper * update tests for registerall with feature flags, check ff keys to remove flags on refresh * PR comments * PR comments * use invalidoperationexception in configurerefresh, update loggingconstants to match behavior * remove unused changes * Give the users the ability to have control over ConfigurationClient instance(s) used by the provider (#598) (#617) * Introduced a new `AzureAppConfigurationClientFactory` class to handle the creation of `ConfigurationClient` instances * remove clients dictionary since we will not have hits and clients are already stored in ConfigurationClientManager * revert * add license + remove unused usings * ran dotnet format * add capability of fallback to different stores * add explicit type * address comments * remove scheme validation --------- Co-authored-by: Sami Sadfa <71456174+samsadsam@users.noreply.github.com> Co-authored-by: Sami Sadfa <samisadfa@microsoft.com> * first draft tag filtering support * add alternate APIs * change to use ienumerable * update featureflagoptions to match main options * update keyvalueselector equals and hashcode * update param comments for selects * fix merge conflict errors * add validation for tagsfilter param, add to comment * edit error message for format * edit comment * add unit tests * remove unused file * revert versions * update tests to include feature flag select * add refresh test * ff only refresh test * update equals for selector * fix equals * update equals * reorder properties in keyvalueselector * upgrade to 8.2.0-preview (#638) * fix incorrect test * fix equals for selector * update gethashcode for keyvalueselector * PR comments, in progress * update tests from PR comments * add validation for number of tags, add test * rename tagsFilter to tagsFilters everywhere * fix usings, missing updates to ffoptions * update ffoptions select again * fix tests * update sdk version * update tagsfilters to tagfilters * remove tagsfilters again * PR comments * PR comments * Revert "Merge pull request #600 from Azure/rossgrambo/allocation_id" This reverts commit 51d4ad7, reversing changes made to d551536. * Revert "Give the users the ability to have control over ConfigurationClient instance(s) used by the provider (#598) (#617)" This reverts commit 6dc9ae2. --------- Co-authored-by: Ross Grambo <rossgrambo@microsoft.com> Co-authored-by: Sami Sadfa <samisadfa@microsoft.com> Co-authored-by: Sami Sadfa <71456174+samsadsam@users.noreply.github.com> Co-authored-by: Sami Sadfa <sami.sadfa2@gmail.com>

amerjusupovic added 30 commits April 3, 2024 11:37

WIP

a6e3300

WIP testing out client extensions methods

15c53fe

WIP added selectors to multikeywatchers

75a01eb

remove unused property

f1b07b4

WIP check for registerall changes to change refreshall

25c262c

WIP

302140b

WIP fixing types and reslving errors

cc1bc58

WIP fixing client extensions class

28ecc05

WIP

38ba768

Merge branch 'preview' of https://github.com/Azure/AppConfiguration-D…

67ec320

…otnetProvider into ajusupovic/collection-monitoring

WIP update feature flag logic

9f4c1de

WIP client extensions

937e012

WIP reload all flags on change

7e17870

WIP

f4c17ae

WIP fixing tests to return response for getconfigurationsettingsasync

65de1cd

WIP etag for tests

25c477b

fix watchedcollections null

913587e

WIP tests, working for examples

5f84295

remove unused variables

e18a835

update to newest sdk version, remove unused

d41499e

Merge branch 'preview' of https://github.com/Azure/AppConfiguration-D…

85d9909

…otnetProvider into ajusupovic/collection-monitoring

WIP fixing tests

d6ac25b

WIP reworking testing to work with new etag approach

5410b5d

tests passing, fix mockasyncpageable

fc752d3

update sdk package version

8b02c76

fix loghelper, tests

3ebf1e4

WIP fixing aspages tests

26d3391

revert watchesfeatureflags test

aa037b6

update test again

415c577

WIP

57f5b80

jimmyca15 reviewed Jan 15, 2025

View reviewed changes

amerjusupovic added 3 commits January 15, 2025 12:15

PR comments

9135d42

add uncommitted changes to testhelper

c55aa60

update tests for registerall with feature flags, check ff keys to rem…

0b6aecb

…ove flags on refresh

samsadsam reviewed Jan 17, 2025

View reviewed changes

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs Outdated Show resolved Hide resolved

samsadsam reviewed Jan 17, 2025

View reviewed changes

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs Outdated Show resolved Hide resolved

samsadsam reviewed Jan 17, 2025

View reviewed changes

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs Outdated Show resolved Hide resolved

samsadsam reviewed Jan 17, 2025

View reviewed changes

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs Outdated Show resolved Hide resolved

PR comments

8b3f49a

jimmyca15 reviewed Jan 18, 2025

View reviewed changes

PR comments

00ac014

jimmyca15 reviewed Jan 21, 2025

View reviewed changes

jimmyca15 approved these changes Jan 21, 2025

View reviewed changes

samsadsam approved these changes Jan 21, 2025

View reviewed changes

use invalidoperationexception in configurerefresh, update loggingcons…

d318127

…tants to match behavior

jimmyca15 approved these changes Jan 21, 2025

View reviewed changes

avanigupta approved these changes Jan 22, 2025

View reviewed changes

remove unused changes

396fe54

avanigupta approved these changes Jan 22, 2025

View reviewed changes

amerjusupovic merged commit 5e6a012 into preview Jan 22, 2025
3 checks passed

amerjusupovic deleted the ajusupovic/collection-monitoring branch January 22, 2025 20:51

amerjusupovic added a commit that referenced this pull request Feb 5, 2025

Revert "Add RegisterAll API to enable monitoring collections of key…

9862bd8

…-values for refresh (#574)" This reverts commit 5e6a012.

		@@ -22,12 +22,14 @@ internal class LoggingConstants
		public const string RefreshKeyVaultSecretRead = "Secret read from Key Vault for key-value.";
		public const string RefreshFeatureFlagRead = "Feature flag read from App Configuration.";

Add RegisterAll API to enable monitoring collections of key-values for refresh #574

Add RegisterAll API to enable monitoring collections of key-values for refresh #574

Uh oh!

Conversation

amerjusupovic commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Example usage

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amerjusupovic Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jimmyca15 commented Jan 30, 2025

Uh oh!

Uh oh!

Add `RegisterAll` API to enable monitoring collections of key-values for refresh #574

Add `RegisterAll` API to enable monitoring collections of key-values for refresh #574

amerjusupovic commented Jul 15, 2024 •

edited

Loading

amerjusupovic Jan 16, 2025 •

edited

Loading