Simplify how experimentalPlugin query param works so we can simplify …

…the feature flag infrastructure. (tensorflow#5836) When we introduced `FeatureFlagMetadata` in tensorflow#5717, we included the`isArray' property, which allowed us to support feature flags that (1) have array type and (2) can be overriden with query parameters. There is only a single feature flag that satisfies this - enabledExperimentalPlugins/experimentalPlugin. The isArray support adds complexity to the feature flag framework - parsing and encoding logic need special handling for array types in addition to basic types. I wondered if we could eliminate the complexity from the feature flag framework and instead isolate the array-handling to a narrower scope. The idea: We can simplify the `experimentalPlugin` query parameter and then simplify FeatureFlagMetadata and the surrounding infrastructure. We change experimentalPlugin query parameter to be a single comma-delimited value instead of multiple query parameters with single values. That is, we would now write `experimentalPlugin=plugin1,plugin2,plugin3` where we previously would have written `experimentalPlugin=plugin1&experimentalPlugin=plugin2&experimentalPlugin=plugin3`. Once experimentalPlugin is just a single query parameter there is a path towards removing `isArray` and simplifying the framework. We remove `isArray`. enabledExperimentalPlugins specifies a function for parseValue that knows how to convert strings of type 'val1,val2,val3' into a string[]. And, fortunately, we can rely on string[].toString() to encode the array value back to the query parameter string. The array-specific logic in the greater framework is removed. The complexity is now isolated to the definition of enabledExperimentalPlugins.
dna2fork · May 1, 2023 · 9880312 · 9880312
1 parent 4aa7469
commit 9880312
Show file tree

Hide file tree

Showing 6 changed files with 96 additions and 67 deletions.
diff --git a/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts b/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts
@@ -14,15 +14,21 @@ limitations under the License.
 ==============================================================================*/
 import {FeatureFlags} from '../types';
 
-export type BaseFeatureFlagType = boolean | number | string | null | undefined;
-
-export type FeatureFlagType = BaseFeatureFlagType | Array<BaseFeatureFlagType>;
+export type FeatureFlagType =
+  | boolean
+  | number
+  | string
+  | string[]
+  | null
+  | undefined;
 
 export type FeatureFlagMetadata<T> = {
   defaultValue: T;
+  // The name of the query param users can use to override the feature flag
+  // value. If unspecified then users cannot override the feature flag value.
   queryParamOverride?: string;
-  parseValue: (str: string) => T extends (infer U)[] ? U : T; // The type, or, if the type is an array, the type of the array contents
-  isArray?: boolean;
+  // Function that translates a query param value into the feature flag value.
+  parseValue: (str: string) => T;
 };
 
 export type FeatureFlagMetadataMapType<T extends FeatureFlags> = {
@@ -40,6 +46,13 @@ export function parseBooleanOrNull(str: string): boolean | null {
   return parseBoolean(str);
 }
 
+export function parseStringArray(str: string): string[] {
+  if (!str) {
+    return [];
+  }
+  return str.split(',');
+}
+
 export const FeatureFlagMetadataMap: FeatureFlagMetadataMapType<FeatureFlags> =
   {
     scalarsBatchSize: {
@@ -60,8 +73,7 @@ export const FeatureFlagMetadataMap: FeatureFlagMetadataMapType<FeatureFlags> =
     enabledExperimentalPlugins: {
       defaultValue: [],
       queryParamOverride: 'experimentalPlugin',
-      parseValue: (str: string) => str,
-      isArray: true,
+      parseValue: parseStringArray,
     },
     enabledLinkedTime: {
       defaultValue: false,

diff --git a/tensorboard/webapp/feature_flag/store/feature_flag_metadata_test.ts b/tensorboard/webapp/feature_flag/store/feature_flag_metadata_test.ts
@@ -1,5 +1,3 @@
-import {parseBoolean, parseBooleanOrNull} from './feature_flag_metadata';
-
 /* Copyright 2022 The TensorFlow Authors. All Rights Reserved.
 
 Licensed under the Apache License, Version 2.0 (the "License");
@@ -14,6 +12,12 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 See the License for the specific language governing permissions and
 limitations under the License.
 ==============================================================================*/
+import {
+  parseBoolean,
+  parseBooleanOrNull,
+  parseStringArray,
+} from './feature_flag_metadata';
+
 describe('feature flag query parameters', () => {
   describe('parseBoolean', () => {
     it('"false" should evaluate to false', () => {
@@ -42,4 +46,23 @@ describe('feature flag query parameters', () => {
       expect(parseBooleanOrNull('')).toBeTrue();
     });
   });
+
+  describe('parseStringArray', () => {
+    it('parses empty value to empty array', () => {
+      expect(parseStringArray('')).toEqual([]);
+    });
+
+    it('parses single value to single element array', () => {
+      expect(parseStringArray('value1')).toEqual(['value1']);
+    });
+
+    it('parses multiple values to array', () => {
+      expect(parseStringArray('value1,value2,,value3')).toEqual([
+        'value1',
+        'value2',
+        '',
+        'value3',
+      ]);
+    });
+  });
 });
diff --git a/tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts b/tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts
@@ -360,9 +360,7 @@ describe('core deeplink provider', () => {
       store.refreshState();
 
       expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([
-        {key: 'experimentalPlugin', value: 'foo'},
-        {key: 'experimentalPlugin', value: 'bar'},
-        {key: 'experimentalPlugin', value: 'baz'},
+        {key: 'experimentalPlugin', value: 'foo,bar,baz'},
       ]);
     });
 

diff --git a/tensorboard/webapp/routes/feature_flag_serializer.ts b/tensorboard/webapp/routes/feature_flag_serializer.ts
@@ -26,33 +26,31 @@ export function featureFlagsToSerializableQueryParams<T extends FeatureFlags>(
 ): SerializableQueryParams {
   return Object.entries(overriddenFeatureFlags)
     .map(([featureFlag, featureValue]) => {
-      const key =
-        featureFlagMetadataMap[featureFlag as keyof FeatureFlags]
-          ?.queryParamOverride;
+      const featureFlagMetadata: FeatureFlagMetadata<any> =
+        featureFlagMetadataMap[featureFlag as keyof FeatureFlags];
+      if (!featureFlagMetadata) {
+        // No metadata for this feature flag. Shouldn't happen but we must
+        // include the check for the compiler.
+        // Return empty item. Will be filtered out.
+        return {};
+      }
+      const key = featureFlagMetadata.queryParamOverride;
       if (!key || featureValue === undefined) {
-        return [];
+        // Feature flag has no query param or there was no overriden value
+        // specified.
+        // Return empty item. Will be filtered out.
+        return {};
       }
-      /**
-       * Features with array values should be serialized as multiple query params, e.g.
-       * enabledExperimentalPlugins: {
-       *    queryParamOverride: 'experimentalPlugin',
-       *    values: ['foo', 'bar'],
-       *  }
-       *    Should be serialized to:
-       * ?experimentalPlugin=foo&experimentalPlugin=bar
-       *
-       * Because values can be arrays it is easiest to convert non array values to an
-       * array, then flatten the result.
-       */
-      const values = Array.isArray(featureValue)
-        ? featureValue
-        : [featureValue];
-      return values.map((value) => ({
+      return {
         key,
-        value: value?.toString(),
-      }));
+        // Note that all FeatureFlagType (string | number | boolean | string[])
+        // support toString() and toString() happens to output the format we
+        // want. Mostly notably, string[].toString() effectively does join(',').
+        // If this does hold when we add new types then consider adding support
+        // for custom encoders.
+        value: featureValue?.toString(),
+      };
     })
-    .flat()
     .filter(
       ({key, value}) => key && value !== undefined
     ) as SerializableQueryParams;
@@ -64,26 +62,16 @@ export function featureFlagsToSerializableQueryParams<T extends FeatureFlags>(
 export function getFeatureFlagValueFromSearchParams<T extends FeatureFlagType>(
   flagMetadata: FeatureFlagMetadata<T>,
   params: URLSearchParams
-): T | T[] | null {
+): T | null {
   const queryParamOverride = flagMetadata.queryParamOverride;
   if (!queryParamOverride || !params.has(queryParamOverride)) {
     return null;
   }
-  /**
-   * Array type feature flags are intended to be overridden multiple times
-   * i.e. ?experimentalPlugin=foo&experimentalPlugin=bar
-   * By using get params.getAll we can reuse the logic between array and non array types.
-   */
-  const paramValues: T[] = params.getAll(queryParamOverride).map((value) => {
-    return flagMetadata.parseValue(value) as T;
-  });
-  if (!paramValues.length) {
+  const paramValue = params.get(queryParamOverride);
+  if (paramValue == null) {
     return null;
   }
-
-  // There will always be an array of values but if the flag is not declared to be an array
-  // there SHOULD only be a single value which should then be returned.
-  return flagMetadata.isArray ? paramValues : paramValues[0];
+  return flagMetadata.parseValue(paramValue);
 }
 
 /**

diff --git a/tensorboard/webapp/routes/feature_flag_serializer_test.ts b/tensorboard/webapp/routes/feature_flag_serializer_test.ts
@@ -15,6 +15,8 @@ limitations under the License.
 import {
   FeatureFlagMetadata,
   FeatureFlagMetadataMapType,
+  parseBoolean,
+  parseStringArray,
 } from '../feature_flag/store/feature_flag_metadata';
 import {
   featureFlagsToSerializableQueryParams,
@@ -24,6 +26,7 @@ import {
 
 const FEATURE_A_NAME = 'featureA';
 const FEATURE_B_NAME = 'featureB';
+const FEATURE_C_NAME = 'featureC';
 
 describe('feature flag serializer', () => {
   let featureFlagsMetadata: FeatureFlagMetadataMapType<any> = {};
@@ -35,10 +38,14 @@ describe('feature flag serializer', () => {
         parseValue: (s: string) => s,
       },
       [FEATURE_B_NAME]: {
-        defaultValue: 'feature_b_456',
+        defaultValue: ['feature_b_456'],
         queryParamOverride: 'feature_b',
-        isArray: true,
-        parseValue: (s: string) => s,
+        parseValue: parseStringArray,
+      },
+      [FEATURE_C_NAME]: {
+        defaultValue: true,
+        queryParamOverride: 'feature_c',
+        parseValue: parseBoolean,
       },
     };
   });
@@ -54,12 +61,12 @@ describe('feature flag serializer', () => {
 
     it('should not serialize feature flags with missing metadata', () => {
       let serializableQueryParams = featureFlagsToSerializableQueryParams(
-        {featureC: 'c'} as any,
+        {featureD: 'd'} as any,
         featureFlagsMetadata
       );
       expect(serializableQueryParams).toEqual([]);
       serializableQueryParams = featureFlagsToSerializableQueryParams(
-        {featureC: 'c', featureA: 'a'} as any,
+        {featureD: 'd', featureA: 'a'} as any,
         featureFlagsMetadata
       );
       expect(serializableQueryParams).toEqual([
@@ -72,22 +79,26 @@ describe('feature flag serializer', () => {
 
     it('should serialize feature flags with falsy values', () => {
       const serializableQueryParams = featureFlagsToSerializableQueryParams(
-        {featureB: false, featureA: ''} as any,
+        {featureB: [''], featureA: '', featureC: false} as any,
         featureFlagsMetadata
       );
       expect(serializableQueryParams).toEqual([
         {
           key: 'feature_b',
-          value: 'false',
+          value: '',
         },
         {
           key: 'feature_a',
           value: '',
         },
+        {
+          key: 'feature_c',
+          value: 'false',
+        },
       ]);
     });
 
-    it('should return multiple entries for features with array values', () => {
+    it('should return single entry for features with string[] type', () => {
       const serializableQueryParams = featureFlagsToSerializableQueryParams(
         {featureA: 'a', featureB: ['foo', 'bar']} as any,
         featureFlagsMetadata
@@ -99,11 +110,7 @@ describe('feature flag serializer', () => {
         },
         {
           key: 'feature_b',
-          value: 'foo',
-        },
-        {
-          key: 'feature_b',
-          value: 'bar',
+          value: 'foo,bar',
         },
       ]);
     });
@@ -129,18 +136,18 @@ describe('feature flag serializer', () => {
       expect(value).toBeNull();
     });
 
-    it('returns first value when feature flag is not an array', () => {
+    it('returns first value when multiple matching query params', () => {
       const value = getFeatureFlagValueFromSearchParams(
         featureFlagsMetadata[FEATURE_A_NAME],
         new URLSearchParams('?feature_a=foo&feature_a=bar')
       );
       expect(value).toEqual('foo');
     });
 
-    it('returns array of values when feature flag is an array', () => {
+    it('returns array of values when feature flag has array decoder', () => {
       const value = getFeatureFlagValueFromSearchParams(
         featureFlagsMetadata[FEATURE_B_NAME],
-        new URLSearchParams('?feature_b=foo&feature_b=bar')
+        new URLSearchParams('?feature_b=foo,bar')
       );
       expect(value).toEqual(['foo', 'bar']);
     });
@@ -166,11 +173,12 @@ describe('feature flag serializer', () => {
     it('parses flag values correctly', () => {
       const featureFlags = getOverriddenFeatureFlagValuesFromSearchParams(
         featureFlagsMetadata,
-        new URLSearchParams('?feature_a=foo&feature_b=bar')
+        new URLSearchParams('?feature_a=foo&feature_b=bar&feature_c=false')
       );
       expect(featureFlags).toEqual({
         featureA: 'foo',
         featureB: ['bar'],
+        featureC: false,
       } as any);
     });
   });

diff --git a/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source_test.ts b/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source_test.ts
@@ -49,7 +49,7 @@ describe('tb_feature_flag_data_source', () => {
 
       it('returns enabledExperimentalPlugins from the query params', () => {
         getParamsSpy.and.returnValue(
-          new URLSearchParams('experimentalPlugin=a&experimentalPlugin=b')
+          new URLSearchParams('experimentalPlugin=a,b')
         );
         expect(dataSource.getFeatures()).toEqual({
           enabledExperimentalPlugins: ['a', 'b'],