diff --git a/tensorboard/webapp/feature_flag/store/BUILD b/tensorboard/webapp/feature_flag/store/BUILD index 72677626fb5..6b6392322f4 100644 --- a/tensorboard/webapp/feature_flag/store/BUILD +++ b/tensorboard/webapp/feature_flag/store/BUILD @@ -10,6 +10,7 @@ tf_ng_module( "feature_flag_store_config_provider.ts", ], deps = [ + ":feature_flag_metadata", ":types", "//tensorboard/webapp/feature_flag:types", "//tensorboard/webapp/feature_flag/actions", @@ -21,6 +22,17 @@ tf_ng_module( ], ) +tf_ng_module( + name = "feature_flag_metadata", + srcs = [ + "feature_flag_metadata.ts", + ], + deps = [ + "//tensorboard/webapp/feature_flag:types", + "@npm//@angular/core", + ], +) + tf_ng_module( name = "types", srcs = [ @@ -45,10 +57,12 @@ tf_ng_module( name = "store_test_lib", testonly = True, srcs = [ + "feature_flag_metadata_test.ts", "feature_flag_reducers_test.ts", "feature_flag_selectors_test.ts", ], deps = [ + ":feature_flag_metadata", ":store", ":testing", ":types", diff --git a/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts b/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts new file mode 100644 index 00000000000..bd64ac3a0c4 --- /dev/null +++ b/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts @@ -0,0 +1,127 @@ +/* Copyright 2022 The TensorFlow Authors. All Rights Reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +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 {FeatureFlags} from '../types'; + +export type BaseFeatureFlagType = boolean | number | string | null | undefined; + +export type FeatureFlagType = BaseFeatureFlagType | Array; + +export type FeatureFlagMetadata = { + defaultValue: T; + 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; +}; + +export type FeatureFlagMetadataMapType = { + [FlagName in keyof T]: FeatureFlagMetadata; +}; + +export function parseBoolean(str: string): boolean { + return str !== 'false'; +} + +export function parseBooleanOrNull(str: string): boolean | null { + if (str === 'null') { + return null; + } + return parseBoolean(str); +} + +export const FeatureFlagMetadataMap: FeatureFlagMetadataMapType = + { + scalarsBatchSize: { + defaultValue: undefined, + queryParamOverride: 'scalarsBatchSize', + parseValue: parseInt, + }, + enabledColorGroup: { + defaultValue: true, + queryParamOverride: 'enableColorGroup', + parseValue: parseBoolean, + }, + enabledColorGroupByRegex: { + defaultValue: true, + queryParamOverride: 'enableColorGroupByRegex', + parseValue: parseBoolean, + }, + enabledExperimentalPlugins: { + defaultValue: [], + queryParamOverride: 'experimentalPlugin', + parseValue: (str: string) => str, + isArray: true, + }, + enabledLinkedTime: { + defaultValue: false, + queryParamOverride: 'enableLinkedTime', + parseValue: parseBoolean, + }, + enabledCardWidthSetting: { + defaultValue: true, + queryParamOverride: 'enableCardWidthSetting', + parseValue: parseBoolean, + }, + enabledScalarDataTable: { + defaultValue: false, + queryParamOverride: 'enableDataTable', + parseValue: parseBoolean, + }, + forceSvg: { + defaultValue: false, + queryParamOverride: 'forceSVG', + parseValue: parseBoolean, + }, + enableDarkModeOverride: { + defaultValue: null, + parseValue: parseBooleanOrNull, + }, + defaultEnableDarkMode: { + defaultValue: false, + queryParamOverride: 'darkMode', + parseValue: parseBoolean, + }, + isAutoDarkModeAllowed: { + defaultValue: true, + parseValue: parseBoolean, + }, + inColab: { + defaultValue: false, + queryParamOverride: 'tensorboardColab', + parseValue: parseBoolean, + }, + metricsImageSupportEnabled: { + defaultValue: true, + parseValue: parseBoolean, + }, + enableTimeSeriesPromotion: { + defaultValue: false, + parseValue: parseBoolean, + }, + }; + +/** + * Gets gets just the default values of each feature flag from the provided metadata. + */ +export function generateFeatureFlagDefaults( + featureFlagMetadataMap: FeatureFlagMetadataMapType +): T { + return Object.entries(featureFlagMetadataMap).reduce( + (map, [key, {defaultValue}]) => { + map[key] = defaultValue; + return map; + }, + {} as Record + ) as T; +} diff --git a/tensorboard/webapp/feature_flag/store/feature_flag_metadata_test.ts b/tensorboard/webapp/feature_flag/store/feature_flag_metadata_test.ts new file mode 100644 index 00000000000..2831f09912f --- /dev/null +++ b/tensorboard/webapp/feature_flag/store/feature_flag_metadata_test.ts @@ -0,0 +1,45 @@ +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"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +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. +==============================================================================*/ +describe('feature flag query parameters', () => { + describe('parseBoolean', () => { + it('"false" should evaluate to false', () => { + expect(parseBoolean('false')).toBeFalse(); + }); + + it('values other than "false" should evaluate to true', () => { + expect(parseBoolean('true')).toBeTrue(); + expect(parseBoolean('foo bar')).toBeTrue(); + expect(parseBoolean('')).toBeTrue(); + }); + }); + + describe('parseBooleanOrNull', () => { + it('"null" should return null', () => { + expect(parseBooleanOrNull('null')).toBeNull(); + }); + + it('"false" should evaluate to false', () => { + expect(parseBooleanOrNull('false')).toBeFalse(); + }); + + it('values other than "false" should evaluate to true', () => { + expect(parseBooleanOrNull('true')).toBeTrue(); + expect(parseBooleanOrNull('foo bar')).toBeTrue(); + expect(parseBooleanOrNull('')).toBeTrue(); + }); + }); +}); diff --git a/tensorboard/webapp/feature_flag/store/feature_flag_store_config_provider.ts b/tensorboard/webapp/feature_flag/store/feature_flag_store_config_provider.ts index c9ea4500702..4abda3e4d23 100644 --- a/tensorboard/webapp/feature_flag/store/feature_flag_store_config_provider.ts +++ b/tensorboard/webapp/feature_flag/store/feature_flag_store_config_provider.ts @@ -14,26 +14,15 @@ limitations under the License. ==============================================================================*/ import {InjectionToken} from '@angular/core'; import {StoreConfig} from '@ngrx/store'; +import { + FeatureFlagMetadataMap, + generateFeatureFlagDefaults, +} from './feature_flag_metadata'; import {FeatureFlagState} from './feature_flag_types'; export const initialState: FeatureFlagState = { isFeatureFlagsLoaded: false, - defaultFlags: { - isAutoDarkModeAllowed: true, - defaultEnableDarkMode: false, - enableDarkModeOverride: null, - enabledColorGroup: true, - enabledColorGroupByRegex: true, - enabledExperimentalPlugins: [], - inColab: false, - scalarsBatchSize: undefined, - metricsImageSupportEnabled: true, - enabledLinkedTime: false, - enableTimeSeriesPromotion: false, - enabledCardWidthSetting: true, - forceSvg: false, - enabledScalarDataTable: false, - }, + defaultFlags: generateFeatureFlagDefaults(FeatureFlagMetadataMap), flagOverrides: {}, }; diff --git a/tensorboard/webapp/routes/BUILD b/tensorboard/webapp/routes/BUILD index 7eacfcf7134..75253f3456e 100644 --- a/tensorboard/webapp/routes/BUILD +++ b/tensorboard/webapp/routes/BUILD @@ -37,11 +37,15 @@ tf_ts_library( ], deps = [ ":dashboard_deeplink_provider_types", + ":feature_flag_serializer", "//tensorboard/webapp:app_state", "//tensorboard/webapp:selectors", "//tensorboard/webapp/app_routing:deep_link_provider", "//tensorboard/webapp/app_routing:route_config", "//tensorboard/webapp/app_routing:types", + "//tensorboard/webapp/feature_flag:types", + "//tensorboard/webapp/feature_flag/store", + "//tensorboard/webapp/feature_flag/store:feature_flag_metadata", "//tensorboard/webapp/metrics:types", "//tensorboard/webapp/metrics/data_source:types", "//tensorboard/webapp/runs:types", @@ -53,6 +57,18 @@ tf_ts_library( ], ) +tf_ts_library( + name = "feature_flag_serializer", + srcs = [ + "feature_flag_serializer.ts", + ], + deps = [ + "//tensorboard/webapp/app_routing:types", + "//tensorboard/webapp/feature_flag:types", + "//tensorboard/webapp/feature_flag/store:feature_flag_metadata", + ], +) + tf_ts_library( name = "testing", testonly = True, @@ -69,16 +85,20 @@ tf_ts_library( testonly = True, srcs = [ "dashboard_deeplink_provider_test.ts", + "feature_flag_serializer_test.ts", ], deps = [ ":dashboard_deeplink_provider", + ":feature_flag_serializer", ":testing", "//tensorboard/webapp:app_state", "//tensorboard/webapp:selectors", "//tensorboard/webapp/angular:expect_angular_core_testing", "//tensorboard/webapp/angular:expect_ngrx_store_testing", "//tensorboard/webapp/app_routing:deep_link_provider", + "//tensorboard/webapp/app_routing:location", "//tensorboard/webapp/app_routing:types", + "//tensorboard/webapp/feature_flag/store:feature_flag_metadata", "//tensorboard/webapp/metrics:test_lib", "//tensorboard/webapp/metrics:types", "//tensorboard/webapp/metrics/data_source:types", diff --git a/tensorboard/webapp/routes/dashboard_deeplink_provider.ts b/tensorboard/webapp/routes/dashboard_deeplink_provider.ts index 80d594e3988..d7f0bafd74e 100644 --- a/tensorboard/webapp/routes/dashboard_deeplink_provider.ts +++ b/tensorboard/webapp/routes/dashboard_deeplink_provider.ts @@ -19,6 +19,12 @@ import {map} from 'rxjs/operators'; import {DeepLinkProvider} from '../app_routing/deep_link_provider'; import {SerializableQueryParams} from '../app_routing/types'; import {State} from '../app_state'; +import { + FeatureFlagMetadata, + FeatureFlagMetadataMap, + FeatureFlagType, +} from '../feature_flag/store/feature_flag_metadata'; +import {getOverriddenFeatureFlags} from '../feature_flag/store/feature_flag_selectors'; import { isPluginType, isSampledPlugin, @@ -27,11 +33,6 @@ import { import {CardUniqueInfo} from '../metrics/types'; import {GroupBy, GroupByKey} from '../runs/types'; import * as selectors from '../selectors'; -import { - ENABLE_COLOR_GROUP_BY_REGEX_QUERY_PARAM_KEY, - ENABLE_COLOR_GROUP_QUERY_PARAM_KEY, - EXPERIMENTAL_PLUGIN_QUERY_PARAM_KEY, -} from '../webapp_data_source/tb_feature_flag_data_source_types'; import { DeserializedState, PINNED_CARDS_KEY, @@ -40,6 +41,7 @@ import { SMOOTHING_KEY, TAG_FILTER_KEY, } from './dashboard_deeplink_provider_types'; +import {featureFlagsToSerializableQueryParams} from './feature_flag_serializer'; const COLOR_GROUP_REGEX_VALUE_PREFIX = 'regex:'; @@ -83,36 +85,6 @@ export class DashboardDeepLinkProvider extends DeepLinkProvider { ); } - private getFeatureFlagStates( - store: Store - ): Observable { - return combineLatest([ - store.select(selectors.getEnabledExperimentalPlugins), - store.select(selectors.getOverriddenFeatureFlags), - ]).pipe( - map(([experimentalPlugins, overriddenFeatureFlags]) => { - const queryParams = experimentalPlugins.map((pluginId) => { - return {key: EXPERIMENTAL_PLUGIN_QUERY_PARAM_KEY, value: pluginId}; - }); - if (typeof overriddenFeatureFlags.enabledColorGroup === 'boolean') { - queryParams.push({ - key: ENABLE_COLOR_GROUP_QUERY_PARAM_KEY, - value: String(overriddenFeatureFlags.enabledColorGroup), - }); - } - if ( - typeof overriddenFeatureFlags.enabledColorGroupByRegex === 'boolean' - ) { - queryParams.push({ - key: ENABLE_COLOR_GROUP_BY_REGEX_QUERY_PARAM_KEY, - value: String(overriddenFeatureFlags.enabledColorGroupByRegex), - }); - } - return queryParams; - }) - ); - } - serializeStateToQueryParams( store: Store ): Observable { @@ -126,7 +98,17 @@ export class DashboardDeepLinkProvider extends DeepLinkProvider { return [{key: TAG_FILTER_KEY, value: filterText}]; }) ), - this.getFeatureFlagStates(store), + store.select(getOverriddenFeatureFlags).pipe( + map((featureFlags) => { + return featureFlagsToSerializableQueryParams( + featureFlags, + FeatureFlagMetadataMap as Record< + string, + FeatureFlagMetadata + > + ); + }) + ), store.select(selectors.getMetricsSettingOverrides).pipe( map((settingOverrides) => { if (Number.isFinite(settingOverrides.scalarSmoothing)) { diff --git a/tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts b/tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts index f4fb27f3030..cf72c247a7b 100644 --- a/tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts +++ b/tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts @@ -347,13 +347,15 @@ describe('core deeplink provider', () => { }); }); + /** + * These tests are intended to verify that feature flags are correctly serialized using + * featureFlagsToSerializableQueryParams + */ describe('feature flag', () => { it('serializes enabled experimental plugins', () => { - store.overrideSelector(selectors.getEnabledExperimentalPlugins, [ - 'foo', - 'bar', - 'baz', - ]); + store.overrideSelector(selectors.getOverriddenFeatureFlags, { + enabledExperimentalPlugins: ['foo', 'bar', 'baz'], + }); store.refreshState(); expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([ diff --git a/tensorboard/webapp/routes/feature_flag_serializer.ts b/tensorboard/webapp/routes/feature_flag_serializer.ts new file mode 100644 index 00000000000..b0dd23af3b0 --- /dev/null +++ b/tensorboard/webapp/routes/feature_flag_serializer.ts @@ -0,0 +1,115 @@ +/* Copyright 2022 The TensorFlow Authors. All Rights Reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +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 {SerializableQueryParams} from '../app_routing/types'; +import { + FeatureFlagMetadata, + FeatureFlagMetadataMapType, + FeatureFlagType, +} from '../feature_flag/store/feature_flag_metadata'; +import {FeatureFlags} from '../feature_flag/types'; + +export function featureFlagsToSerializableQueryParams< + T extends FeatureFlagType +>( + overriddenFeatureFlags: Partial, + featureFlagMetadataMap: Record> +): SerializableQueryParams { + return Object.entries(overriddenFeatureFlags) + .map(([featureFlag, featureValue]) => { + const key = + featureFlagMetadataMap[featureFlag as keyof FeatureFlags] + ?.queryParamOverride; + if (!key || featureValue === undefined) { + 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) => ({ + key, + value: value?.toString(), + })); + }) + .flat() + .filter( + ({key, value}) => key && value !== undefined + ) as SerializableQueryParams; +} + +/** + * Parses the value of a feature flag from the query params. + */ +export function getFeatureFlagValueFromSearchParams( + flagMetadata: FeatureFlagMetadata, + params: URLSearchParams +): T | 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) { + 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]; +} + +/** + * Parses all feature flags from the query params. + */ +export function getOverriddenFeatureFlagValuesFromSearchParams< + T extends FeatureFlags +>( + featureFlagMetadataMap: FeatureFlagMetadataMapType, + params: URLSearchParams +) { + return Object.entries(featureFlagMetadataMap).reduce( + (overrides, [flagName, flagMetadata]) => { + const featureValue = getFeatureFlagValueFromSearchParams( + flagMetadata as FeatureFlagMetadata, + params + ); + + if (featureValue !== null) { + const f = flagName as keyof FeatureFlags; + overrides[f] = featureValue; + } + return overrides; + }, + {} as Partial> + ); +} diff --git a/tensorboard/webapp/routes/feature_flag_serializer_test.ts b/tensorboard/webapp/routes/feature_flag_serializer_test.ts new file mode 100644 index 00000000000..9eabf73ebde --- /dev/null +++ b/tensorboard/webapp/routes/feature_flag_serializer_test.ts @@ -0,0 +1,174 @@ +/* Copyright 2022 The TensorFlow Authors. All Rights Reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +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 { + FeatureFlagMetadata, + FeatureFlagMetadataMapType, +} from '../feature_flag/store/feature_flag_metadata'; +import { + featureFlagsToSerializableQueryParams, + getFeatureFlagValueFromSearchParams, + getOverriddenFeatureFlagValuesFromSearchParams, +} from './feature_flag_serializer'; + +describe('feature flag serializer', () => { + let featureFlagsMetadata: Record> = {}; + beforeEach(() => { + featureFlagsMetadata = { + featureA: { + defaultValue: 'feature_a_123', + queryParamOverride: 'feature_a', + parseValue: (s: string) => s, + }, + featureB: { + defaultValue: 'feature_b_456', + queryParamOverride: 'feature_b', + isArray: true, + parseValue: (s: string) => s, + }, + }; + }); + + describe('featureFlagsToSerializableQueryParams', () => { + it('should return empty list when no flags are overridden', () => { + const serializableQueryParams = featureFlagsToSerializableQueryParams( + {}, + featureFlagsMetadata + ); + expect(serializableQueryParams).toEqual([]); + }); + + it('should not serialize feature flags with missing metadata', () => { + let serializableQueryParams = featureFlagsToSerializableQueryParams( + {featureC: 'c'} as any, + featureFlagsMetadata + ); + expect(serializableQueryParams).toEqual([]); + serializableQueryParams = featureFlagsToSerializableQueryParams( + {featureC: 'c', featureA: 'a'} as any, + featureFlagsMetadata + ); + expect(serializableQueryParams).toEqual([ + { + key: 'feature_a', + value: 'a', + }, + ]); + }); + + it('should serialize feature flags with falsy values', () => { + const serializableQueryParams = featureFlagsToSerializableQueryParams( + {featureB: false, featureA: ''} as any, + featureFlagsMetadata + ); + expect(serializableQueryParams).toEqual([ + { + key: 'feature_b', + value: 'false', + }, + { + key: 'feature_a', + value: '', + }, + ]); + }); + + it('should return multiple entries for features with array values', () => { + const serializableQueryParams = featureFlagsToSerializableQueryParams( + {featureA: 'a', featureB: ['foo', 'bar']} as any, + featureFlagsMetadata + ); + expect(serializableQueryParams).toEqual([ + { + key: 'feature_a', + value: 'a', + }, + { + key: 'feature_b', + value: 'foo', + }, + { + key: 'feature_b', + value: 'bar', + }, + ]); + }); + }); + + describe('getFeatureFlagValueFromSearchParams', () => { + it('returns null when provided feature flag not present in search params', () => { + const value = getFeatureFlagValueFromSearchParams( + featureFlagsMetadata.featureA, + new URLSearchParams('') + ); + expect(value).toBeNull(); + }); + + it('returns null when feature flag does not have a query param override', () => { + const value = getFeatureFlagValueFromSearchParams( + { + defaultValue: 'some value', + parseValue: (s: string) => s, + } as FeatureFlagMetadata, + new URLSearchParams('') + ); + expect(value).toBeNull(); + }); + + it('returns first value when feature flag is not an array', () => { + const value = getFeatureFlagValueFromSearchParams( + featureFlagsMetadata.featureA, + new URLSearchParams('?feature_a=foo&feature_a=bar') + ); + expect(value).toEqual('foo'); + }); + + it('returns array of values when feature flag is an array', () => { + const value = getFeatureFlagValueFromSearchParams( + featureFlagsMetadata.featureB, + new URLSearchParams('?feature_b=foo&feature_b=bar') + ); + expect(value).toEqual(['foo', 'bar']); + }); + }); + + describe('getOverriddenFeatureFlagValuesFromSearchParams', () => { + it('returns empty object when metadata is empty', () => { + const featureFlags = getOverriddenFeatureFlagValuesFromSearchParams( + {} as FeatureFlagMetadataMapType, + new URLSearchParams('?feature_a=foo') + ); + expect(featureFlags).toEqual({}); + }); + + it('returns empty object when url search params are empty', () => { + const featureFlags = getOverriddenFeatureFlagValuesFromSearchParams( + featureFlagsMetadata as FeatureFlagMetadataMapType, + new URLSearchParams('') + ); + expect(featureFlags).toEqual({}); + }); + + it('parses flag values correctly', () => { + const featureFlags = getOverriddenFeatureFlagValuesFromSearchParams( + featureFlagsMetadata as FeatureFlagMetadataMapType, + new URLSearchParams('?feature_a=foo&feature_b=bar') + ); + expect(featureFlags).toEqual({ + featureA: 'foo', + featureB: ['bar'], + } as any); + }); + }); +}); diff --git a/tensorboard/webapp/webapp_data_source/BUILD b/tensorboard/webapp/webapp_data_source/BUILD index 575a40ea7e4..f42b3fef2a6 100644 --- a/tensorboard/webapp/webapp_data_source/BUILD +++ b/tensorboard/webapp/webapp_data_source/BUILD @@ -98,6 +98,8 @@ tf_ng_module( deps = [ ":feature_flag_types", "//tensorboard/webapp/feature_flag:types", + "//tensorboard/webapp/feature_flag/store:feature_flag_metadata", + "//tensorboard/webapp/routes:feature_flag_serializer", "//tensorboard/webapp/types", "@npm//@angular/common", "@npm//@angular/core", @@ -128,6 +130,7 @@ tf_ng_module( deps = [ ":feature_flag", "//tensorboard/webapp/angular:expect_angular_core_testing", + "//tensorboard/webapp/feature_flag/store:feature_flag_metadata", "@npm//@angular/core", "@npm//@types/jasmine", ], diff --git a/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source.ts b/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source.ts index 3668005ef3b..e63ddd17e81 100644 --- a/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source.ts +++ b/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source.ts @@ -13,20 +13,14 @@ See the License for the specific language governing permissions and limitations under the License. ==============================================================================*/ import {Injectable} from '@angular/core'; +import { + FeatureFlagMetadataMap, + FeatureFlagType, +} from '../feature_flag/store/feature_flag_metadata'; import {FeatureFlags} from '../feature_flag/types'; +import {getOverriddenFeatureFlagValuesFromSearchParams} from '../routes/feature_flag_serializer'; import {QueryParams} from './query_params'; -import { - ENABLE_CARD_WIDTH_SETTING_PARAM_KEY, - ENABLE_COLOR_GROUP_BY_REGEX_QUERY_PARAM_KEY, - ENABLE_COLOR_GROUP_QUERY_PARAM_KEY, - ENABLE_DARK_MODE_QUERY_PARAM_KEY, - ENABLE_DATA_TABLE_PARAM_KEY, - ENABLE_LINKED_TIME_PARAM_KEY, - EXPERIMENTAL_PLUGIN_QUERY_PARAM_KEY, - FORCE_SVG_RENDERER, - SCALARS_BATCH_SIZE_PARAM_KEY, - TBFeatureFlagDataSource, -} from './tb_feature_flag_data_source_types'; +import {TBFeatureFlagDataSource} from './tb_feature_flag_data_source_types'; const DARK_MODE_MEDIA_QUERY = '(prefers-color-scheme: dark)'; @@ -40,62 +34,20 @@ export class QueryParamsFeatureFlagDataSource constructor(readonly queryParams: QueryParams) {} getFeatures(enableMediaQuery: boolean = false) { - const params = this.queryParams.getParams(); // Set feature flag value for query parameters that are explicitly // specified. Feature flags for unspecified query parameters remain unset so // their values in the underlying state are not inadvertently changed. - const featureFlags: Partial = enableMediaQuery - ? this.getPartialFeaturesFromMediaQuery() - : {}; - if (params.has(EXPERIMENTAL_PLUGIN_QUERY_PARAM_KEY)) { - featureFlags.enabledExperimentalPlugins = params.getAll( - EXPERIMENTAL_PLUGIN_QUERY_PARAM_KEY - ); - } - if (params.has('tensorboardColab')) { - featureFlags.inColab = params.get('tensorboardColab') === 'true'; - } - if (params.has(SCALARS_BATCH_SIZE_PARAM_KEY)) { - featureFlags.scalarsBatchSize = Number( - params.get(SCALARS_BATCH_SIZE_PARAM_KEY) - ); - } - - if (params.has(ENABLE_COLOR_GROUP_QUERY_PARAM_KEY)) { - featureFlags.enabledColorGroup = - params.get(ENABLE_COLOR_GROUP_QUERY_PARAM_KEY) !== 'false'; - } - - if (params.has(ENABLE_COLOR_GROUP_BY_REGEX_QUERY_PARAM_KEY)) { - featureFlags.enabledColorGroupByRegex = - params.get(ENABLE_COLOR_GROUP_BY_REGEX_QUERY_PARAM_KEY) !== 'false'; - } - - if (params.has(ENABLE_DARK_MODE_QUERY_PARAM_KEY)) { - featureFlags.defaultEnableDarkMode = - params.get(ENABLE_DARK_MODE_QUERY_PARAM_KEY) !== 'false'; - } - - if (params.has(ENABLE_LINKED_TIME_PARAM_KEY)) { - featureFlags.enabledLinkedTime = - params.get(ENABLE_LINKED_TIME_PARAM_KEY) !== 'false'; - } - - if (params.has(ENABLE_CARD_WIDTH_SETTING_PARAM_KEY)) { - featureFlags.enabledCardWidthSetting = - params.get(ENABLE_CARD_WIDTH_SETTING_PARAM_KEY) !== 'false'; - } - - if (params.has(FORCE_SVG_RENDERER)) { - featureFlags.forceSvg = params.get(FORCE_SVG_RENDERER) !== 'false'; - } - - if (params.has(ENABLE_DATA_TABLE_PARAM_KEY)) { - featureFlags.enabledScalarDataTable = - params.get(ENABLE_DATA_TABLE_PARAM_KEY) !== 'false'; - } - - return featureFlags; + const featuresFromMediaQuery: Partial< + Record + > = enableMediaQuery ? this.getPartialFeaturesFromMediaQuery() : {}; + const overriddenFeatures = getOverriddenFeatureFlagValuesFromSearchParams( + FeatureFlagMetadataMap, + this.queryParams.getParams() + ); + return { + ...featuresFromMediaQuery, + ...overriddenFeatures, + } as Partial; } protected getPartialFeaturesFromMediaQuery(): { 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 f054f64f2ef..70a144058be 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 @@ -58,7 +58,7 @@ describe('tb_feature_flag_data_source', () => { it('returns inColab=false when `tensorboardColab` is empty', () => { getParamsSpy.and.returnValue(new URLSearchParams('tensorboardColab')); - expect(dataSource.getFeatures()).toEqual({inColab: false}); + expect(dataSource.getFeatures()).toEqual({inColab: true}); }); it('returns inColab=true when `tensorboardColab` is`true`', () => { @@ -84,123 +84,197 @@ describe('tb_feature_flag_data_source', () => { }); }); - it('returns enableColorGroup from the query params', () => { - getParamsSpy.and.returnValues( - new URLSearchParams('enableColorGroup=false'), - new URLSearchParams('enableColorGroup='), - new URLSearchParams('enableColorGroup=true'), - new URLSearchParams('enableColorGroup=foo') - ); - expect(dataSource.getFeatures()).toEqual({enabledColorGroup: false}); - expect(dataSource.getFeatures()).toEqual({enabledColorGroup: true}); - expect(dataSource.getFeatures()).toEqual({enabledColorGroup: true}); - expect(dataSource.getFeatures()).toEqual({enabledColorGroup: true}); + describe('returns enableColorGroup from the query params', () => { + it('when set to false', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableColorGroup=false') + ); + expect(dataSource.getFeatures()).toEqual({enabledColorGroup: false}); + }); + + it('when set to empty string', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableColorGroup=') + ); + expect(dataSource.getFeatures()).toEqual({enabledColorGroup: true}); + }); + + it('when set to true', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableColorGroup=true') + ); + expect(dataSource.getFeatures()).toEqual({enabledColorGroup: true}); + }); + + it('when set to an arbitrary string', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableColorGroup=foo') + ); + expect(dataSource.getFeatures()).toEqual({enabledColorGroup: true}); + }); }); - it('returns enableColorGroupByRegex from the query params', () => { - getParamsSpy.and.returnValues( - new URLSearchParams('enableColorGroupByRegex=false'), - new URLSearchParams('enableColorGroupByRegex='), - new URLSearchParams('enableColorGroupByRegex=true'), - new URLSearchParams('enableColorGroupByRegex=foo') - ); - expect(dataSource.getFeatures()).toEqual({ - enabledColorGroupByRegex: false, + describe('returns enabledColorGroupByRegex from the query params', () => { + it('when set to false', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableColorGroupByRegex=false') + ); + expect(dataSource.getFeatures()).toEqual({ + enabledColorGroupByRegex: false, + }); }); - expect(dataSource.getFeatures()).toEqual({ - enabledColorGroupByRegex: true, + + it('when set to empty string', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableColorGroupByRegex=') + ); + expect(dataSource.getFeatures()).toEqual({ + enabledColorGroupByRegex: true, + }); }); - expect(dataSource.getFeatures()).toEqual({ - enabledColorGroupByRegex: true, + + it('when set to true', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableColorGroupByRegex=true') + ); + expect(dataSource.getFeatures()).toEqual({ + enabledColorGroupByRegex: true, + }); }); - expect(dataSource.getFeatures()).toEqual({ - enabledColorGroupByRegex: true, + + it('when set to an arbitrary string', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableColorGroupByRegex=foo') + ); + expect(dataSource.getFeatures()).toEqual({ + enabledColorGroupByRegex: true, + }); }); }); - it('returns enableLinkedTime from the query params', () => { - getParamsSpy.and.returnValues( - new URLSearchParams('enableLinkedTime=false'), - new URLSearchParams('enableLinkedTime='), - new URLSearchParams('enableLinkedTime=true'), - new URLSearchParams('enableLinkedTime=foo') - ); - expect(dataSource.getFeatures()).toEqual({ - enabledLinkedTime: false, + describe('returns enabledLinkedTime from the query params', () => { + it('when set to false', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableLinkedTime=false') + ); + expect(dataSource.getFeatures()).toEqual({enabledLinkedTime: false}); }); - expect(dataSource.getFeatures()).toEqual({ - enabledLinkedTime: true, + + it('when set to empty string', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableLinkedTime=') + ); + expect(dataSource.getFeatures()).toEqual({enabledLinkedTime: true}); }); - expect(dataSource.getFeatures()).toEqual({ - enabledLinkedTime: true, + + it('when set to true', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableLinkedTime=true') + ); + expect(dataSource.getFeatures()).toEqual({enabledLinkedTime: true}); }); - expect(dataSource.getFeatures()).toEqual({ - enabledLinkedTime: true, + + it('when set to an arbitrary string', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableLinkedTime=foo') + ); + expect(dataSource.getFeatures()).toEqual({enabledLinkedTime: true}); }); }); - it('returns enableCardWidthSetting from the query params', () => { - getParamsSpy.and.returnValues( - new URLSearchParams('enableCardWidthSetting=false'), - new URLSearchParams('enableCardWidthSetting='), - new URLSearchParams('enableCardWidthSetting=true'), - new URLSearchParams('enableCardWidthSetting=foo') - ); - - expect(dataSource.getFeatures()).toEqual({ - enabledCardWidthSetting: false, + describe('returns enabledCardWidthSetting from the query params', () => { + it('when set to false', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableCardWidthSetting=false') + ); + expect(dataSource.getFeatures()).toEqual({ + enabledCardWidthSetting: false, + }); }); - expect(dataSource.getFeatures()).toEqual({ - enabledCardWidthSetting: true, + + it('when set to empty string', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableCardWidthSetting=') + ); + expect(dataSource.getFeatures()).toEqual({ + enabledCardWidthSetting: true, + }); }); - expect(dataSource.getFeatures()).toEqual({ - enabledCardWidthSetting: true, + + it('when set to true', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableCardWidthSetting=true') + ); + expect(dataSource.getFeatures()).toEqual({ + enabledCardWidthSetting: true, + }); }); - expect(dataSource.getFeatures()).toEqual({ - enabledCardWidthSetting: true, + + it('when set to an arbitrary string', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableCardWidthSetting=foo') + ); + expect(dataSource.getFeatures()).toEqual({ + enabledCardWidthSetting: true, + }); }); }); - it('returns forceSVG from the query params', () => { - getParamsSpy.and.returnValues( - new URLSearchParams('forceSVG=false'), - new URLSearchParams('forceSVG='), - new URLSearchParams('forceSVG=true'), - new URLSearchParams('forceSVG=foo') - ); - - expect(dataSource.getFeatures()).toEqual({ - forceSvg: false, + describe('returns forceSvg from the query params', () => { + it('when set to false', () => { + getParamsSpy.and.returnValue(new URLSearchParams('forceSVG=false')); + expect(dataSource.getFeatures()).toEqual({forceSvg: false}); }); - expect(dataSource.getFeatures()).toEqual({ - forceSvg: true, + + it('when set to empty string', () => { + getParamsSpy.and.returnValue(new URLSearchParams('forceSVG=')); + expect(dataSource.getFeatures()).toEqual({forceSvg: true}); }); - expect(dataSource.getFeatures()).toEqual({ - forceSvg: true, + + it('when set to true', () => { + getParamsSpy.and.returnValue(new URLSearchParams('forceSVG=true')); + expect(dataSource.getFeatures()).toEqual({forceSvg: true}); }); - expect(dataSource.getFeatures()).toEqual({ - forceSvg: true, + + it('when set to an arbitrary string', () => { + getParamsSpy.and.returnValue(new URLSearchParams('forceSVG=foo')); + expect(dataSource.getFeatures()).toEqual({forceSvg: true}); }); }); - it('returns enableDataTable from the query params', () => { - getParamsSpy.and.returnValues( - new URLSearchParams('enableDataTable=false'), - new URLSearchParams('enableDataTable='), - new URLSearchParams('enableDataTable=true'), - new URLSearchParams('enableDataTable=foo') - ); - expect(dataSource.getFeatures()).toEqual({ - enabledScalarDataTable: false, + describe('returns enabledDataTable from the query params', () => { + it('when set to false', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableDataTable=false') + ); + expect(dataSource.getFeatures()).toEqual({ + enabledScalarDataTable: false, + }); }); - expect(dataSource.getFeatures()).toEqual({ - enabledScalarDataTable: true, + + it('when set to empty string', () => { + getParamsSpy.and.returnValue(new URLSearchParams('enableDataTable=')); + expect(dataSource.getFeatures()).toEqual({ + enabledScalarDataTable: true, + }); }); - expect(dataSource.getFeatures()).toEqual({ - enabledScalarDataTable: true, + + it('when set to true', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableDataTable=true') + ); + expect(dataSource.getFeatures()).toEqual({ + enabledScalarDataTable: true, + }); }); - expect(dataSource.getFeatures()).toEqual({ - enabledScalarDataTable: true, + + it('when set to an arbitrary string', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableDataTable=foo') + ); + expect(dataSource.getFeatures()).toEqual({ + enabledScalarDataTable: true, + }); }); }); @@ -215,7 +289,7 @@ describe('tb_feature_flag_data_source', () => { ); expect(dataSource.getFeatures()).toEqual({ enabledExperimentalPlugins: ['a'], - inColab: false, + inColab: true, scalarsBatchSize: 16, }); }); diff --git a/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source_types.ts b/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source_types.ts index 4e4516aec84..8fd6ea01ef9 100644 --- a/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source_types.ts +++ b/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source_types.ts @@ -29,22 +29,3 @@ export abstract class TBFeatureFlagDataSource { */ abstract getFeatures(enableMediaQuery?: boolean): Partial; } - -export const EXPERIMENTAL_PLUGIN_QUERY_PARAM_KEY = 'experimentalPlugin'; - -export const SCALARS_BATCH_SIZE_PARAM_KEY = 'scalarsBatchSize'; - -export const ENABLE_CARD_WIDTH_SETTING_PARAM_KEY = 'enableCardWidthSetting'; - -export const ENABLE_COLOR_GROUP_QUERY_PARAM_KEY = 'enableColorGroup'; - -export const ENABLE_COLOR_GROUP_BY_REGEX_QUERY_PARAM_KEY = - 'enableColorGroupByRegex'; - -export const ENABLE_DARK_MODE_QUERY_PARAM_KEY = 'darkMode'; - -export const ENABLE_LINKED_TIME_PARAM_KEY = 'enableLinkedTime'; - -export const FORCE_SVG_RENDERER = 'forceSVG'; - -export const ENABLE_DATA_TABLE_PARAM_KEY = 'enableDataTable';