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, but it 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.

…otnetProvider into ajusupovic/collection-monitoring

jimmyca15 · 2025-01-15T01:10:10Z

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

+        /// <summary>
+        /// For use in tests only. An optional class used to process pageable results from Azure App Configuration.
+        /// </summary>
+        internal IPageableConfigurationSettings PageableConfigurationSettings { get; set; }


I suggested this name (IPageableConfigurationSettings), and I think it was a bad suggestion. Because this is not configuration settings, it is indeed something that iterates through pages. The original name IConfigurationSettingsPageIterator would be better, or something along those lines.

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

jimmyca15 · 2025-01-15T18:23:02Z

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

+            bool hasWatchers = watchers.Any();
+            TimeSpan minWatcherRefreshInterval = hasWatchers ? watchers.Min(w => w.RefreshInterval) : TimeSpan.MaxValue;
+
+            if (options.RegisterAllEnabled)


The embedded ternary condition in the else statement makes more sense to be a part of the upper if/else.

if (registerAllEnabled) { } else if (watchers.Any()) { } else { }

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

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.

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

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

jimmyca15 reviewed Jan 15, 2025

View reviewed changes

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

jimmyca15 Jan 15, 2025 •

edited

Loading

jimmyca15 Jan 15, 2025

jimmyca15 Jan 15, 2025

amerjusupovic Jan 16, 2025 •

edited

Loading

jimmyca15 Jan 18, 2025

amerjusupovic Jan 21, 2025

jimmyca15 Jan 18, 2025

amerjusupovic Jan 21, 2025

jimmyca15 Jan 21, 2025

amerjusupovic Jan 21, 2025

jimmyca15 Jan 18, 2025

jimmyca15 Jan 18, 2025

jimmyca15 Jan 18, 2025

amerjusupovic Jan 21, 2025

jimmyca15 Jan 21, 2025

jimmyca15 Jan 21, 2025

avanigupta Jan 22, 2025

		@@ -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

Conversation

amerjusupovic commented Jul 15, 2024 • edited Loading

jimmyca15 Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amerjusupovic Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

jimmyca15 Jan 15, 2025 •

edited

Loading

amerjusupovic Jan 16, 2025 •

edited

Loading