feat: Bootstrap common `EppoClient` #251

typotter · 2025-03-17T19:39:09Z

labels: mergeable

Towards FF-4076
Towards FF-4147

Motivation and Context

Within the EppoClient the configuration is loaded and written to active memory by the ConfigurationRequestor, however that object is only present in the context of a fetching client. In order to see bootstrapped configuration in the common EppoClient, we need to be able to make reads and writes to the configuration outside of the scope of ConfigurationRequestor. The format of the bootstrap configuration is a higher order object containing the responses normally received by the ConfigurationRequestor to populate the configuration (IUniversalConfigResponse and IBanditParametersResponse). Thus both the ConfigurationRequestor and the bootstrap routine require building a configuration from these responses, a shared functionality.

Additionally, the complexity of actually storing the configuration in the three IConfigurationStore instances should really not be the concern of the ConfigRequestor or the bootstrap routine, as we may (hopefully) change out the architecture of the configuration stores in the future. Recent refactoring work has unified Configuration access, and this work follows suit to extract one more useful layer.

Finally, the architecture of the common EppoClient and its derivatives require that the underlying IConfigurationStore instances be swapped out at basically any time (there are public setters for these stores). This is the final loose end towards supporting onConfigurationChange.

Description

IConfigurationManager: allows setting the Configuration based on UFC responses, and changing out the IConfigurationStoreinstances used to "back" the configuration. We let theEppoClient` maintain the sources of truth for the config store instances as we to set all three (or the flag store + undefined) all at once.
ConfigurationManager code pulled from ConfigurationRequestor concerning config store hydration and populating from UFC. Also method to set the ConfigurationStores.
ConfigurationRequestor trimmed down to just the optimized loading logic, delegating storing of the configuration to the ConfigurationManager ** internal API change:** ConfigurationRequestor.constructor
Modifed EppoClient set*Store methods to update the Configuration stores on the ConfigurationManager. This actually fixes a bug whereby the config store could be set after initialization but it wouldn't take effect until fetchFlagConfigurations was called.
New EppoClient method: setConfigurationStores to encourage setting all of the stores at once and deprecated setStore methods
Added EppoClient.configurationManager, created on instantiation, so it is always available to return a Configuration so simplified EppoClient.getConfiguration in turn
bootstrap method added to Eppo Client.

How has this been tested?

Updates to the existing tests to reflect the refactors responsibility (from ConfigurationRequestor to ConfigurationManager)
The configuration is a critical and sensitive part of the EppoClient stack, which is nearly fully exercised by our unit test suite which continues to pass
Test suites to run the shared test cases against the bootstrap method

Makefile

aarsilv · 2025-03-18T15:11:02Z

src/client/eppo-client-with-bandits.spec.ts

@@ -130,6 +134,35 @@ describe('EppoClient Bandits E2E test', () => {
      });
      // Ensure that this test case correctly checked some test assignments
      expect(numAssignmentsChecked).toBeGreaterThan(0);
+    }
+
+    describe('bootstrapped client', () => {


🔥 LOVE that you did this 💪

src/client/eppo-client-with-bandits.spec.ts

aarsilv · 2025-03-18T15:13:27Z

src/client/eppo-client.ts

  // noinspection JSUnusedGlobalSymbols
+  /**
+   * @deprecated use `setConfigurationStores` instead


This would make us lose the ability to update just one of the stores at one time. Is this something we're ok losing? I imagine so.

After some thought, this collecting into a single method may not be necessary. I think we need to take another look at the config stores anyway so I'll avoid changes here for now.

src/client/eppo-client.spec.ts

aarsilv · 2025-03-18T15:16:17Z

src/client/eppo-client.ts

+      : undefined;
+
+    // We need to run this method sync, but, because the configuration stores potentially have an async write at the end
+    // of updating the configuration, the method to do so it also async. Use an IIFE to wrap the async call.


🤨
Why does this need to be run synchronously?

(fixed the comment)

It doesn't. I gave up trying to force bootstrap to be sync. The js-client offlineInit method is sync (in fact, the options type for that method is IClientConfigSync), so I need to understand a little more why that method is so explicitly sync.

We can also just fire and forget the async method to keep bootstrap sync, but that hanging promise feels awkward (and indeed eslint dislikes it)

We can also just fire and forget the async method to keep bootstrap sync, but that hanging promise feels awkward (and indeed eslint dislikes it)

I don't think that's an issue as nobody has a reason to await it, so letting it hang makes a lot of sense

src/configuration-store/configuration-manager.spec.ts

aarsilv · 2025-03-18T15:17:44Z

src/i-configuration.ts

@@ -63,6 +63,8 @@ export class StoreBackedConfiguration implements IConfiguration {
      );
    }
    await Promise.all(promises);
+
+    // TODO: notify of config change if `didUpdateFlags` is true


This PR or a future one?

Future one (this TODO has been moved around a couple of times now).

All of this config refactoring is leading to this line being called every time the configuration changes. Then we can implement the onConfigurationChange and waitForInitialization methods

rasendubi

This looks good but I'm honestly getting lost in all these abstractions. They to try to abstract something but they seem rather shallow—you have to read the code to understand what each of these things is doing

I feel a bit ranty, so here's my general struggles with js codebase (please don't take it personally—I just hope that you can help me resolve some of these):

Configuration stores don't store configurations but rather arbitrary key-value entries and some random associated bits that don't always make sense (like salt or format). This directly leads to us having dozens of these stores for all bits of data.
Store-backed configuration is mutable, so might be changing under me
Stores mix two concerns: source of truth for currently active configuration and configuration persistence. It would help to untangle the two.
Configuration manager seems rather shallow and I can't name what is it's purpose. Most of method seem like their should either belong to configuration or would belong to a central configuration store

My current thinking around configuration handling (partially implemented in core) is as follows (not saying that we should follow it but I gave it quite a bit of thought, so if it makes sense we can use this thinking across SDKs):

"Configuration" is all pieces of data that client needs for evaluation (flags, bandits, etc.). Note that it's just data—it's passive, it doesn't do anything and doesn't change. If we fetch new flags from the server, that's a new configuration.
"Configuration store" is here to answer "what's the currently active Configuration" and coordinate other components. Conceptually, it's just a global variable with watcher: getConfiguration()/setConfiguration()/onConfigurationChange()—that's it.
Other components interact with the central configuration store and either get configuration and do something with it (e.g., evaluation, persistence) or produce new configurations (configuration poller, get/set configuration api in client)
Persistence is just another component with two features: (1) race to load initial configuration, and (2) on configuration change, persist configuration in background

Kind of like this:

rasendubi · 2025-03-19T15:30:20Z

src/client/eppo-client.ts

+   * accommodate writing to a persistent store. For fastest initialization, (at the cost of persisting configuration),
+   * use `bootstrap` in conjunction with `MemoryOnlyConfigurationStore` instances which won't do an async write.
+   */
+  async bootstrap(configuration: IConfigurationWire): Promise<void> {


We call this method setConfiguration in other SDKs as it's also useful beside the initial configuration (e.g., when user wants to manually control how configuration is refreshed). Would that name work here?

For bootstrap specifically, other SDKs also have initialConfiguration initialization option, so SDK is instantly available after init.

I think setConfiguration works as a method name here.
In the downstream SDKs, we typically have init and offlineInit, but the desire is to move away from "offline" and instead use the term, bootstrap.

rasendubi · 2025-03-19T15:33:18Z

src/client/eppo-client.ts

+   * Initializes the `EppoClient` from the provided configuration. This method is async only to
+   * accommodate writing to a persistent store. For fastest initialization, (at the cost of persisting configuration),
+   * use `bootstrap` in conjunction with `MemoryOnlyConfigurationStore` instances which won't do an async write.


Do we have to wait for persistent storage? (I imagine we can start serving assignment immediately without waiting for storage)

rasendubi · 2025-03-19T15:35:39Z

src/client/eppo-client.ts

-      this.banditVariationConfigurationStore ?? null,
-      this.banditModelConfigurationStore ?? null,
+      this.configurationManager,
+      !!this.banditModelConfigurationStore && !!this.banditVariationConfigurationStore,


nit: as with most boolean flags, it took me a second to figure out what's going on here (I guess it's a flag for whether it need to fetch bandits). Consider having an object with named options instead

rasendubi · 2025-03-19T15:36:23Z

src/client/test-utils.ts

-    null,
-    null,
+    new ConfigurationManager(configurationStore),
+    false,


nit: even worse here—I have no idea what this false means from reading this the code

rasendubi · 2025-03-19T15:39:56Z

src/configuration-store/configuration-manager.ts

+import { IConfigurationStore } from './configuration-store';
+import { IConfigurationManager } from './i-configuration-manager';
+
+export class ConfigurationManager implements IConfigurationManager {


nit: having a separate interface look like an oop cargo cult—I don't think we're going to have multiple implementations of configuration managers?

Suggested change

export class ConfigurationManager implements IConfigurationManager {

export class ConfigurationManager {

tough, but fair. fixed.

rasendubi · 2025-03-19T15:49:35Z

src/configuration-store/configuration-manager.ts

+    this.configuration = new StoreBackedConfiguration(
+      this.flagConfigurationStore,
+      this.banditReferenceConfigurationStore,
+      this.banditConfigurationStore,
+    );


minor: haven't seen the previous PR but this kind of "configuration" is internally mutable and may cause troubles down the line when it changes on user unexpectedly. I like to treat configurations as immutable/constant data as that simplifies many things and avoids surprises (like flags disappearing from under me)

I actually have a PR to produce an immutable config (though a better implementation is forthcoming) each time the getConfiguration method is called. Thus an assignment or evaluation step can run with an unchanging config.

typotter added 4 commits March 14, 2025 13:16

chore: ConfigurationManager

104642b

lint

31c3db9

bootstrap to set initial config then, fire-and-forget initialization

759d038

Config Manager test

3defee3

typotter requested review from aarsilv, leoromanovsky and greghuels March 17, 2025 19:43

typotter added 2 commits March 17, 2025 14:02

unify config store setting and round out bootstrap method

247213c

MOAR tests

5d2b695

typotter requested a review from rasendubi March 17, 2025 21:54

universal tests

e26207b

typotter mentioned this pull request Mar 18, 2025

feat: Bootstrap config files Eppo-exp/sdk-test-data#127

Merged

typotter added 2 commits March 17, 2025 23:00

point at new test data

7d8df74

don't initialize after bootstrap

fc1c4e9

aarsilv reviewed Mar 18, 2025

View reviewed changes

typotter added 3 commits March 19, 2025 09:03

update comment

79aa01c

revert temp branch

102cefa

await bootstrapping

a404826

typotter assigned rasendubi Mar 19, 2025

typotter requested a review from aarsilv March 19, 2025 17:03

typotter assigned aarsilv Mar 19, 2025

rasendubi approved these changes Mar 19, 2025

View reviewed changes

typotter mentioned this pull request Mar 19, 2025

chore:refactor JSON utils, add configurationFromString method for exporting #255

Open

typotter added 3 commits March 19, 2025 11:35

lint and revert setStore changes

f1db0f1

drop superfluous interface

b7882b5

un async bootstrap :)

e256730

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Bootstrap common `EppoClient` #251

feat: Bootstrap common `EppoClient` #251

typotter commented Mar 17, 2025 •

edited

Loading

aarsilv Mar 18, 2025

aarsilv Mar 18, 2025

typotter Mar 19, 2025

aarsilv Mar 18, 2025

typotter Mar 19, 2025 •

edited

Loading

rasendubi Mar 19, 2025

aarsilv Mar 18, 2025

typotter Mar 19, 2025

rasendubi left a comment

rasendubi Mar 19, 2025

typotter Mar 19, 2025

rasendubi Mar 19, 2025

rasendubi Mar 19, 2025

rasendubi Mar 19, 2025

rasendubi Mar 19, 2025

typotter Mar 19, 2025

rasendubi Mar 19, 2025

typotter Mar 19, 2025

	export class ConfigurationManager implements IConfigurationManager {
	export class ConfigurationManager {

feat: Bootstrap common EppoClient #251

Are you sure you want to change the base?

feat: Bootstrap common EppoClient #251

Conversation

typotter commented Mar 17, 2025 • edited Loading

labels: mergeable

Motivation and Context

Description

How has this been tested?

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

typotter Mar 19, 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

rasendubi 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

feat: Bootstrap common `EppoClient` #251

feat: Bootstrap common `EppoClient` #251

typotter commented Mar 17, 2025 •

edited

Loading

typotter Mar 19, 2025 •

edited

Loading