Configuration Management via MonitoringContext #8497

anthony-murphy · 2021-12-08T01:23:35Z

This change adds a simple configuration provider abstraction that can be injected via the loader. The configuration and logger are then available on an object currently called the MonitoringContext. To avoid needing to update all our api, we smuggle the MonitoringContext around via the logger. After this initial change has had time to soak we can slowly move our apis over to leveraging MonitoringContexts all rather than logger.

One are we have explicitly not tackled here is namespaces. this means we've gone with a flat structure or all config key look ups. In the future we may want to expand this introduce some kind of namespacing, but there are concerns we'll need to tackle around naming consistency. We face similar naming consistency issues with our child logger. Ideally whatever strategy we land on can help to solve the issues for both.

related to #5921

…luidFramework-1 into config-provider

…to config-provider

api-report/driver-base.api.md

andre4i · 2021-12-16T20:34:23Z

packages/utils/telemetry-utils/src/config.ts

+
+export function loggerToMonitoringContext<T extends ITelemetryBaseLogger = ITelemetryLogger>(
+    logger: T): MonitoringContext<T> {
+    if(loggerIsMonitoringContext(logger)) {


I think there's some formatting inconsistencies in this file

i'll take a look

Suggested change

if(loggerIsMonitoringContext(logger)) {

if (loggerIsMonitoringContext(logger)) {

packages/utils/telemetry-utils/src/index.ts

andre4i · 2021-12-16T20:37:59Z

packages/utils/telemetry-utils/src/test/config.spec.ts

+        assert.equal(config1.getBoolean("featureEnabled"), false); // from settings1.BreakGlass
+    });
+
+    // #region SettingsProvider


How do you feel about removing this region altogether? I was thinking this would be useful but only as a guideline for how consumers of SettingsProvider are able to use our interfaces but I'm not entirely sure it belongs in the main branch

markfields

Phew I made it through config.ts... I will have to come back to review the rest later (I guess after it's merged).

packages/utils/telemetry-utils/src/config.ts

markfields · 2021-12-17T18:52:10Z

packages/utils/telemetry-utils/src/config.ts

+            },
+        });
+    }
+    return NullConfigProvider;


Do we want an API so a caller could check if the provider is operational v. the Null provider? I guess the only usage I can imagine would be a single log line when getting the provider to know whether config is going to work for that session or not.

markfields · 2021-12-17T20:05:54Z

packages/utils/telemetry-utils/src/config.ts

