From 988031281f52b6c0ea48d9be291f151e4d55a1db Mon Sep 17 00:00:00 2001 From: Brian Dubois Date: Wed, 3 Aug 2022 07:08:25 -0400 Subject: [PATCH] Simplify how experimentalPlugin query param works so we can simplify the feature flag infrastructure. (#5836) When we introduced `FeatureFlagMetadata` in https://github.com/tensorflow/tensorboard/pull/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. --- .../store/feature_flag_metadata.ts | 26 +++++--- .../store/feature_flag_metadata_test.ts | 27 +++++++- .../dashboard_deeplink_provider_test.ts | 4 +- .../webapp/routes/feature_flag_serializer.ts | 62 ++++++++----------- .../routes/feature_flag_serializer_test.ts | 42 ++++++++----- .../tb_feature_flag_data_source_test.ts | 2 +- 6 files changed, 96 insertions(+), 67 deletions(-) diff --git a/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts b/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts index bd64ac3a0c4..896058bfe62 100644 --- 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; +export type FeatureFlagType = + | boolean + | number + | string + | string[] + | null + | undefined; export type FeatureFlagMetadata = { 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 = { @@ -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 = { scalarsBatchSize: { @@ -60,8 +73,7 @@ export const FeatureFlagMetadataMap: FeatureFlagMetadataMapType = 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 index 2831f09912f..a4a0a573034 100644 --- 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 index 2ddf3325015..b212e1a8189 100644 --- 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 index 040e84b5af7..474fa1ea38e 100644 --- a/tensorboard/webapp/routes/feature_flag_serializer.ts +++ b/tensorboard/webapp/routes/feature_flag_serializer.ts @@ -26,33 +26,31 @@ export function featureFlagsToSerializableQueryParams( ): SerializableQueryParams { return Object.entries(overriddenFeatureFlags) .map(([featureFlag, featureValue]) => { - const key = - featureFlagMetadataMap[featureFlag as keyof FeatureFlags] - ?.queryParamOverride; + const featureFlagMetadata: FeatureFlagMetadata = + 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( export function getFeatureFlagValueFromSearchParams( flagMetadata: FeatureFlagMetadata, 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 index f2a541c0d21..8cd8a07b982 100644 --- 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 = {}; @@ -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,7 +136,7 @@ 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') @@ -137,10 +144,10 @@ describe('feature flag serializer', () => { 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 index 70a144058be..314c17bea58 100644 --- 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'],