Refactor RuntimeConfig into Interface with Local and Hosted Providers #1981

aaronburtle · 2024-01-18T11:51:37Z

Why make this change?

Related to Support hot reloading of configuration #67
Closes to Refactor RuntimeConfigProvider to be more extensible for Hot Reloading #1966
Closes to Change default data source name for Hot Reload #1967

In order to better facilitate Hot Reload, we need to split the responsibility for loading and accessing the RuntimeConfig between the two different scenarios that our service can run within, local and hosted. We already have a FileSystemRuntimeConfigLoader which handles the loading of the config from a local file system. This refactor naturally extends that approach so that we can Hot Reload both in the local and eventually, the hosted scenario.

We also make some necessary changes for hot reloading to work within the service.

What is this change?

As a part of the changes, we split the RuntimeConfigProvider into a LocalRuntimeConfigProvider and a HostedRuntimeConfigProvider. These classes then implement the IRuntimeConfigProvider interface in the way that makes sense for their scenario.

Rather than having these providers storing the internal copy of the RuntimeConfig, the natural approach is for the loaders to be responsible for maintaining this internal representation, and for the loaders to handle all of the loading, including any hot reloading that takes place for that RuntimeConfig.

This was previously not possible, because we could not store a copy of the RuntimeConfig with the loader due to circular dependency. This refactor removes that issue as part of its changes. We then move the internally stored RuntimeConfig to these loaders, as well as all of the logic for Hot Reloading. Since we are only handling Hot Reloading in the local scenario to being, we only have the loader for the local scenario currently setup to do this. When we extend Hot Reloading to the hosted scenario, that will mean adding a loader that can handle that specific logic in a hosted scenario.

We also change the way that the default data source names are generated so that this is deterministic. This is a requirement to pass validation during time of handling requests. In this case we are taking a hash of the connection string, though this means to hot reload the connection string we will need a new approach.

Within the tests, there was an attempt to use the LocalRuntimeConfigProvider when possible, since testing is naturally happening in a local scenario. However, some tests utilize functions that are only relevant for the HostedRuntimeConfigProvider and so in those cases, we used that class. This created some issues, and workarounds were found for these tests.

How was this tested?

Current test suite.

Sample Request(s)

Any valid request.

…mments

seantleonard · 2024-01-22T21:23:33Z

src/Core/Configurations/IRuntimeConfigProvider.cs

+        /// </summary>
+        public RuntimeConfigLoader ConfigLoader { get; set; }
+
+        public delegate Task<bool> RuntimeConfigLoadedHandler(IRuntimeConfigProvider sender, RuntimeConfig config);


add comment

seantleonard · 2024-01-22T21:24:51Z

src/Core/Configurations/IRuntimeConfigProvider.cs

+
+        public delegate Task<bool> RuntimeConfigLoadedHandler(IRuntimeConfigProvider sender, RuntimeConfig config);
+
+        public List<RuntimeConfigLoadedHandler> RuntimeConfigLoadedHandlers { get; }


need comment to explain what the handlers are for and why a list of handlers is needed

seantleonard · 2024-01-22T21:25:40Z

src/Core/Configurations/IRuntimeConfigProvider.cs

+        /// The access tokens representing a Managed Identity to connect to the database.
+        /// The key is the unique datasource name and the value is the access token.
+        /// </summary>
+        public Dictionary<string, string?> ManagedIdentityAccessToken { get; set; }


as you were refactoring, did you identify whether there may be a more appropriate place for ManagedIdentityAccessToken and TrySetAccesstoken as they don't seem appropriate to be in this interface.

seantleonard · 2024-01-22T21:51:26Z

src/Config/ObjectModel/RuntimeConfig.cs