+}
+
+export function loggerIsMonitoringContext<T extends ITelemetryBaseLogger = ITelemetryLogger>(
+    obj: T): obj is T & MonitoringContext<T> {


You don't need to say T &, it knows it's still T.

Suggested change

obj: T): obj is T & MonitoringContext<T> {

obj: T,

): obj is MonitoringContext<T> {

markfields · 2021-12-17T20:07:09Z

packages/utils/telemetry-utils/src/config.ts

+export function loggerIsMonitoringContext<T extends ITelemetryBaseLogger = ITelemetryLogger>(
+    obj: T): obj is T & MonitoringContext<T> {
+    const maybeConfig = obj as Partial<MonitoringContext<T>> | undefined;
+    return isConfigProviderBase(maybeConfig?.config) && maybeConfig?.logger !== undefined;


No checks on the shape of logger prop?

Wait... I thought maybeConfig is a logger in this case, not has a logger... Haven't looked at usages yet. But that's what the types tell me, I think?

Ohhh... when you mix it in you do mc = logger; mc.logger = logger. Wanna do maybeConfig?.logger === maybeConfig? 🤪 jk

markfields · 2021-12-17T20:07:21Z

packages/utils/telemetry-utils/src/config.ts

+
+export function loggerToMonitoringContext<T extends ITelemetryBaseLogger = ITelemetryLogger>(
+    logger: T): MonitoringContext<T> {
+    if(loggerIsMonitoringContext(logger)) {


Suggested change

if(loggerIsMonitoringContext(logger)) {

if (loggerIsMonitoringContext(logger)) {

markfields · 2021-12-17T20:07:41Z

packages/utils/telemetry-utils/src/config.ts

+}
+
+export function loggerToMonitoringContext<T extends ITelemetryBaseLogger = ITelemetryLogger>(
+    logger: T): MonitoringContext<T> {


style nit:

Suggested change

logger: T): MonitoringContext<T> {

logger: T,

): MonitoringContext<T> {

markfields · 2021-12-17T20:09:54Z

packages/utils/telemetry-utils/src/config.ts

+}
+
+export function mixinMonitoringContext<T extends ITelemetryBaseLogger = ITelemetryLogger>(
+    logger: T, ... configs: (IConfigProviderBase | undefined)[]) {


style nit:

Suggested change

logger: T, ... configs: (IConfigProviderBase | undefined)[]) {

logger: T,

...configs: (IConfigProviderBase | undefined)[],

) {

packages/utils/telemetry-utils/src/logger.ts

packages/utils/telemetry-utils/src/config.ts

markfields · 2021-12-17T20:29:21Z

packages/utils/telemetry-utils/src/test/config.spec.ts

+        assert.equal(config.getNumber("badNumber"), undefined);
+        assert.equal(config.getNumber("stringAndNumber"), 1);
+
+        assert.equal(config.getString("stringAndNumber"), "1");


I'd drop the "stringAnd" cases from this test, and simply test getString on every single setting. As-is, it's weird to have number: "1" above, and only be testing getString on those, when the arrays are also in there as strings.

I'd leave these cases but probably a rename is in order to iron out any confusion. The problem was that the converter would have looked at the value ("1"), interpret it as a primitive type and return a number. This broke the scenario of trying to read a string which was actually the serialization of a number so every time you'd try to read a string, you'd get the default value because you weren't getting the expected type. When that was fixed, these test cases have been added.

How about this, for each case:

getRawConfig should return the string value

getFoo should return the value parsed as Foo

getString should return the string value

Then remove the "stringAnd" cases as redundant.

packages/utils/telemetry-utils/src/test/config.spec.ts

…to config-provider

Co-authored-by: Mark Fields <markfields@users.noreply.github.com>

…luidFramework-1 into config-provider

packages/drivers/odsp-driver/src/odspDocumentDeltaConnection.ts

packages/runtime/container-runtime/src/containerRuntime.ts

packages/runtime/container-runtime/src/garbageCollection.ts

markfields · 2021-12-27T21:46:04Z

packages/utils/telemetry-utils/src/config.ts

-    const mc: T & Partial<MonitoringContext<T>> = logger;
-    mc.config = new CachedConfigProvider(... configs);
+    const mc: L & Partial<MonitoringContext<N,L>> = logger;
+    mc.config = new CachedConfigProvider(... configs) as unknown as NamespacedConfigProvider<N>;


Wondering about updating CachedConfigProvider to be CachedConfigProvider<N> extends NamespacedConfigProvider<N> to avoid the cast.

packages/utils/telemetry-utils/src/config.ts

markfields · 2021-12-27T22:13:39Z

packages/utils/telemetry-utils/src/test/config.spec.ts

+        assert.equal(config.getNumber("badNumber"), undefined);
+        assert.equal(config.getNumber("stringAndNumber"), 1);
+
+        assert.equal(config.getString("stringAndNumber"), "1");


How about this, for each case:

getRawConfig should return the string value

getFoo should return the value parsed as Foo

getString should return the string value

Then remove the "stringAnd" cases as redundant.

markfields · 2021-12-27T22:39:32Z

packages/utils/telemetry-utils/src/config.ts

+                    return parsed;
+                }
+            }
+            // configs are immutable, if the first lookup returned no results, all lookups should


Just giving feedback that the wording here was hard to understand at first. Consider something like "cached config values are permanent" instead of "configs are immutable".

I'm bringing a different dialect to the table too, where "config" more naturally means what you're calling "config provider", and what you call "config" I would call "config value" or "setting". So it's very subjective feedback :)

markfields · 2021-12-27T23:37:36Z

packages/drivers/driver-base/src/documentDeltaConnection.ts

-        try {
-            return localStorage !== undefined
-                && typeof localStorage === "object"
-                && localStorage.getItem(batchManagerDisabledKey) === "1";


FC knows this is all changing?

packages/drivers/odsp-driver/src/odspSummaryUploadManager.ts

markfields · 2021-12-27T23:40:41Z

packages/loader/container-loader/src/loader.ts

@@ -289,17 +303,25 @@ export class Loader implements IHostLoader {
            loaderId: uuid(),
            loaderVersion: pkgVersion,
        };
+
+        const subMc = mixinMonitoringContext(
+            DebugLogger.mixinDebugLogger("fluid:telemetry", loaderProps.logger, { all: telemetryProps }),


Tangent: I wish I could magically change this to just "fluid" namespace, the ":telemetry" is just unnecessary

packages/loader/container-loader/src/loader.ts

markfields · 2021-12-27T23:50:25Z

packages/loader/driver-utils/src/blobAggregationStorage.ts

        packingLevel = 2,
    ) {
        if (storage instanceof BlobAggregationStorage) {
            return storage;
        }
+        const mc = loggerToMonitoringContext(logger);
+        const realAllowPackaging = mc.config.getBoolean("FluidAggregateBlobs") ??  allowPacking ?? false;


Also, I think this changes the semantics of the default value of allowPacking. Previously, config was only checked if the arg was not supplied. Now the config is the authority and the param is the fallback.

markfields

🎂 Really happy to see this get in! LGTM once you've considered the comments I left, some may be changes you want to take.

Co-authored-by: Mark Fields <markfields@users.noreply.github.com>

anthony-murphy and others added 26 commits November 8, 2021 15:46

initial config provider

95f333d

initial config provider

3c0238c

Merge branch 'config-provider' of https://github.com/anthony-murphy/F…

a744c44

…luidFramework-1 into config-provider

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

35df44a

…to config-provider

alpha

80baf21

mixin with child logger

0cbebb1

add debug logging

d31bc25

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

1346d85

…to config-provider

improve typing

072350e

checkpoint

4e901a8

Add some tests

ee39887

Add more tests

5905c48

Override with breakglass

4d9e79b

Fix all instances of old API

6780154

Fix another usage of old API

feaf781

sessionStorage may not be defined

902f092

Merge branch 'main' into config-provider

2a246eb

Fix sessionstorage vs storage ref

f2c16fb

Fix undefined sessionstorage

cbe2925

moving some stuff around

48e6e19

SettingsProvider bridge

b9d9fb1

Some comments here and there

05fd94c

Merge branch 'main' into config-provider

d8f4c41

Fix region name

217b15a

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

2c3eae6

…to config-provider

add monitoring context

f0dab34

github-actions bot added area: driver Driver related issues area: loader Loader related issues area: odsp-driver area: runtime Runtime related issues labels Dec 8, 2021

clean up and simplfy parsing code

f785807

github-actions bot added the dependencies Pull requests that update a dependency file label Dec 15, 2021

github-actions bot removed the request for review from a team December 15, 2021 18:54

github-actions bot removed the area: tests Tests to add, test infrastructure improvements, etc label Dec 15, 2021

docs update

db5ffe8

anthony-murphy linked an issue Dec 15, 2021 that may be closed by this pull request

Unified Configuration Management System #5921

Closed

markfields reviewed Dec 15, 2021

View reviewed changes

api-report/driver-base.api.md Outdated Show resolved Hide resolved

andre4i approved these changes Dec 16, 2021

View reviewed changes

markfields reviewed Dec 17, 2021

View reviewed changes

anthony-murphy added 2 commits December 27, 2021 09:57

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

975998b

…to config-provider

add namespacing via template literal types

c38a1cc

github-actions bot removed the dependencies Pull requests that update a dependency file label Dec 27, 2021

Apply suggestions from code review

f6c50c2

Co-authored-by: Mark Fields <markfields@users.noreply.github.com>

This was referenced Dec 27, 2021

Remove batchmanager and default FlushMode related feature gates #7365

Closed

BatchManager should be removed, all the bugs in Runtime related to not using batches in right places fixed #3788

Closed

anthony-murphy added 2 commits December 27, 2021 12:27

add odsp driver namesapce

fe2a9d4

Merge branch 'config-provider' of https://github.com/anthony-murphy/F…

bd1e790

…luidFramework-1 into config-provider

markfields reviewed Dec 27, 2021

View reviewed changes

packages/drivers/odsp-driver/src/odspSummaryUploadManager.ts Outdated Show resolved Hide resolved

markfields reviewed Dec 27, 2021

View reviewed changes

packages/loader/container-loader/src/loader.ts Outdated Show resolved Hide resolved

markfields reviewed Dec 27, 2021

View reviewed changes

markfields approved these changes Dec 28, 2021

View reviewed changes

anthony-murphy and others added 2 commits December 27, 2021 16:36

remove template literal

95720fd

Apply suggestions from code review

a43e639

Co-authored-by: Mark Fields <markfields@users.noreply.github.com>

anthony-murphy merged commit d70cc05 into microsoft:main Dec 28, 2021

anthony-murphy deleted the config-provider branch December 28, 2021 02:00

anthony-murphy mentioned this pull request Jan 10, 2022

Add test automation that promotes defensive access of localStorage #5783

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configuration Management via MonitoringContext #8497

Configuration Management via MonitoringContext #8497

anthony-murphy commented Dec 8, 2021 •

edited

Loading

andre4i Dec 16, 2021

anthony-murphy Dec 16, 2021

markfields Dec 17, 2021

andre4i Dec 16, 2021

markfields left a comment

markfields Dec 17, 2021

markfields Dec 17, 2021

markfields Dec 17, 2021

markfields Dec 17, 2021

markfields Dec 17, 2021

markfields Dec 17, 2021

markfields Dec 17, 2021

markfields Dec 17, 2021

markfields Dec 17, 2021

andre4i Dec 27, 2021

markfields Dec 27, 2021

markfields Dec 27, 2021

markfields Dec 27, 2021

markfields Dec 27, 2021

markfields Dec 27, 2021 •

edited

Loading

markfields Dec 27, 2021

markfields Dec 27, 2021

markfields Dec 27, 2021

markfields left a comment

	if(loggerIsMonitoringContext(logger)) {
	if (loggerIsMonitoringContext(logger)) {

	obj: T): obj is T & MonitoringContext<T> {
	obj: T,
	): obj is MonitoringContext<T> {

	logger: T): MonitoringContext<T> {
	logger: T,
	): MonitoringContext<T> {

Configuration Management via MonitoringContext #8497

Configuration Management via MonitoringContext #8497

Conversation

anthony-murphy commented Dec 8, 2021 • 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

markfields 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

markfields Dec 27, 2021 • 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

markfields left a comment

Choose a reason for hiding this comment

anthony-murphy commented Dec 8, 2021 •

edited

Loading

markfields Dec 27, 2021 •

edited

Loading