@@ -169,7 +169,7 @@ public RuntimeConfig(string? Schema, DataSource DataSource, RuntimeEntities Enti
        this.DataSource = DataSource;
        this.Runtime = Runtime;
        this.Entities = Entities;
-        _defaultDataSourceName = Guid.NewGuid().ToString();
+        _defaultDataSourceName = DataSource.ConnectionString.GetHashCode().ToString();


Do you have in your pr description or here as a comment why hash code? vs guid?

The PR description says This is a requirement to pass validation during time of handling requests.. What kind of validation are we talking here? Could you please elaborate?

Also, will this still be needed if we now have mutable records? Can we retain the originally generated Guid and modify only parts that changed in the hot reload?

we can pass the Guid along through the hot reload function and then overwrite the RuntimeConfig's default data source name with that passed along Guid if it exists. I updated this, so we can avoid using the connection string's hash.

seantleonard · 2024-01-22T23:06:15Z

src/Service.Tests/Unittests/MySqlQueryExecutorUnitTests.cs

@@ -54,7 +54,10 @@ public class MySqlQueryExecutorUnitTests
            MockFileSystem fileSystem = new();
            fileSystem.AddFile(FileSystemRuntimeConfigLoader.DEFAULT_CONFIG_FILE_NAME, new MockFileData(mockConfig.ToJson()));
            FileSystemRuntimeConfigLoader loader = new(fileSystem);
-            RuntimeConfigProvider provider = new(loader);
+            HostedRuntimeConfigProvider provider = new(loader)


How are you determining whether to use Local/Hosted RuntimeConfigProvider?

seantleonard · 2024-01-22T23:20:41Z

src/Core/Configurations/HostedRuntimeConfigProvider.cs


-    private RuntimeConfig? _runtimeConfig;
+    public RuntimeConfig? _runtimeConfig;


why make change this field from private to public? we already have accessor methods like TryGetConfig TryGetLoaded config etc. which provide access to the runtime config.

There were tests that in order to make work with the new refactor need these to be public for the mocking that was required. I could not find a straight forward way to get those tests to work without making this public, but perhaps we could sync on this.

seantleonard · 2024-01-22T23:22:05Z

src/Core/Configurations/HostedRuntimeConfigProvider.cs

+/// This class is responsible for exposing the runtime config to the rest of the service when in a hosted scenario.
+/// The <c>HostedRuntimeConfigProvider</c> won't directly load the config, but will instead rely on the <see cref="FileSystemRuntimeConfigLoader"/> to do so.


This description should explain what is unique about this runtimeconfigprovider. Why/how does it enable DAB to function appropriately in a hosted environment?

seantleonard · 2024-01-22T23:23:52Z

src/Core/Configurations/LocalRuntimeConfigProvider.cs

+/// This class should be treated as the sole accessor of the config that is available within the service, and other classes
+/// should not access the config directly, or maintain a reference to it, so that we can do hot-reloading by replacing


By 'this class' do you mean both Local/HostedRuntimeConfigProvider classes? if so, would need to clarify the comment

seantleonard · 2024-01-22T23:50:55Z

src/Core/Configurations/LocalRuntimeConfigProvider.cs

+/// </remarks>
+public class LocalRuntimeConfigProvider : IRuntimeConfigProvider
+{
+    public bool IsLateConfigured { get; set; }


Should this just return false?

src/Config/FileSystemRuntimeConfigLoader.cs

ayush3797 · 2024-01-24T09:17:56Z

src/Core/Configurations/LocalRuntimeConfigProvider.cs

+        if (ConfigLoader.RuntimeConfig is null)
+        {
+            ConfigLoader.TryLoadKnownConfig(out _, replaceEnvVar: true);
+


nit: remove extra line

src/Config/ConfigFileWatcher.cs

src/Config/FileSystemRuntimeConfigLoader.cs

ayush3797 · 2024-01-24T10:07:01Z

src/Config/FileSystemRuntimeConfigLoader.cs

@@ -104,7 +132,10 @@ public string GetConfigFileName()
        {
            Console.WriteLine($"Loading config file from {path}.");
            string json = _fileSystem.File.ReadAllText(path);
-            return TryParseConfig(json, out config, connectionString: _connectionString, replaceEnvVar: replaceEnvVar);
+            bool res = TryParseConfig(json, out RuntimeConfig, connectionString: _connectionString, replaceEnvVar: replaceEnvVar);


Not something changed in this PR: In the TryParseConfig() method, the order of parameters in the summary is messed up. Could you please fix that?

The function below? Seems correct to me.

perhaps Ayush meant that most other methods in our project have the out parameter as the last parameter in the method signature.

Aniruddh25

Posting comments so far

Aniruddh25 · 2024-01-25T05:19:51Z

src/Config/ObjectModel/RuntimeConfig.cs

@@ -169,7 +169,7 @@ public RuntimeConfig(string? Schema, DataSource DataSource, RuntimeEntities Enti
        this.DataSource = DataSource;
        this.Runtime = Runtime;
        this.Entities = Entities;
-        _defaultDataSourceName = Guid.NewGuid().ToString();
+        _defaultDataSourceName = DataSource.ConnectionString.GetHashCode().ToString();


The PR description says This is a requirement to pass validation during time of handling requests.. What kind of validation are we talking here? Could you please elaborate?

Also, will this still be needed if we now have mutable records? Can we retain the originally generated Guid and modify only parts that changed in the hot reload?

src/Config/ConfigFileWatcher.cs

Aniruddh25 · 2024-01-25T05:35:19Z

src/Config/RuntimeConfigLoader.cs

@@ -22,6 +22,8 @@ public abstract class RuntimeConfigLoader
 {
    protected readonly string? _connectionString;

+    public RuntimeConfig? RuntimeConfig;


nit: did you mean to add this as a property or a field?

src/Config/FileSystemRuntimeConfigLoader.cs

Aniruddh25 · 2024-01-25T05:42:37Z

src/Core/Configurations/IRuntimeConfigProvider.cs

+        /// As opposed to using a json input and regenerating the runtimconfig, it sets the access token for the current runtimeConfig on the provider.
+        /// </summary>
+        public bool TrySetAccesstoken(
+        string? accessToken,


Do we need all kinds of RuntimeConfigProvider to implement the method to TrySetAccessToken?

Suggested change

string? accessToken,

string? accessToken,

Aniruddh25

Good starter, but needs clarity on when would HostedRuntimeConfigProvider will be created so that previous scenarios are not broken.

Aniruddh25 · 2024-01-25T22:54:54Z

src/Core/Configurations/HostedRuntimeConfigProvider.cs


-    public RuntimeConfigProvider(RuntimeConfigLoader runtimeConfigLoader)
+    public HostedRuntimeConfigProvider(RuntimeConfigLoader runtimeConfigLoader)
    {
        ConfigLoader = runtimeConfigLoader;


What is the runtimeConfigLoader in case of HostedRuntimeConfigProvider?

It is used for testing because our tests are using the initialize function, which is for the hosted scenario. I am looking into how to refactor the tests to eliminate this, but the answer is not obvious.

Aniruddh25 · 2024-01-25T22:55:36Z

src/Core/Configurations/HostedRuntimeConfigProvider.cs

@@ -278,7 +197,7 @@ public void HotReloadConfig()
        return false;
    }

-    private async Task<bool> InvokeConfigLoadedHandlersAsync()
+    public async Task<bool> InvokeConfigLoadedHandlersAsync()


why do we need this function in HostedRuntimeConfigProvider?

Aniruddh25 · 2024-01-26T01:45:49Z

src/Service/Startup.cs

@@ -79,7 +79,7 @@ public void ConfigureServices(IServiceCollection services)
                null);
            IFileSystem fileSystem = new FileSystem();
            FileSystemRuntimeConfigLoader configLoader = new(fileSystem, configFileName, connectionString);
-            RuntimeConfigProvider configProvider = new(configLoader);
+            IRuntimeConfigProvider configProvider = new LocalRuntimeConfigProvider(configLoader);

            services.AddSingleton(fileSystem);
            services.AddSingleton(configProvider);


where is the hosted runtime config provider added to the set of services? If its not added, how will our late configured scenarios work?

Aniruddh25 · 2024-01-26T01:49:14Z

src/Service/Startup.cs

@@ -403,7 +403,7 @@ public static ILoggerFactory CreateLoggerFactoryForHostedAndNonHostedScenario(IS
                // attempt to get the runtime config to determine the loglevel based on host.mode.
                // If runtime config is available, set the loglevel to Error if host.mode is Production,
                // Debug if it is Development.
-                RuntimeConfigProvider configProvider = serviceProvider.GetRequiredService<RuntimeConfigProvider>();
+                IRuntimeConfigProvider configProvider = serviceProvider.GetRequiredService<IRuntimeConfigProvider>();
                if (configProvider.TryGetConfig(out RuntimeConfig? runtimeConfig))


For hosted scenarios, the configProvider thats picked up here, should be HostedRuntimeConfigProvider. But the implementation of TryGetConfig is not provided for that provider - so this will throw an exception. We should implement TryGetConfig even for HostedRuntimeConfigProvider

Aniruddh25 · 2024-01-26T01:55:45Z

src/Service.Tests/Configuration/ConfigurationTests.cs

@@ -917,7 +917,7 @@ public async Task TestSettingConfigurationCreatesCorrectClasses(string configura
            Assert.AreEqual(HttpStatusCode.OK, postResult.StatusCode);

            ValidateCosmosDbSetup(server);
-            RuntimeConfigProvider configProvider = server.Services.GetService<RuntimeConfigProvider>();
+            IRuntimeConfigProvider configProvider = server.Services.GetService<IRuntimeConfigProvider>();

            Assert.IsNotNull(configProvider, "Configuration Provider shouldn't be null after setting the configuration at runtime.");
            Assert.IsTrue(configProvider.TryGetConfig(out RuntimeConfig configuration), "TryGetConfig should return true when the config is set.");


This assert should fail since the configProvider in this test should result in hosted config provider that doesnt have the TryGetConfig method implemented

src/Config/FileSystemRuntimeConfigLoader.cs

seantleonard · 2024-02-09T18:08:32Z

src/Config/ObjectModel/RuntimeConfig.cs

@@ -169,18 +169,18 @@ public RuntimeConfig(string? Schema, DataSource DataSource, RuntimeEntities Enti
        this.DataSource = DataSource;
        this.Runtime = Runtime;
        this.Entities = Entities;
-        _defaultDataSourceName = Guid.NewGuid().ToString();
+        DefaultDataSourceName = Guid.NewGuid().ToString(); //DataSource.ConnectionString.GetHashCode().ToString();


commented code

also seems inconsistent where all other members are set using this.Member = Parameter

seantleonard · 2024-02-09T18:09:28Z

src/Config/ObjectModel/RuntimeConfig.cs

@@ -130,7 +130,7 @@ public bool AllowIntrospection
        }
    }

-    private string _defaultDataSourceName;
+    public string DefaultDataSourceName;


This seems to be only public member not following getter setter syntax. { get; set; } any reason not to?

seantleonard · 2024-02-09T18:18:57Z

src/Config/ConfigFileWatcher.cs

            {
-                _configProvider.HotReloadConfig();
+                _configLoader.HotReloadConfig(_configLoader.RuntimeConfig.DefaultDataSourceName);


may be helpful to print/log that hot reloading was skipped because config definded production mode.

seantleonard · 2024-02-09T18:19:30Z

src/Config/ConfigFileWatcher.cs

@@ -31,14 +38,19 @@ private void OnConfigFileChange(object sender, FileSystemEventArgs e)
    {
        try
        {
-            if (_configProvider is null)
+            if (_configLoader is null)


summary comment needs completion for params.

also nit in the summary: this trigger function -> this event handler

seantleonard · 2024-02-09T18:32:14Z

src/Config/FileSystemRuntimeConfigLoader.cs

+            bool configParsed = TryParseConfig(json, out RuntimeConfig, connectionString: _connectionString, replaceEnvVar: replaceEnvVar);
+            TrySetupConfigFileWatcher();
+            config = RuntimeConfig;


Can you clarify these few lines here?

Why would we still attempt to execute TrySetupConfigFileWatcher() if configParsed is false indicating some failure?

Also, the line config=Runtimeconfig wouldn't work or shouldn't occur if configParsed is false.

seantleonard · 2024-02-09T18:34:34Z

src/Config/FileSystemRuntimeConfigLoader.cs

+    /// Hot Reloads the runtime config when the file watcher
+    /// is active and detects a change to the underlying config file.
+    /// </summary>
+    public void HotReloadConfig(string defaultDataSourceName)


why is the param named defaultDataSourceName? what happens when the value is not from the default data source and, instead, from a different data source?

seantleonard · 2024-02-09T18:35:57Z

src/Config/RuntimeConfigLoader.cs

@@ -22,6 +22,8 @@ public abstract class RuntimeConfigLoader
 {
    protected readonly string? _connectionString;

+    public RuntimeConfig? RuntimeConfig { get; set; }


should this class be renamed to RuntimeConfigLoaderBase?

seantleonard · 2024-02-09T18:48:25Z

src/Core/Configurations/HostedRuntimeConfigProvider.cs

-    /// </summary>
-    /// <param name="runtimeConfig">Populated runtime configuration, if present.</param>
-    /// <returns>True when runtime config is provided, otherwise false.</returns>
+    // Not currently used in hosted scenario, but needed for interface


comments need to explain why:
TryGetConfig([NotNullWhen(true)] out RuntimeConfig? runtimeConfig)
public bool TryGetLoadedConfig([NotNullWhen(true)] out RuntimeConfig? runtimeConfig)

are not used in hosted scenario. It doesn't make sense to me that these two methods are promoted to the interface if they aren't used in implementating classes.

seantleonard · 2024-02-09T18:48:39Z

src/Core/Configurations/HostedRuntimeConfigProvider.cs

+    //public async Task<bool> InvokeConfigLoadedHandlersAsync()
+    //{
+    //    List<Task<bool>> configLoadedTasks = new();
+    //    if (_runtimeConfig is not null)
+    //    {
+    //        foreach (RuntimeConfigLoadedHandler configLoadedHandler in RuntimeConfigLoadedHandlers)
+    //        {
+    //            configLoadedTasks.Add(configLoadedHandler(this, _runtimeConfig));
+    //        }
+    //    }
+
+    //    bool[] results = await Task.WhenAll(configLoadedTasks);
+
+    //    // Verify that all tasks succeeded.
+    //    return results.All(x => x);
+    //}


commented code.

aaronburtle added 4 commits January 10, 2024 04:22

initial refactor, fix errors, hashcode for config ID

311e6eb

fix an error

662178f

fix error

489d16c

additional refactoring, fix build errors, fix test errors, cleanup co…

7d03b8e

…mments

aaronburtle requested review from mbhaskar, Aniruddh25, JelteF, seantleonard, Mathos1432, gledis69, milismsft, tarazou9, ayush3797, abhishekkumams, aaronpowell, severussundar and ravishetye as code owners January 18, 2024 11:51

aaronburtle self-assigned this Jan 18, 2024

aaronburtle added the refactor label Jan 18, 2024

aaronburtle added this to the 0.11rc milestone Jan 18, 2024

seantleonard requested changes Jan 23, 2024

View reviewed changes

seantleonard assigned seantleonard and ayush3797 and unassigned aaronburtle Jan 24, 2024

ayush3797 reviewed Jan 24, 2024

View reviewed changes

src/Config/ConfigFileWatcher.cs Show resolved Hide resolved

ayush3797 reviewed Jan 24, 2024

View reviewed changes

src/Config/FileSystemRuntimeConfigLoader.cs Outdated Show resolved Hide resolved

ayush3797 reviewed Jan 24, 2024

View reviewed changes

src/Config/FileSystemRuntimeConfigLoader.cs Show resolved Hide resolved

ayush3797 reviewed Jan 24, 2024

View reviewed changes

aaronburtle linked an issue Jan 24, 2024 that may be closed by this pull request

Refactor RuntimeConfigProvider to be more extensible for Hot Reloading #1966

Open

aaronburtle linked an issue Jan 24, 2024 that may be closed by this pull request

Change default data source name for Hot Reload #1967

Closed

aaronburtle mentioned this pull request Jan 24, 2024

Support hot reloading of configuration #67

Open

Aniruddh25 requested changes Jan 25, 2024

View reviewed changes

Aniruddh25 requested changes Jan 26, 2024

View reviewed changes

ayush3797 reviewed Jan 29, 2024

View reviewed changes

src/Config/FileSystemRuntimeConfigLoader.cs Show resolved Hide resolved

addressing comments

72ee378

seantleonard requested changes Feb 9, 2024

View reviewed changes

Aniruddh25 modified the milestones: 0.11rc, 0.12rc Mar 7, 2024

seantleonard modified the milestones: 0.12rc, 1.2rc Mar 12, 2024

seantleonard added the do-not-merge label Mar 12, 2024


		public delegate Task<bool> RuntimeConfigLoadedHandler(IRuntimeConfigProvider sender, RuntimeConfig config);

		public List<RuntimeConfigLoadedHandler> RuntimeConfigLoadedHandlers { get; }


		private RuntimeConfig? _runtimeConfig;
		public RuntimeConfig? _runtimeConfig;

		/// This class is responsible for exposing the runtime config to the rest of the service when in a hosted scenario.
		/// The <c>HostedRuntimeConfigProvider</c> won't directly load the config, but will instead rely on the <see cref="FileSystemRuntimeConfigLoader"/> to do so.

		/// This class should be treated as the sole accessor of the config that is available within the service, and other classes
		/// should not access the config directly, or maintain a reference to it, so that we can do hot-reloading by replacing

Refactor RuntimeConfig into Interface with Local and Hosted Providers #1981

Are you sure you want to change the base?

Refactor RuntimeConfig into Interface with Local and Hosted Providers #1981

Conversation

aaronburtle commented Jan 18, 2024 • edited by Aniruddh25

Why make this change?

What is this change?

How was this tested?

Sample Request(s)

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

ayush3797 Jan 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aniruddh25 left a 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

Aniruddh25 left a 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

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

aaronburtle commented Jan 18, 2024 •

edited by Aniruddh25

ayush3797 Jan 24, 2024 •

edited