From a1d51cb5651ce692cb972ada0282aff7a42561c2 Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Mon, 11 Jul 2022 11:18:17 -0700 Subject: [PATCH 01/12] Stop removing query parameters when persisting application state to the url --- tensorboard/webapp/routes/BUILD | 22 ++++ .../routes/dashboard_deeplink_provider.ts | 38 +----- .../webapp/routes/feature_flag_serializer.ts | 57 +++++++++ .../routes/feature_flag_serializer_test.ts | 85 +++++++++++++ tensorboard/webapp/webapp_data_source/BUILD | 15 +++ .../tb_feature_flag_data_source.ts | 94 +++++--------- .../tb_feature_flag_query_parameters.ts | 117 ++++++++++++++++++ .../tb_feature_flag_query_parameters_test.ts | 48 +++++++ 8 files changed, 379 insertions(+), 97 deletions(-) create mode 100644 tensorboard/webapp/routes/feature_flag_serializer.ts create mode 100644 tensorboard/webapp/routes/feature_flag_serializer_test.ts create mode 100644 tensorboard/webapp/webapp_data_source/tb_feature_flag_query_parameters.ts create mode 100644 tensorboard/webapp/webapp_data_source/tb_feature_flag_query_parameters_test.ts diff --git a/tensorboard/webapp/routes/BUILD b/tensorboard/webapp/routes/BUILD index 7eacfcf7134..63912b20964 100644 --- a/tensorboard/webapp/routes/BUILD +++ b/tensorboard/webapp/routes/BUILD @@ -37,6 +37,7 @@ 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", @@ -53,6 +54,24 @@ tf_ts_library( ], ) +tf_ts_library( + name = "feature_flag_serializer", + srcs = [ + "feature_flag_serializer.ts", + ], + deps = [ + "//tensorboard/webapp:app_state", + "//tensorboard/webapp:selectors", + "//tensorboard/webapp/app_routing:location", + "//tensorboard/webapp/app_routing:types", + "//tensorboard/webapp/webapp_data_source:feature_flag_query_parameters", + "//tensorboard/webapp/webapp_data_source:feature_flag_types", + "@npm//@angular/core", + "@npm//@ngrx/store", + "@npm//rxjs", + ], +) + tf_ts_library( name = "testing", testonly = True, @@ -69,10 +88,13 @@ 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_routing:location", "//tensorboard/webapp:app_state", "//tensorboard/webapp:selectors", "//tensorboard/webapp/angular:expect_angular_core_testing", diff --git a/tensorboard/webapp/routes/dashboard_deeplink_provider.ts b/tensorboard/webapp/routes/dashboard_deeplink_provider.ts index 80d594e3988..39b7d7560e3 100644 --- a/tensorboard/webapp/routes/dashboard_deeplink_provider.ts +++ b/tensorboard/webapp/routes/dashboard_deeplink_provider.ts @@ -27,11 +27,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 +35,7 @@ import { SMOOTHING_KEY, TAG_FILTER_KEY, } from './dashboard_deeplink_provider_types'; +import {getFeatureFlagStates} from './feature_flag_serializer'; const COLOR_GROUP_REGEX_VALUE_PREFIX = 'regex:'; @@ -83,36 +79,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 +92,7 @@ export class DashboardDeepLinkProvider extends DeepLinkProvider { return [{key: TAG_FILTER_KEY, value: filterText}]; }) ), - this.getFeatureFlagStates(store), + getFeatureFlagStates(store), store.select(selectors.getMetricsSettingOverrides).pipe( map((settingOverrides) => { if (Number.isFinite(settingOverrides.scalarSmoothing)) { diff --git a/tensorboard/webapp/routes/feature_flag_serializer.ts b/tensorboard/webapp/routes/feature_flag_serializer.ts new file mode 100644 index 00000000000..1366bbc99ac --- /dev/null +++ b/tensorboard/webapp/routes/feature_flag_serializer.ts @@ -0,0 +1,57 @@ +/* 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 {Store} from '@ngrx/store'; +import {Observable} from 'rxjs'; +import {map} from 'rxjs/operators'; + +import {Location} from '../app_routing/location'; +import {SerializableQueryParams} from '../app_routing/types'; +import {State} from '../app_state'; +import * as selectors from '../selectors'; +import {EXPERIMENTAL_PLUGIN_QUERY_PARAM_KEY} from '../webapp_data_source/tb_feature_flag_data_source_types'; +import {FeatureFlagMetadata, FeatureFlagQueryParameters,} from '../webapp_data_source/tb_feature_flag_query_parameters'; + +export function getFeatureFlagStates(store: Store): + Observable { + return store.select(selectors.getEnabledExperimentalPlugins) + .pipe(map((experimentalPlugins) => { + const queryParams = experimentalPlugins.map((pluginId) => { + return {key: EXPERIMENTAL_PLUGIN_QUERY_PARAM_KEY, value: pluginId}; + }); + + const currentQueryParams = Object.fromEntries( + serializableQueryParamsToEntries(new Location().getSearch() || [])); + + Object.values(FeatureFlagQueryParameters) + .forEach((overriddenFeatureFlag: FeatureFlagMetadata) => { + const queryParamOverride = + overriddenFeatureFlag.queryParamOverride; + if (queryParamOverride && + queryParamOverride in currentQueryParams) { + queryParams.push({ + key: queryParamOverride, + value: currentQueryParams[queryParamOverride], + }); + } + }); + + return queryParams; + + function serializableQueryParamsToEntries( + params: SerializableQueryParams): [string, string][] { + return params.map(({key, value}) => [key, value]); + } + })); +} 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..80cbb409080 --- /dev/null +++ b/tensorboard/webapp/routes/feature_flag_serializer_test.ts @@ -0,0 +1,85 @@ +/* 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 {TestBed} from '@angular/core/testing'; +import {Store} from '@ngrx/store'; +import {MockStore, provideMockStore} from '@ngrx/store/testing'; + +import {Location} from '../app_routing/location'; +import {SerializableQueryParams} from '../app_routing/types'; +import {State} from '../app_state'; +import {appStateFromMetricsState, buildMetricsState} from '../metrics/testing'; +import * as selectors from '../selectors'; + +import {getFeatureFlagStates} from './feature_flag_serializer'; + +describe('feature flag serializer', () => { + let store: MockStore; + let location: Location; + let getSearchSpy: jasmine.Spy; + + beforeEach(async () => { + await TestBed + .configureTestingModule({ + providers: [ + provideMockStore({ + initialState: { + ...appStateFromMetricsState(buildMetricsState()), + }, + }), + ], + }) + .compileComponents(); + + store = TestBed.inject>(Store) as MockStore; + store.overrideSelector(selectors.getEnabledExperimentalPlugins, []); + + location = TestBed.inject(Location); + getSearchSpy = spyOn(location, 'getSearch').and.returnValue([]); + }); + + describe('getFeatureFlagStates', () => { + it('returns empty list when no feature flags are active', async () => { + const queryParams = await promiseGetFeatureFlagStates(store); + expect(queryParams.length).toEqual(0); + }); + + it('persists values of enabled experimental plugins', () => {}); + + it('persists flag states overridden by query params', async () => { + store.overrideSelector(selectors.getEnabledExperimentalPlugins, []); + getSearchSpy = spyOn(location, 'getSearch').and.returnValue([ + { + key: 'defaultEnableDarkMode', + value: 'true', + }, + ]); + const queryParams = await promiseGetFeatureFlagStates(store); + expect(queryParams.length).toEqual(1); + expect(queryParams[0].key).toEqual('defaultEnableDarkMode'); + expect(queryParams[0].value).toEqual('true'); + }); + }); +}); + +function promiseGetFeatureFlagStates(store: Store): + Promise { + return new Promise((resolve) => { + getFeatureFlagStates(store) + .subscribe((queryParams) => { + resolve(queryParams); + }) + .unsubscribe(); + }); +} diff --git a/tensorboard/webapp/webapp_data_source/BUILD b/tensorboard/webapp/webapp_data_source/BUILD index 575a40ea7e4..7762024befe 100644 --- a/tensorboard/webapp/webapp_data_source/BUILD +++ b/tensorboard/webapp/webapp_data_source/BUILD @@ -77,6 +77,18 @@ tf_ng_module( ], ) +tf_ng_module( + name = "feature_flag_query_parameters", + srcs = [ + "tb_feature_flag_query_parameters.ts", + ], + deps = [ + ":feature_flag_types", + "//tensorboard/webapp/feature_flag:types", + "@npm//@angular/core", + ], +) + tf_ng_module( name = "feature_flag_types", srcs = [ @@ -96,6 +108,7 @@ tf_ng_module( "tb_feature_flag_module.ts", ], deps = [ + ":feature_flag_query_parameters", ":feature_flag_types", "//tensorboard/webapp/feature_flag:types", "//tensorboard/webapp/types", @@ -124,9 +137,11 @@ tf_ng_module( testonly = True, srcs = [ "tb_feature_flag_data_source_test.ts", + "tb_feature_flag_query_parameters_test.ts", ], deps = [ ":feature_flag", + ":feature_flag_query_parameters", "//tensorboard/webapp/angular:expect_angular_core_testing", "@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..3896a1a2fda 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 @@ -15,18 +15,13 @@ limitations under the License. import {Injectable} from '@angular/core'; import {FeatureFlags} from '../feature_flag/types'; import {QueryParams} from './query_params'; +import {TBFeatureFlagDataSource} from './tb_feature_flag_data_source_types'; 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'; + BaseFeatureFlagType, + FeatureFlagMetadata, + FeatureFlagQueryParameters, + FeatureFlagType, +} from './tb_feature_flag_query_parameters'; const DARK_MODE_MEDIA_QUERY = '(prefers-color-scheme: dark)'; @@ -40,62 +35,39 @@ 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'; - } + const featureFlags: Partial> = + enableMediaQuery ? this.getPartialFeaturesFromMediaQuery() : {}; + Object.entries(FeatureFlagQueryParameters).forEach( + ([flagName, flagMetadata]) => { + const featureValue = this.getFeatureValue(flagMetadata); + if (featureValue !== null) { + const f = flagName as keyof FeatureFlags; + featureFlags[f] = featureValue; + } + } + ); + return featureFlags as Partial; + } - if (params.has(FORCE_SVG_RENDERER)) { - featureFlags.forceSvg = params.get(FORCE_SVG_RENDERER) !== 'false'; + protected getFeatureValue( + flagMetadata: FeatureFlagMetadata + ): FeatureFlagType { + const params = this.queryParams.getParams(); + const queryParamOverride = flagMetadata.queryParamOverride; + if (!queryParamOverride || !params.has(queryParamOverride)) { + return null; } - - if (params.has(ENABLE_DATA_TABLE_PARAM_KEY)) { - featureFlags.enabledScalarDataTable = - params.get(ENABLE_DATA_TABLE_PARAM_KEY) !== 'false'; + const paramValues: BaseFeatureFlagType[] = this.queryParams + .getParams() + .getAll(queryParamOverride) + .map(flagMetadata.parseValue); + if (!paramValues.length) { + return null; } - - return featureFlags; + return paramValues.length > 1 ? paramValues : paramValues[0]; } protected getPartialFeaturesFromMediaQuery(): { diff --git a/tensorboard/webapp/webapp_data_source/tb_feature_flag_query_parameters.ts b/tensorboard/webapp/webapp_data_source/tb_feature_flag_query_parameters.ts new file mode 100644 index 00000000000..852a02b4ed4 --- /dev/null +++ b/tensorboard/webapp/webapp_data_source/tb_feature_flag_query_parameters.ts @@ -0,0 +1,117 @@ +/* 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 '../feature_flag/types'; +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, +} from './tb_feature_flag_data_source_types'; + +export type BaseFeatureFlagType = Boolean | Number | String | null | undefined; + +export type FeatureFlagType = BaseFeatureFlagType | Array; + +export type FeatureFlagMetadata = { + displayName: string; + queryParamOverride?: string; + parseValue: (str: string) => BaseFeatureFlagType; +}; + +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 FeatureFlagQueryParameters: { + [FlagName in keyof FeatureFlags]: FeatureFlagMetadata; +} = { + scalarsBatchSize: { + displayName: 'scalarsBatchSize', + queryParamOverride: SCALARS_BATCH_SIZE_PARAM_KEY, + parseValue: parseInt, + }, + enabledColorGroup: { + displayName: 'enabledColorGroup', + queryParamOverride: ENABLE_COLOR_GROUP_QUERY_PARAM_KEY, + parseValue: parseBoolean, + }, + enabledColorGroupByRegex: { + displayName: 'enabledColorGroupByRegex', + queryParamOverride: ENABLE_COLOR_GROUP_BY_REGEX_QUERY_PARAM_KEY, + parseValue: parseBoolean, + }, + enabledExperimentalPlugins: { + displayName: 'enabledExperimentalPlugins', + queryParamOverride: EXPERIMENTAL_PLUGIN_QUERY_PARAM_KEY, + parseValue: (str) => str, + }, + enabledLinkedTime: { + displayName: 'enabledLinkedTime', + queryParamOverride: ENABLE_LINKED_TIME_PARAM_KEY, + parseValue: parseBoolean, + }, + enabledCardWidthSetting: { + displayName: 'enabledCardWidthSetting', + queryParamOverride: ENABLE_CARD_WIDTH_SETTING_PARAM_KEY, + parseValue: parseBoolean, + }, + enabledScalarDataTable: { + displayName: 'enabledScalarDataTable', + queryParamOverride: ENABLE_DATA_TABLE_PARAM_KEY, + parseValue: parseBoolean, + }, + forceSvg: { + displayName: 'forceSvg', + queryParamOverride: FORCE_SVG_RENDERER, + parseValue: parseBoolean, + }, + enableDarkModeOverride: { + displayName: 'enableDarkModeOverride', + parseValue: parseBooleanOrNull, + }, + defaultEnableDarkMode: { + displayName: 'defaultEnableDarkMode', + queryParamOverride: ENABLE_DARK_MODE_QUERY_PARAM_KEY, + parseValue: parseBoolean, + }, + isAutoDarkModeAllowed: { + displayName: 'isAutoDarkModeAllowed', + parseValue: parseBoolean, + }, + inColab: { + displayName: 'inColab', + parseValue: parseBoolean, + }, + metricsImageSupportEnabled: { + displayName: 'metricsImageSupportEnabled', + parseValue: parseBoolean, + }, + enableTimeSeriesPromotion: { + displayName: 'enableTimeSeriesPromotion', + parseValue: parseBoolean, + }, +}; diff --git a/tensorboard/webapp/webapp_data_source/tb_feature_flag_query_parameters_test.ts b/tensorboard/webapp/webapp_data_source/tb_feature_flag_query_parameters_test.ts new file mode 100644 index 00000000000..3028d955514 --- /dev/null +++ b/tensorboard/webapp/webapp_data_source/tb_feature_flag_query_parameters_test.ts @@ -0,0 +1,48 @@ +import { + parseBoolean, + parseBooleanOrNull, +} from './tb_feature_flag_query_parameters'; + +/* 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(); + }); + }); +}); From 444ad485cdad7ea2ff47615289622048b131ac6c Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Thu, 14 Jul 2022 16:31:21 -0700 Subject: [PATCH 02/12] address pr comments --- tensorboard/webapp/feature_flag/store/BUILD | 14 +++++++++++ .../store/feature_flag_metadata.ts} | 20 ++++++++-------- .../store/feature_flag_metadata_test.ts} | 2 +- tensorboard/webapp/routes/BUILD | 6 +++-- .../routes/dashboard_deeplink_provider.ts | 3 ++- .../webapp/routes/feature_flag_serializer.ts | 6 ++--- .../routes/feature_flag_serializer_test.ts | 3 ++- tensorboard/webapp/webapp_data_source/BUILD | 17 ++------------ .../tb_feature_flag_data_source.ts | 23 +++++++++---------- 9 files changed, 49 insertions(+), 45 deletions(-) rename tensorboard/webapp/{webapp_data_source/tb_feature_flag_query_parameters.ts => feature_flag/store/feature_flag_metadata.ts} (85%) rename tensorboard/webapp/{webapp_data_source/tb_feature_flag_query_parameters_test.ts => feature_flag/store/feature_flag_metadata_test.ts} (97%) diff --git a/tensorboard/webapp/feature_flag/store/BUILD b/tensorboard/webapp/feature_flag/store/BUILD index 72677626fb5..cb1fb338a14 100644 --- a/tensorboard/webapp/feature_flag/store/BUILD +++ b/tensorboard/webapp/feature_flag/store/BUILD @@ -21,6 +21,18 @@ tf_ng_module( ], ) +tf_ng_module( + name = "feature_flag_metadata", + srcs = [ + "feature_flag_metadata.ts", + ], + deps = [ + "//tensorboard/webapp/feature_flag:types", + "//tensorboard/webapp/webapp_data_source: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/webapp_data_source/tb_feature_flag_query_parameters.ts b/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts similarity index 85% rename from tensorboard/webapp/webapp_data_source/tb_feature_flag_query_parameters.ts rename to tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts index 852a02b4ed4..c19f787f121 100644 --- a/tensorboard/webapp/webapp_data_source/tb_feature_flag_query_parameters.ts +++ b/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts @@ -12,7 +12,6 @@ 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 '../feature_flag/types'; import { ENABLE_CARD_WIDTH_SETTING_PARAM_KEY, ENABLE_COLOR_GROUP_BY_REGEX_QUERY_PARAM_KEY, @@ -23,31 +22,32 @@ import { EXPERIMENTAL_PLUGIN_QUERY_PARAM_KEY, FORCE_SVG_RENDERER, SCALARS_BATCH_SIZE_PARAM_KEY, -} from './tb_feature_flag_data_source_types'; +} from '../../webapp_data_source/tb_feature_flag_data_source_types'; +import {FeatureFlags} from '../types'; -export type BaseFeatureFlagType = Boolean | Number | String | null | undefined; +export type BaseFeatureFlagType = boolean | number | string | null | undefined; export type FeatureFlagType = BaseFeatureFlagType | Array; -export type FeatureFlagMetadata = { +export type FeatureFlagMetadata = { displayName: string; queryParamOverride?: string; - parseValue: (str: string) => BaseFeatureFlagType; + parseValue: (str: string) => T; }; -export function parseBoolean(str: string): Boolean { +export function parseBoolean(str: string): boolean { return str !== 'false'; } -export function parseBooleanOrNull(str: string): Boolean | null { +export function parseBooleanOrNull(str: string): boolean | null { if (str === 'null') { return null; } return parseBoolean(str); } -export const FeatureFlagQueryParameters: { - [FlagName in keyof FeatureFlags]: FeatureFlagMetadata; +export const FeatureFlagMetadataMap: { + [FlagName in keyof FeatureFlags]: FeatureFlagMetadata; } = { scalarsBatchSize: { displayName: 'scalarsBatchSize', @@ -67,7 +67,7 @@ export const FeatureFlagQueryParameters: { enabledExperimentalPlugins: { displayName: 'enabledExperimentalPlugins', queryParamOverride: EXPERIMENTAL_PLUGIN_QUERY_PARAM_KEY, - parseValue: (str) => str, + parseValue: (str: string) => [str], }, enabledLinkedTime: { displayName: 'enabledLinkedTime', diff --git a/tensorboard/webapp/webapp_data_source/tb_feature_flag_query_parameters_test.ts b/tensorboard/webapp/feature_flag/store/feature_flag_metadata_test.ts similarity index 97% rename from tensorboard/webapp/webapp_data_source/tb_feature_flag_query_parameters_test.ts rename to tensorboard/webapp/feature_flag/store/feature_flag_metadata_test.ts index 3028d955514..ac11806fdf6 100644 --- a/tensorboard/webapp/webapp_data_source/tb_feature_flag_query_parameters_test.ts +++ b/tensorboard/webapp/feature_flag/store/feature_flag_metadata_test.ts @@ -1,7 +1,7 @@ import { parseBoolean, parseBooleanOrNull, -} from './tb_feature_flag_query_parameters'; +} from './feature_flag_metadata'; /* Copyright 2022 The TensorFlow Authors. All Rights Reserved. diff --git a/tensorboard/webapp/routes/BUILD b/tensorboard/webapp/routes/BUILD index 63912b20964..4a52d12047b 100644 --- a/tensorboard/webapp/routes/BUILD +++ b/tensorboard/webapp/routes/BUILD @@ -43,6 +43,7 @@ tf_ts_library( "//tensorboard/webapp/app_routing:deep_link_provider", "//tensorboard/webapp/app_routing:route_config", "//tensorboard/webapp/app_routing:types", + "//tensorboard/webapp/feature_flag/store:feature_flag_metadata", "//tensorboard/webapp/metrics:types", "//tensorboard/webapp/metrics/data_source:types", "//tensorboard/webapp/runs:types", @@ -64,7 +65,7 @@ tf_ts_library( "//tensorboard/webapp:selectors", "//tensorboard/webapp/app_routing:location", "//tensorboard/webapp/app_routing:types", - "//tensorboard/webapp/webapp_data_source:feature_flag_query_parameters", + "//tensorboard/webapp/feature_flag/store:feature_flag_metadata", "//tensorboard/webapp/webapp_data_source:feature_flag_types", "@npm//@angular/core", "@npm//@ngrx/store", @@ -94,13 +95,14 @@ tf_ts_library( ":dashboard_deeplink_provider", ":feature_flag_serializer", ":testing", - "//tensorboard/webapp/app_routing:location", "//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 39b7d7560e3..5c80bc1112f 100644 --- a/tensorboard/webapp/routes/dashboard_deeplink_provider.ts +++ b/tensorboard/webapp/routes/dashboard_deeplink_provider.ts @@ -19,6 +19,7 @@ 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, FeatureFlagType, FeatureFlagMetadataMap} from '../feature_flag/store/feature_flag_metadata'; import { isPluginType, isSampledPlugin, @@ -92,7 +93,7 @@ export class DashboardDeepLinkProvider extends DeepLinkProvider { return [{key: TAG_FILTER_KEY, value: filterText}]; }) ), - getFeatureFlagStates(store), + getFeatureFlagStates(store, FeatureFlagMetadataMap as Record>), store.select(selectors.getMetricsSettingOverrides).pipe( map((settingOverrides) => { if (Number.isFinite(settingOverrides.scalarSmoothing)) { diff --git a/tensorboard/webapp/routes/feature_flag_serializer.ts b/tensorboard/webapp/routes/feature_flag_serializer.ts index 1366bbc99ac..7641d637942 100644 --- a/tensorboard/webapp/routes/feature_flag_serializer.ts +++ b/tensorboard/webapp/routes/feature_flag_serializer.ts @@ -19,11 +19,11 @@ import {map} from 'rxjs/operators'; import {Location} from '../app_routing/location'; import {SerializableQueryParams} from '../app_routing/types'; import {State} from '../app_state'; +import {FeatureFlagMetadata} from '../feature_flag/store/feature_flag_metadata'; import * as selectors from '../selectors'; import {EXPERIMENTAL_PLUGIN_QUERY_PARAM_KEY} from '../webapp_data_source/tb_feature_flag_data_source_types'; -import {FeatureFlagMetadata, FeatureFlagQueryParameters,} from '../webapp_data_source/tb_feature_flag_query_parameters'; -export function getFeatureFlagStates(store: Store): +export function getFeatureFlagStates(store: Store, featureFlagQueryParameters: Record>): Observable { return store.select(selectors.getEnabledExperimentalPlugins) .pipe(map((experimentalPlugins) => { @@ -34,7 +34,7 @@ export function getFeatureFlagStates(store: Store): const currentQueryParams = Object.fromEntries( serializableQueryParamsToEntries(new Location().getSearch() || [])); - Object.values(FeatureFlagQueryParameters) + Object.values(featureFlagQueryParameters) .forEach((overriddenFeatureFlag: FeatureFlagMetadata) => { const queryParamOverride = overriddenFeatureFlag.queryParamOverride; diff --git a/tensorboard/webapp/routes/feature_flag_serializer_test.ts b/tensorboard/webapp/routes/feature_flag_serializer_test.ts index 80cbb409080..7895dd2dc23 100644 --- a/tensorboard/webapp/routes/feature_flag_serializer_test.ts +++ b/tensorboard/webapp/routes/feature_flag_serializer_test.ts @@ -19,6 +19,7 @@ import {MockStore, provideMockStore} from '@ngrx/store/testing'; import {Location} from '../app_routing/location'; import {SerializableQueryParams} from '../app_routing/types'; import {State} from '../app_state'; +import {FeatureFlagMetadata, FeatureFlagType, FeatureFlagMetadataMap} from '../feature_flag/store/feature_flag_metadata'; import {appStateFromMetricsState, buildMetricsState} from '../metrics/testing'; import * as selectors from '../selectors'; @@ -76,7 +77,7 @@ describe('feature flag serializer', () => { function promiseGetFeatureFlagStates(store: Store): Promise { return new Promise((resolve) => { - getFeatureFlagStates(store) + getFeatureFlagStates(store, FeatureFlagMetadataMap as Record>) .subscribe((queryParams) => { resolve(queryParams); }) diff --git a/tensorboard/webapp/webapp_data_source/BUILD b/tensorboard/webapp/webapp_data_source/BUILD index 7762024befe..70966ad3a0f 100644 --- a/tensorboard/webapp/webapp_data_source/BUILD +++ b/tensorboard/webapp/webapp_data_source/BUILD @@ -77,18 +77,6 @@ tf_ng_module( ], ) -tf_ng_module( - name = "feature_flag_query_parameters", - srcs = [ - "tb_feature_flag_query_parameters.ts", - ], - deps = [ - ":feature_flag_types", - "//tensorboard/webapp/feature_flag:types", - "@npm//@angular/core", - ], -) - tf_ng_module( name = "feature_flag_types", srcs = [ @@ -108,9 +96,9 @@ tf_ng_module( "tb_feature_flag_module.ts", ], deps = [ - ":feature_flag_query_parameters", ":feature_flag_types", "//tensorboard/webapp/feature_flag:types", + "//tensorboard/webapp/feature_flag/store:feature_flag_metadata", "//tensorboard/webapp/types", "@npm//@angular/common", "@npm//@angular/core", @@ -137,12 +125,11 @@ tf_ng_module( testonly = True, srcs = [ "tb_feature_flag_data_source_test.ts", - "tb_feature_flag_query_parameters_test.ts", ], deps = [ ":feature_flag", - ":feature_flag_query_parameters", "//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 3896a1a2fda..5e21a369d5e 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,15 +13,14 @@ See the License for the specific language governing permissions and limitations under the License. ==============================================================================*/ import {Injectable} from '@angular/core'; -import {FeatureFlags} from '../feature_flag/types'; -import {QueryParams} from './query_params'; -import {TBFeatureFlagDataSource} from './tb_feature_flag_data_source_types'; import { - BaseFeatureFlagType, FeatureFlagMetadata, - FeatureFlagQueryParameters, + FeatureFlagMetadataMap, FeatureFlagType, -} from './tb_feature_flag_query_parameters'; +} from '../feature_flag/store/feature_flag_metadata'; +import {FeatureFlags} from '../feature_flag/types'; +import {QueryParams} from './query_params'; +import {TBFeatureFlagDataSource} from './tb_feature_flag_data_source_types'; const DARK_MODE_MEDIA_QUERY = '(prefers-color-scheme: dark)'; @@ -40,9 +39,9 @@ export class QueryParamsFeatureFlagDataSource // their values in the underlying state are not inadvertently changed. const featureFlags: Partial> = enableMediaQuery ? this.getPartialFeaturesFromMediaQuery() : {}; - Object.entries(FeatureFlagQueryParameters).forEach( + Object.entries(FeatureFlagMetadataMap).forEach( ([flagName, flagMetadata]) => { - const featureValue = this.getFeatureValue(flagMetadata); + const featureValue = this.getFeatureValue(flagMetadata as FeatureFlagMetadata); if (featureValue !== null) { const f = flagName as keyof FeatureFlags; featureFlags[f] = featureValue; @@ -52,15 +51,15 @@ export class QueryParamsFeatureFlagDataSource return featureFlags as Partial; } - protected getFeatureValue( - flagMetadata: FeatureFlagMetadata - ): FeatureFlagType { + protected getFeatureValue( + flagMetadata: FeatureFlagMetadata + ) { const params = this.queryParams.getParams(); const queryParamOverride = flagMetadata.queryParamOverride; if (!queryParamOverride || !params.has(queryParamOverride)) { return null; } - const paramValues: BaseFeatureFlagType[] = this.queryParams + const paramValues: T[] = this.queryParams .getParams() .getAll(queryParamOverride) .map(flagMetadata.parseValue); From f1b27170bd6bebb924a9371543e30c7e9644d065 Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Thu, 14 Jul 2022 17:22:26 -0700 Subject: [PATCH 03/12] simplify query param serialization logic --- .../routes/dashboard_deeplink_provider.ts | 4 +- .../webapp/routes/feature_flag_serializer.ts | 58 +++++++------------ .../routes/feature_flag_serializer_test.ts | 39 ++----------- 3 files changed, 27 insertions(+), 74 deletions(-) diff --git a/tensorboard/webapp/routes/dashboard_deeplink_provider.ts b/tensorboard/webapp/routes/dashboard_deeplink_provider.ts index 5c80bc1112f..ddbd318ef8b 100644 --- a/tensorboard/webapp/routes/dashboard_deeplink_provider.ts +++ b/tensorboard/webapp/routes/dashboard_deeplink_provider.ts @@ -36,7 +36,7 @@ import { SMOOTHING_KEY, TAG_FILTER_KEY, } from './dashboard_deeplink_provider_types'; -import {getFeatureFlagStates} from './feature_flag_serializer'; +import {getOverriddenFeatureFlagStates} from './feature_flag_serializer'; const COLOR_GROUP_REGEX_VALUE_PREFIX = 'regex:'; @@ -93,7 +93,7 @@ export class DashboardDeepLinkProvider extends DeepLinkProvider { return [{key: TAG_FILTER_KEY, value: filterText}]; }) ), - getFeatureFlagStates(store, FeatureFlagMetadataMap as Record>), + getOverriddenFeatureFlagStates(FeatureFlagMetadataMap as Record>), store.select(selectors.getMetricsSettingOverrides).pipe( map((settingOverrides) => { if (Number.isFinite(settingOverrides.scalarSmoothing)) { diff --git a/tensorboard/webapp/routes/feature_flag_serializer.ts b/tensorboard/webapp/routes/feature_flag_serializer.ts index 7641d637942..2cd955a0414 100644 --- a/tensorboard/webapp/routes/feature_flag_serializer.ts +++ b/tensorboard/webapp/routes/feature_flag_serializer.ts @@ -12,46 +12,28 @@ 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 {Store} from '@ngrx/store'; -import {Observable} from 'rxjs'; -import {map} from 'rxjs/operators'; - import {Location} from '../app_routing/location'; import {SerializableQueryParams} from '../app_routing/types'; -import {State} from '../app_state'; import {FeatureFlagMetadata} from '../feature_flag/store/feature_flag_metadata'; -import * as selectors from '../selectors'; -import {EXPERIMENTAL_PLUGIN_QUERY_PARAM_KEY} from '../webapp_data_source/tb_feature_flag_data_source_types'; - -export function getFeatureFlagStates(store: Store, featureFlagQueryParameters: Record>): - Observable { - return store.select(selectors.getEnabledExperimentalPlugins) - .pipe(map((experimentalPlugins) => { - const queryParams = experimentalPlugins.map((pluginId) => { - return {key: EXPERIMENTAL_PLUGIN_QUERY_PARAM_KEY, value: pluginId}; - }); - - const currentQueryParams = Object.fromEntries( - serializableQueryParamsToEntries(new Location().getSearch() || [])); - - Object.values(featureFlagQueryParameters) - .forEach((overriddenFeatureFlag: FeatureFlagMetadata) => { - const queryParamOverride = - overriddenFeatureFlag.queryParamOverride; - if (queryParamOverride && - queryParamOverride in currentQueryParams) { - queryParams.push({ - key: queryParamOverride, - value: currentQueryParams[queryParamOverride], - }); - } - }); - - return queryParams; - function serializableQueryParamsToEntries( - params: SerializableQueryParams): [string, string][] { - return params.map(({key, value}) => [key, value]); - } - })); +export function getOverriddenFeatureFlagStates(featureFlagQueryParameters: Record>): SerializableQueryParams { + const currentQueryParams = Object.fromEntries( + serializableQueryParamsToEntries(new Location().getSearch() || [])); + + const currentlyOverriddenQueryParams = Object.values(featureFlagQueryParameters) + .map(({queryParamOverride}: FeatureFlagMetadata) => queryParamOverride) + .filter((queryParamOverride) => + (queryParamOverride && + queryParamOverride in currentQueryParams)) as string[]; + return currentlyOverriddenQueryParams.map((queryParamOverride) => { + return { + key: queryParamOverride, + value: currentQueryParams[queryParamOverride], + }; + }); + + function serializableQueryParamsToEntries( + params: SerializableQueryParams): [string, string][] { + return params.map(({key, value}) => [key, value]); + } } diff --git a/tensorboard/webapp/routes/feature_flag_serializer_test.ts b/tensorboard/webapp/routes/feature_flag_serializer_test.ts index 7895dd2dc23..1fa1f82caee 100644 --- a/tensorboard/webapp/routes/feature_flag_serializer_test.ts +++ b/tensorboard/webapp/routes/feature_flag_serializer_test.ts @@ -13,20 +13,11 @@ See the License for the specific language governing permissions and limitations under the License. ==============================================================================*/ import {TestBed} from '@angular/core/testing'; -import {Store} from '@ngrx/store'; -import {MockStore, provideMockStore} from '@ngrx/store/testing'; - import {Location} from '../app_routing/location'; -import {SerializableQueryParams} from '../app_routing/types'; -import {State} from '../app_state'; import {FeatureFlagMetadata, FeatureFlagType, FeatureFlagMetadataMap} from '../feature_flag/store/feature_flag_metadata'; -import {appStateFromMetricsState, buildMetricsState} from '../metrics/testing'; -import * as selectors from '../selectors'; - -import {getFeatureFlagStates} from './feature_flag_serializer'; +import {getOverriddenFeatureFlagStates} from './feature_flag_serializer'; describe('feature flag serializer', () => { - let store: MockStore; let location: Location; let getSearchSpy: jasmine.Spy; @@ -34,53 +25,33 @@ describe('feature flag serializer', () => { await TestBed .configureTestingModule({ providers: [ - provideMockStore({ - initialState: { - ...appStateFromMetricsState(buildMetricsState()), - }, - }), ], }) .compileComponents(); - store = TestBed.inject>(Store) as MockStore; - store.overrideSelector(selectors.getEnabledExperimentalPlugins, []); - location = TestBed.inject(Location); getSearchSpy = spyOn(location, 'getSearch').and.returnValue([]); }); - describe('getFeatureFlagStates', () => { - it('returns empty list when no feature flags are active', async () => { - const queryParams = await promiseGetFeatureFlagStates(store); + describe('getOverriddenFeatureFlagStates', () => { + it('returns empty list when no feature flags are active', () => { + const queryParams = getOverriddenFeatureFlagStates(FeatureFlagMetadataMap as Record>); expect(queryParams.length).toEqual(0); }); it('persists values of enabled experimental plugins', () => {}); it('persists flag states overridden by query params', async () => { - store.overrideSelector(selectors.getEnabledExperimentalPlugins, []); getSearchSpy = spyOn(location, 'getSearch').and.returnValue([ { key: 'defaultEnableDarkMode', value: 'true', }, ]); - const queryParams = await promiseGetFeatureFlagStates(store); + const queryParams = getOverriddenFeatureFlagStates(FeatureFlagMetadataMap as Record>); expect(queryParams.length).toEqual(1); expect(queryParams[0].key).toEqual('defaultEnableDarkMode'); expect(queryParams[0].value).toEqual('true'); }); }); }); - -function promiseGetFeatureFlagStates(store: Store): - Promise { - return new Promise((resolve) => { - getFeatureFlagStates(store, FeatureFlagMetadataMap as Record>) - .subscribe((queryParams) => { - resolve(queryParams); - }) - .unsubscribe(); - }); -} From a4ecd92b6d01492ca779ad12c03b516726a92d29 Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Fri, 15 Jul 2022 09:37:39 -0700 Subject: [PATCH 04/12] force array flags to be specified --- .../store/feature_flag_metadata.ts | 2 ++ .../store/feature_flag_metadata_test.ts | 5 +-- .../routes/dashboard_deeplink_provider.ts | 13 ++++++-- .../webapp/routes/feature_flag_serializer.ts | 33 +++++++++++-------- .../routes/feature_flag_serializer_test.ts | 29 +++++++++++----- .../tb_feature_flag_data_source.ts | 11 ++++--- 6 files changed, 60 insertions(+), 33 deletions(-) diff --git a/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts b/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts index c19f787f121..9210ea04569 100644 --- a/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts +++ b/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts @@ -33,6 +33,7 @@ export type FeatureFlagMetadata = { displayName: string; queryParamOverride?: string; parseValue: (str: string) => T; + isArray?: boolean; }; export function parseBoolean(str: string): boolean { @@ -68,6 +69,7 @@ export const FeatureFlagMetadataMap: { displayName: 'enabledExperimentalPlugins', queryParamOverride: EXPERIMENTAL_PLUGIN_QUERY_PARAM_KEY, parseValue: (str: string) => [str], + isArray: true, }, enabledLinkedTime: { displayName: 'enabledLinkedTime', 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 ac11806fdf6..2831f09912f 100644 --- a/tensorboard/webapp/feature_flag/store/feature_flag_metadata_test.ts +++ b/tensorboard/webapp/feature_flag/store/feature_flag_metadata_test.ts @@ -1,7 +1,4 @@ -import { - parseBoolean, - parseBooleanOrNull, -} from './feature_flag_metadata'; +import {parseBoolean, parseBooleanOrNull} from './feature_flag_metadata'; /* Copyright 2022 The TensorFlow Authors. All Rights Reserved. diff --git a/tensorboard/webapp/routes/dashboard_deeplink_provider.ts b/tensorboard/webapp/routes/dashboard_deeplink_provider.ts index ddbd318ef8b..b3ad706c7a7 100644 --- a/tensorboard/webapp/routes/dashboard_deeplink_provider.ts +++ b/tensorboard/webapp/routes/dashboard_deeplink_provider.ts @@ -19,7 +19,11 @@ 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, FeatureFlagType, FeatureFlagMetadataMap} from '../feature_flag/store/feature_flag_metadata'; +import { + FeatureFlagMetadata, + FeatureFlagMetadataMap, + FeatureFlagType, +} from '../feature_flag/store/feature_flag_metadata'; import { isPluginType, isSampledPlugin, @@ -93,7 +97,12 @@ export class DashboardDeepLinkProvider extends DeepLinkProvider { return [{key: TAG_FILTER_KEY, value: filterText}]; }) ), - getOverriddenFeatureFlagStates(FeatureFlagMetadataMap as Record>), + getOverriddenFeatureFlagStates( + FeatureFlagMetadataMap as Record< + string, + FeatureFlagMetadata + > + ), store.select(selectors.getMetricsSettingOverrides).pipe( map((settingOverrides) => { if (Number.isFinite(settingOverrides.scalarSmoothing)) { diff --git a/tensorboard/webapp/routes/feature_flag_serializer.ts b/tensorboard/webapp/routes/feature_flag_serializer.ts index 2cd955a0414..249b33deb66 100644 --- a/tensorboard/webapp/routes/feature_flag_serializer.ts +++ b/tensorboard/webapp/routes/feature_flag_serializer.ts @@ -16,24 +16,31 @@ import {Location} from '../app_routing/location'; import {SerializableQueryParams} from '../app_routing/types'; import {FeatureFlagMetadata} from '../feature_flag/store/feature_flag_metadata'; -export function getOverriddenFeatureFlagStates(featureFlagQueryParameters: Record>): SerializableQueryParams { +export function getOverriddenFeatureFlagStates( + featureFlagQueryParameters: Record> +): SerializableQueryParams { const currentQueryParams = Object.fromEntries( - serializableQueryParamsToEntries(new Location().getSearch() || [])); + serializableQueryParamsToEntries(new Location().getSearch() || []) + ); - const currentlyOverriddenQueryParams = Object.values(featureFlagQueryParameters) - .map(({queryParamOverride}: FeatureFlagMetadata) => queryParamOverride) - .filter((queryParamOverride) => - (queryParamOverride && - queryParamOverride in currentQueryParams)) as string[]; + const currentlyOverriddenQueryParams = Object.values( + featureFlagQueryParameters + ) + .map(({queryParamOverride}: FeatureFlagMetadata) => queryParamOverride) + .filter( + (queryParamOverride) => + queryParamOverride && queryParamOverride in currentQueryParams + ) as string[]; return currentlyOverriddenQueryParams.map((queryParamOverride) => { - return { - key: queryParamOverride, - value: currentQueryParams[queryParamOverride], - }; - }); + return { + key: queryParamOverride, + value: currentQueryParams[queryParamOverride], + }; + }); function serializableQueryParamsToEntries( - params: SerializableQueryParams): [string, string][] { + params: SerializableQueryParams + ): [string, string][] { return params.map(({key, value}) => [key, value]); } } diff --git a/tensorboard/webapp/routes/feature_flag_serializer_test.ts b/tensorboard/webapp/routes/feature_flag_serializer_test.ts index 1fa1f82caee..b182ae65bd0 100644 --- a/tensorboard/webapp/routes/feature_flag_serializer_test.ts +++ b/tensorboard/webapp/routes/feature_flag_serializer_test.ts @@ -14,7 +14,11 @@ limitations under the License. ==============================================================================*/ import {TestBed} from '@angular/core/testing'; import {Location} from '../app_routing/location'; -import {FeatureFlagMetadata, FeatureFlagType, FeatureFlagMetadataMap} from '../feature_flag/store/feature_flag_metadata'; +import { + FeatureFlagMetadata, + FeatureFlagMetadataMap, + FeatureFlagType, +} from '../feature_flag/store/feature_flag_metadata'; import {getOverriddenFeatureFlagStates} from './feature_flag_serializer'; describe('feature flag serializer', () => { @@ -22,12 +26,9 @@ describe('feature flag serializer', () => { let getSearchSpy: jasmine.Spy; beforeEach(async () => { - await TestBed - .configureTestingModule({ - providers: [ - ], - }) - .compileComponents(); + await TestBed.configureTestingModule({ + providers: [], + }).compileComponents(); location = TestBed.inject(Location); getSearchSpy = spyOn(location, 'getSearch').and.returnValue([]); @@ -35,7 +36,12 @@ describe('feature flag serializer', () => { describe('getOverriddenFeatureFlagStates', () => { it('returns empty list when no feature flags are active', () => { - const queryParams = getOverriddenFeatureFlagStates(FeatureFlagMetadataMap as Record>); + const queryParams = getOverriddenFeatureFlagStates( + FeatureFlagMetadataMap as Record< + string, + FeatureFlagMetadata + > + ); expect(queryParams.length).toEqual(0); }); @@ -48,7 +54,12 @@ describe('feature flag serializer', () => { value: 'true', }, ]); - const queryParams = getOverriddenFeatureFlagStates(FeatureFlagMetadataMap as Record>); + const queryParams = getOverriddenFeatureFlagStates( + FeatureFlagMetadataMap as Record< + string, + FeatureFlagMetadata + > + ); expect(queryParams.length).toEqual(1); expect(queryParams[0].key).toEqual('defaultEnableDarkMode'); expect(queryParams[0].value).toEqual('true'); 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 5e21a369d5e..f32a0df8c5b 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 @@ -41,7 +41,9 @@ export class QueryParamsFeatureFlagDataSource enableMediaQuery ? this.getPartialFeaturesFromMediaQuery() : {}; Object.entries(FeatureFlagMetadataMap).forEach( ([flagName, flagMetadata]) => { - const featureValue = this.getFeatureValue(flagMetadata as FeatureFlagMetadata); + const featureValue = this.getFeatureValue( + flagMetadata as FeatureFlagMetadata + ); if (featureValue !== null) { const f = flagName as keyof FeatureFlags; featureFlags[f] = featureValue; @@ -51,9 +53,7 @@ export class QueryParamsFeatureFlagDataSource return featureFlags as Partial; } - protected getFeatureValue( - flagMetadata: FeatureFlagMetadata - ) { + protected getFeatureValue(flagMetadata: FeatureFlagMetadata) { const params = this.queryParams.getParams(); const queryParamOverride = flagMetadata.queryParamOverride; if (!queryParamOverride || !params.has(queryParamOverride)) { @@ -66,7 +66,8 @@ export class QueryParamsFeatureFlagDataSource if (!paramValues.length) { return null; } - return paramValues.length > 1 ? paramValues : paramValues[0]; + + return flagMetadata.isArray ? paramValues : paramValues[0]; } protected getPartialFeaturesFromMediaQuery(): { From 260b1052bb1b3ecf1837c161cab23bc30590c672 Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Fri, 15 Jul 2022 16:04:40 -0700 Subject: [PATCH 05/12] trying to fix some tests --- tensorboard/webapp/app_routing/location.ts | 5 +- .../store/feature_flag_metadata.ts | 3 + .../routes/dashboard_deeplink_provider.ts | 6 +- .../dashboard_deeplink_provider_test.ts | 71 ------------------- .../routes/feature_flag_serializer_test.ts | 33 +++++---- .../tb_feature_flag_data_source.ts | 7 +- 6 files changed, 37 insertions(+), 88 deletions(-) diff --git a/tensorboard/webapp/app_routing/location.ts b/tensorboard/webapp/app_routing/location.ts index 95057966453..520c4d8cf70 100644 --- a/tensorboard/webapp/app_routing/location.ts +++ b/tensorboard/webapp/app_routing/location.ts @@ -48,6 +48,9 @@ const utils = { getHref() { return window.location.href; }, + getSearch() { + return window.location.search; + } }; @Injectable() @@ -57,7 +60,7 @@ export class Location implements LocationInterface { } getSearch(): SerializableQueryParams { - const searchParams = new URLSearchParams(window.location.search); + const searchParams = new URLSearchParams(utils.getSearch()); const serializableSearchParams: SerializableQueryParams = []; // URLSearchParams is a Iterable but TypeScript does not know about that. diff --git a/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts b/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts index 9210ea04569..800bf0005b4 100644 --- a/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts +++ b/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts @@ -31,6 +31,7 @@ export type FeatureFlagType = BaseFeatureFlagType | Array; export type FeatureFlagMetadata = { displayName: string; + defaultValue?: T; queryParamOverride?: string; parseValue: (str: string) => T; isArray?: boolean; @@ -106,6 +107,8 @@ export const FeatureFlagMetadataMap: { }, inColab: { displayName: 'inColab', + defaultValue: false, + queryParamOverride: 'tensorboardColab', parseValue: parseBoolean, }, metricsImageSupportEnabled: { diff --git a/tensorboard/webapp/routes/dashboard_deeplink_provider.ts b/tensorboard/webapp/routes/dashboard_deeplink_provider.ts index b3ad706c7a7..eb5695de35e 100644 --- a/tensorboard/webapp/routes/dashboard_deeplink_provider.ts +++ b/tensorboard/webapp/routes/dashboard_deeplink_provider.ts @@ -14,7 +14,7 @@ limitations under the License. ==============================================================================*/ import {Injectable} from '@angular/core'; import {Store} from '@ngrx/store'; -import {combineLatest, Observable} from 'rxjs'; +import {combineLatest, of, Observable} from 'rxjs'; import {map} from 'rxjs/operators'; import {DeepLinkProvider} from '../app_routing/deep_link_provider'; import {SerializableQueryParams} from '../app_routing/types'; @@ -97,12 +97,12 @@ export class DashboardDeepLinkProvider extends DeepLinkProvider { return [{key: TAG_FILTER_KEY, value: filterText}]; }) ), - getOverriddenFeatureFlagStates( + of(getOverriddenFeatureFlagStates( 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..b57e7d41356 100644 --- a/tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts +++ b/tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts @@ -347,77 +347,6 @@ describe('core deeplink provider', () => { }); }); - describe('feature flag', () => { - it('serializes enabled experimental plugins', () => { - store.overrideSelector(selectors.getEnabledExperimentalPlugins, [ - 'foo', - 'bar', - 'baz', - ]); - store.refreshState(); - - expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([ - {key: 'experimentalPlugin', value: 'foo'}, - {key: 'experimentalPlugin', value: 'bar'}, - {key: 'experimentalPlugin', value: 'baz'}, - ]); - }); - - it('serializes enableColorGroup state', () => { - store.overrideSelector(selectors.getOverriddenFeatureFlags, { - enabledColorGroup: true, - }); - store.refreshState(); - - expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([ - {key: 'enableColorGroup', value: 'true'}, - ]); - - store.overrideSelector(selectors.getOverriddenFeatureFlags, { - enabledColorGroup: false, - }); - store.refreshState(); - - expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([ - {key: 'enableColorGroup', value: 'false'}, - ]); - - store.overrideSelector(selectors.getOverriddenFeatureFlags, {}); - store.refreshState(); - - expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual( - [] - ); - }); - - it('serializes enableColorGroupByRegex state', () => { - store.overrideSelector(selectors.getOverriddenFeatureFlags, { - enabledColorGroupByRegex: true, - }); - store.refreshState(); - - expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([ - {key: 'enableColorGroupByRegex', value: 'true'}, - ]); - - store.overrideSelector(selectors.getOverriddenFeatureFlags, { - enabledColorGroupByRegex: false, - }); - store.refreshState(); - - expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([ - {key: 'enableColorGroupByRegex', value: 'false'}, - ]); - - store.overrideSelector(selectors.getOverriddenFeatureFlags, {}); - store.refreshState(); - - expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual( - [] - ); - }); - }); - describe('runs', () => { describe('color group', () => { it('does not put state in the URL when user set color group is null', () => { diff --git a/tensorboard/webapp/routes/feature_flag_serializer_test.ts b/tensorboard/webapp/routes/feature_flag_serializer_test.ts index b182ae65bd0..985f19d533d 100644 --- a/tensorboard/webapp/routes/feature_flag_serializer_test.ts +++ b/tensorboard/webapp/routes/feature_flag_serializer_test.ts @@ -13,7 +13,7 @@ See the License for the specific language governing permissions and limitations under the License. ==============================================================================*/ import {TestBed} from '@angular/core/testing'; -import {Location} from '../app_routing/location'; +import {Location, TEST_ONLY} from '../app_routing/location'; import { FeatureFlagMetadata, FeatureFlagMetadataMap, @@ -23,15 +23,13 @@ import {getOverriddenFeatureFlagStates} from './feature_flag_serializer'; describe('feature flag serializer', () => { let location: Location; - let getSearchSpy: jasmine.Spy; beforeEach(async () => { await TestBed.configureTestingModule({ - providers: [], + providers: [Location], }).compileComponents(); location = TestBed.inject(Location); - getSearchSpy = spyOn(location, 'getSearch').and.returnValue([]); }); describe('getOverriddenFeatureFlagStates', () => { @@ -45,15 +43,26 @@ describe('feature flag serializer', () => { expect(queryParams.length).toEqual(0); }); - it('persists values of enabled experimental plugins', () => {}); + it('persists values of enabled experimental plugins', () => { + spyOn(TEST_ONLY.utils, 'getSearch').and.returnValue( + '?experimentalPlugin=0&experimentalPlugin=1&experimentalPlugin=2' + ); + const queryParams = getOverriddenFeatureFlagStates( + FeatureFlagMetadataMap as Record< + string, + FeatureFlagMetadata + > + ); + expect(queryParams.length).toEqual(3); + expect(queryParams[0].key).toEqual('experimentalPlugin'); + expect(queryParams[0].value).toEqual('0'); + }); it('persists flag states overridden by query params', async () => { - getSearchSpy = spyOn(location, 'getSearch').and.returnValue([ - { - key: 'defaultEnableDarkMode', - value: 'true', - }, - ]); + spyOn(TEST_ONLY.utils, 'getSearch').and.returnValue( + '?darkMode=true' + ); + console.log(location.getSearch()); const queryParams = getOverriddenFeatureFlagStates( FeatureFlagMetadataMap as Record< string, @@ -61,7 +70,7 @@ describe('feature flag serializer', () => { > ); expect(queryParams.length).toEqual(1); - expect(queryParams[0].key).toEqual('defaultEnableDarkMode'); + expect(queryParams[0].key).toEqual('darkMode'); expect(queryParams[0].value).toEqual('true'); }); }); 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 f32a0df8c5b..92de9375ab1 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 @@ -62,7 +62,12 @@ export class QueryParamsFeatureFlagDataSource const paramValues: T[] = this.queryParams .getParams() .getAll(queryParamOverride) - .map(flagMetadata.parseValue); + .map((value) => { + if (value === '' && flagMetadata.defaultValue !== undefined) { + return flagMetadata.defaultValue; + } + return flagMetadata.parseValue(value); + }); if (!paramValues.length) { return null; } From 5325f802cecab3a4ee4a207b034771216cb2be36 Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Mon, 18 Jul 2022 13:34:20 -0700 Subject: [PATCH 06/12] rewrite tests to avoid using returnValues which for some reason fails --- tensorboard/webapp/app_routing/location.ts | 2 +- .../routes/dashboard_deeplink_provider.ts | 16 +- .../webapp/routes/feature_flag_serializer.ts | 53 ++-- .../routes/feature_flag_serializer_test.ts | 4 +- .../tb_feature_flag_data_source.ts | 17 +- .../tb_feature_flag_data_source_test.ts | 250 ++++++++++++------ 6 files changed, 215 insertions(+), 127 deletions(-) diff --git a/tensorboard/webapp/app_routing/location.ts b/tensorboard/webapp/app_routing/location.ts index 520c4d8cf70..bf1cd56fa02 100644 --- a/tensorboard/webapp/app_routing/location.ts +++ b/tensorboard/webapp/app_routing/location.ts @@ -50,7 +50,7 @@ const utils = { }, getSearch() { return window.location.search; - } + }, }; @Injectable() diff --git a/tensorboard/webapp/routes/dashboard_deeplink_provider.ts b/tensorboard/webapp/routes/dashboard_deeplink_provider.ts index eb5695de35e..8333de18975 100644 --- a/tensorboard/webapp/routes/dashboard_deeplink_provider.ts +++ b/tensorboard/webapp/routes/dashboard_deeplink_provider.ts @@ -14,7 +14,7 @@ limitations under the License. ==============================================================================*/ import {Injectable} from '@angular/core'; import {Store} from '@ngrx/store'; -import {combineLatest, of, Observable} from 'rxjs'; +import {combineLatest, Observable, of} from 'rxjs'; import {map} from 'rxjs/operators'; import {DeepLinkProvider} from '../app_routing/deep_link_provider'; import {SerializableQueryParams} from '../app_routing/types'; @@ -97,12 +97,14 @@ export class DashboardDeepLinkProvider extends DeepLinkProvider { return [{key: TAG_FILTER_KEY, value: filterText}]; }) ), - of(getOverriddenFeatureFlagStates( - FeatureFlagMetadataMap as Record< - string, - FeatureFlagMetadata - > - )), + of( + getOverriddenFeatureFlagStates( + FeatureFlagMetadataMap as Record< + string, + FeatureFlagMetadata + > + ) + ), store.select(selectors.getMetricsSettingOverrides).pipe( map((settingOverrides) => { if (Number.isFinite(settingOverrides.scalarSmoothing)) { diff --git a/tensorboard/webapp/routes/feature_flag_serializer.ts b/tensorboard/webapp/routes/feature_flag_serializer.ts index 249b33deb66..0249d7dcaaa 100644 --- a/tensorboard/webapp/routes/feature_flag_serializer.ts +++ b/tensorboard/webapp/routes/feature_flag_serializer.ts @@ -16,31 +16,48 @@ import {Location} from '../app_routing/location'; import {SerializableQueryParams} from '../app_routing/types'; import {FeatureFlagMetadata} from '../feature_flag/store/feature_flag_metadata'; +/** + * Finds all FeatureFlags from the provided FeatureFlagMetadata that are present in the + * query params and returns their overridden values. + * Note: If a flag has multiple values in the query params it will be returned multiple + * times. + * + * i.e. The query params '?experimentalPlugin=0&experimentalPlugin=1&experimentalPlugin=2' + * will result in a return value of + * [ + * { key: 'experimentalPlugin', value: '0' }, + * { key: 'experimentalPlugin', value: '1' }, + * { key: 'experimentalPlugin', value: '2' }, + * ] + */ export function getOverriddenFeatureFlagStates( - featureFlagQueryParameters: Record> + featureFlagMetadataMap: Record> ): SerializableQueryParams { - const currentQueryParams = Object.fromEntries( - serializableQueryParamsToEntries(new Location().getSearch() || []) + // Converting the array to a map allows for a more efficient filter function below. + const currentQueryParams = (new Location().getSearch() || []).reduce( + (map, {key, value}) => { + if (!map[key]) { + map[key] = []; + } + map[key].push(value); + + return map; + }, + {} as Record ); - const currentlyOverriddenQueryParams = Object.values( - featureFlagQueryParameters - ) + const currentlyOverriddenQueryParams = Object.values(featureFlagMetadataMap) .map(({queryParamOverride}: FeatureFlagMetadata) => queryParamOverride) .filter( (queryParamOverride) => queryParamOverride && queryParamOverride in currentQueryParams ) as string[]; - return currentlyOverriddenQueryParams.map((queryParamOverride) => { - return { - key: queryParamOverride, - value: currentQueryParams[queryParamOverride], - }; - }); - - function serializableQueryParamsToEntries( - params: SerializableQueryParams - ): [string, string][] { - return params.map(({key, value}) => [key, value]); - } + return currentlyOverriddenQueryParams + .map((queryParamOverride) => { + return currentQueryParams[queryParamOverride].map((value) => ({ + key: queryParamOverride, + value, + })); + }) + .flat(); } diff --git a/tensorboard/webapp/routes/feature_flag_serializer_test.ts b/tensorboard/webapp/routes/feature_flag_serializer_test.ts index 985f19d533d..c49ddb19cc2 100644 --- a/tensorboard/webapp/routes/feature_flag_serializer_test.ts +++ b/tensorboard/webapp/routes/feature_flag_serializer_test.ts @@ -59,9 +59,7 @@ describe('feature flag serializer', () => { }); it('persists flag states overridden by query params', async () => { - spyOn(TEST_ONLY.utils, 'getSearch').and.returnValue( - '?darkMode=true' - ); + spyOn(TEST_ONLY.utils, 'getSearch').and.returnValue('?darkMode=true'); console.log(location.getSearch()); const queryParams = getOverriddenFeatureFlagStates( FeatureFlagMetadataMap as Record< 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 92de9375ab1..fb3e73b5d6e 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 @@ -59,20 +59,17 @@ export class QueryParamsFeatureFlagDataSource if (!queryParamOverride || !params.has(queryParamOverride)) { return null; } - const paramValues: T[] = this.queryParams - .getParams() - .getAll(queryParamOverride) - .map((value) => { - if (value === '' && flagMetadata.defaultValue !== undefined) { - return flagMetadata.defaultValue; - } - return flagMetadata.parseValue(value); - }); + const paramValues: T[] = params.getAll(queryParamOverride).map((value) => { + if (value === '' && flagMetadata.defaultValue !== undefined) { + return flagMetadata.defaultValue; + } + return flagMetadata.parseValue(value); + }); if (!paramValues.length) { return null; } - return flagMetadata.isArray ? paramValues : paramValues[0]; + return flagMetadata.isArray ? paramValues.flat() : paramValues[0]; } 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..cde48c6c375 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 @@ -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, + }); }); }); From efa2aa30aa0cfce1a51f1f3ba1cc5c66615b2e6b Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Tue, 26 Jul 2022 12:02:08 -0700 Subject: [PATCH 07/12] serialize all overridden feature flags instead of just overridden feature flags in the query params --- tensorboard/webapp/routes/BUILD | 23 +----- .../routes/dashboard_deeplink_provider.ts | 64 ++++++++++++---- .../dashboard_deeplink_provider_test.ts | 71 ++++++++++++++++++ .../webapp/routes/feature_flag_serializer.ts | 63 ---------------- .../routes/feature_flag_serializer_test.ts | 75 ------------------- 5 files changed, 124 insertions(+), 172 deletions(-) delete mode 100644 tensorboard/webapp/routes/feature_flag_serializer.ts delete mode 100644 tensorboard/webapp/routes/feature_flag_serializer_test.ts diff --git a/tensorboard/webapp/routes/BUILD b/tensorboard/webapp/routes/BUILD index 4a52d12047b..c872f618700 100644 --- a/tensorboard/webapp/routes/BUILD +++ b/tensorboard/webapp/routes/BUILD @@ -37,12 +37,13 @@ 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", @@ -55,24 +56,6 @@ tf_ts_library( ], ) -tf_ts_library( - name = "feature_flag_serializer", - srcs = [ - "feature_flag_serializer.ts", - ], - deps = [ - "//tensorboard/webapp:app_state", - "//tensorboard/webapp:selectors", - "//tensorboard/webapp/app_routing:location", - "//tensorboard/webapp/app_routing:types", - "//tensorboard/webapp/feature_flag/store:feature_flag_metadata", - "//tensorboard/webapp/webapp_data_source:feature_flag_types", - "@npm//@angular/core", - "@npm//@ngrx/store", - "@npm//rxjs", - ], -) - tf_ts_library( name = "testing", testonly = True, @@ -89,11 +72,9 @@ 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", diff --git a/tensorboard/webapp/routes/dashboard_deeplink_provider.ts b/tensorboard/webapp/routes/dashboard_deeplink_provider.ts index 8333de18975..c92c652edd9 100644 --- a/tensorboard/webapp/routes/dashboard_deeplink_provider.ts +++ b/tensorboard/webapp/routes/dashboard_deeplink_provider.ts @@ -14,16 +14,17 @@ limitations under the License. ==============================================================================*/ import {Injectable} from '@angular/core'; import {Store} from '@ngrx/store'; -import {combineLatest, Observable, of} from 'rxjs'; +import {combineLatest, Observable} from 'rxjs'; 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 {FeatureFlagMetadataMap} from '../feature_flag/store/feature_flag_metadata'; import { - FeatureFlagMetadata, - FeatureFlagMetadataMap, - FeatureFlagType, -} from '../feature_flag/store/feature_flag_metadata'; + getEnabledExperimentalPlugins, + getOverriddenFeatureFlags, +} from '../feature_flag/store/feature_flag_selectors'; +import {FeatureFlags} from '../feature_flag/types'; import { isPluginType, isSampledPlugin, @@ -40,7 +41,6 @@ import { SMOOTHING_KEY, TAG_FILTER_KEY, } from './dashboard_deeplink_provider_types'; -import {getOverriddenFeatureFlagStates} from './feature_flag_serializer'; const COLOR_GROUP_REGEX_VALUE_PREFIX = 'regex:'; @@ -97,13 +97,51 @@ export class DashboardDeepLinkProvider extends DeepLinkProvider { return [{key: TAG_FILTER_KEY, value: filterText}]; }) ), - of( - getOverriddenFeatureFlagStates( - FeatureFlagMetadataMap as Record< - string, - FeatureFlagMetadata - > - ) + store.select(getEnabledExperimentalPlugins).pipe( + map((enabledExperimentalPlugins) => { + return enabledExperimentalPlugins.map((pluginName) => ({ + key: FeatureFlagMetadataMap.enabledExperimentalPlugins + .queryParamOverride!, + value: pluginName, + })); + }) + ), + store.select(getOverriddenFeatureFlags).pipe( + map((featureFlags) => { + return Object.entries(featureFlags) + .map(([featureFlag, featureValue]) => { + const key = + FeatureFlagMetadataMap[featureFlag as keyof FeatureFlags] + ?.queryParamOverride; + if (!key || !featureValue) { + 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 Array<{ + key: string; + value: string; + }>; + }) ), store.select(selectors.getMetricsSettingOverrides).pipe( map((settingOverrides) => { diff --git a/tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts b/tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts index b57e7d41356..478582a8297 100644 --- a/tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts +++ b/tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts @@ -347,6 +347,77 @@ describe('core deeplink provider', () => { }); }); + describe('feature flag', () => { + it('serializes enabled experimental plugins', () => { + store.overrideSelector(selectors.getEnabledExperimentalPlugins, [ + 'foo', + 'bar', + 'baz', + ]); + store.refreshState(); + + expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([ + {key: 'experimentalPlugin', value: 'foo'}, + {key: 'experimentalPlugin', value: 'bar'}, + {key: 'experimentalPlugin', value: 'baz'}, + ]); + }); + + it('serializes enableColorGroup state', () => { + store.overrideSelector(selectors.getOverriddenFeatureFlags, { + enabledColorGroup: true, + }); + store.refreshState(); + + expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([ + {key: 'enableColorGroup', value: 'true'}, + ]); + + store.overrideSelector(selectors.getOverriddenFeatureFlags, { + enabledColorGroup: false, + }); + store.refreshState(); + + expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual( + [] + ); + + store.overrideSelector(selectors.getOverriddenFeatureFlags, {}); + store.refreshState(); + + expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual( + [] + ); + }); + + it('serializes enableColorGroupByRegex state', () => { + store.overrideSelector(selectors.getOverriddenFeatureFlags, { + enabledColorGroupByRegex: true, + }); + store.refreshState(); + + expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([ + {key: 'enableColorGroupByRegex', value: 'true'}, + ]); + + store.overrideSelector(selectors.getOverriddenFeatureFlags, { + enabledColorGroupByRegex: false, + }); + store.refreshState(); + + expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual( + [] + ); + + store.overrideSelector(selectors.getOverriddenFeatureFlags, {}); + store.refreshState(); + + expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual( + [] + ); + }); + }); + describe('runs', () => { describe('color group', () => { it('does not put state in the URL when user set color group is null', () => { diff --git a/tensorboard/webapp/routes/feature_flag_serializer.ts b/tensorboard/webapp/routes/feature_flag_serializer.ts deleted file mode 100644 index 0249d7dcaaa..00000000000 --- a/tensorboard/webapp/routes/feature_flag_serializer.ts +++ /dev/null @@ -1,63 +0,0 @@ -/* 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 {Location} from '../app_routing/location'; -import {SerializableQueryParams} from '../app_routing/types'; -import {FeatureFlagMetadata} from '../feature_flag/store/feature_flag_metadata'; - -/** - * Finds all FeatureFlags from the provided FeatureFlagMetadata that are present in the - * query params and returns their overridden values. - * Note: If a flag has multiple values in the query params it will be returned multiple - * times. - * - * i.e. The query params '?experimentalPlugin=0&experimentalPlugin=1&experimentalPlugin=2' - * will result in a return value of - * [ - * { key: 'experimentalPlugin', value: '0' }, - * { key: 'experimentalPlugin', value: '1' }, - * { key: 'experimentalPlugin', value: '2' }, - * ] - */ -export function getOverriddenFeatureFlagStates( - featureFlagMetadataMap: Record> -): SerializableQueryParams { - // Converting the array to a map allows for a more efficient filter function below. - const currentQueryParams = (new Location().getSearch() || []).reduce( - (map, {key, value}) => { - if (!map[key]) { - map[key] = []; - } - map[key].push(value); - - return map; - }, - {} as Record - ); - - const currentlyOverriddenQueryParams = Object.values(featureFlagMetadataMap) - .map(({queryParamOverride}: FeatureFlagMetadata) => queryParamOverride) - .filter( - (queryParamOverride) => - queryParamOverride && queryParamOverride in currentQueryParams - ) as string[]; - return currentlyOverriddenQueryParams - .map((queryParamOverride) => { - return currentQueryParams[queryParamOverride].map((value) => ({ - key: queryParamOverride, - value, - })); - }) - .flat(); -} diff --git a/tensorboard/webapp/routes/feature_flag_serializer_test.ts b/tensorboard/webapp/routes/feature_flag_serializer_test.ts deleted file mode 100644 index c49ddb19cc2..00000000000 --- a/tensorboard/webapp/routes/feature_flag_serializer_test.ts +++ /dev/null @@ -1,75 +0,0 @@ -/* 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 {TestBed} from '@angular/core/testing'; -import {Location, TEST_ONLY} from '../app_routing/location'; -import { - FeatureFlagMetadata, - FeatureFlagMetadataMap, - FeatureFlagType, -} from '../feature_flag/store/feature_flag_metadata'; -import {getOverriddenFeatureFlagStates} from './feature_flag_serializer'; - -describe('feature flag serializer', () => { - let location: Location; - - beforeEach(async () => { - await TestBed.configureTestingModule({ - providers: [Location], - }).compileComponents(); - - location = TestBed.inject(Location); - }); - - describe('getOverriddenFeatureFlagStates', () => { - it('returns empty list when no feature flags are active', () => { - const queryParams = getOverriddenFeatureFlagStates( - FeatureFlagMetadataMap as Record< - string, - FeatureFlagMetadata - > - ); - expect(queryParams.length).toEqual(0); - }); - - it('persists values of enabled experimental plugins', () => { - spyOn(TEST_ONLY.utils, 'getSearch').and.returnValue( - '?experimentalPlugin=0&experimentalPlugin=1&experimentalPlugin=2' - ); - const queryParams = getOverriddenFeatureFlagStates( - FeatureFlagMetadataMap as Record< - string, - FeatureFlagMetadata - > - ); - expect(queryParams.length).toEqual(3); - expect(queryParams[0].key).toEqual('experimentalPlugin'); - expect(queryParams[0].value).toEqual('0'); - }); - - it('persists flag states overridden by query params', async () => { - spyOn(TEST_ONLY.utils, 'getSearch').and.returnValue('?darkMode=true'); - console.log(location.getSearch()); - const queryParams = getOverriddenFeatureFlagStates( - FeatureFlagMetadataMap as Record< - string, - FeatureFlagMetadata - > - ); - expect(queryParams.length).toEqual(1); - expect(queryParams[0].key).toEqual('darkMode'); - expect(queryParams[0].value).toEqual('true'); - }); - }); -}); From 6f6a14d406f0b4080b451ffeefbb7940c0ea6bf2 Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Wed, 27 Jul 2022 15:57:57 -0700 Subject: [PATCH 08/12] break feature flag serialization logic back out into it's own file --- tensorboard/webapp/routes/BUILD | 22 ++++++ .../routes/dashboard_deeplink_provider.ts | 48 ++++--------- .../webapp/routes/feature_flag_serializer.ts | 41 +++++++++++ .../routes/feature_flag_serializer_test.ts | 68 +++++++++++++++++++ 4 files changed, 144 insertions(+), 35 deletions(-) create mode 100644 tensorboard/webapp/routes/feature_flag_serializer.ts create mode 100644 tensorboard/webapp/routes/feature_flag_serializer_test.ts diff --git a/tensorboard/webapp/routes/BUILD b/tensorboard/webapp/routes/BUILD index c872f618700..38051e6ff43 100644 --- a/tensorboard/webapp/routes/BUILD +++ b/tensorboard/webapp/routes/BUILD @@ -37,6 +37,7 @@ 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", @@ -56,6 +57,25 @@ tf_ts_library( ], ) +tf_ts_library( + name = "feature_flag_serializer", + srcs = [ + "feature_flag_serializer.ts", + ], + deps = [ + "//tensorboard/webapp:app_state", + "//tensorboard/webapp:selectors", + "//tensorboard/webapp/app_routing:location", + "//tensorboard/webapp/app_routing:types", + "//tensorboard/webapp/feature_flag:types", + "//tensorboard/webapp/feature_flag/store:feature_flag_metadata", + "//tensorboard/webapp/webapp_data_source:feature_flag_types", + "@npm//@angular/core", + "@npm//@ngrx/store", + "@npm//rxjs", + ], +) + tf_ts_library( name = "testing", testonly = True, @@ -72,9 +92,11 @@ 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", diff --git a/tensorboard/webapp/routes/dashboard_deeplink_provider.ts b/tensorboard/webapp/routes/dashboard_deeplink_provider.ts index c92c652edd9..21493fc728f 100644 --- a/tensorboard/webapp/routes/dashboard_deeplink_provider.ts +++ b/tensorboard/webapp/routes/dashboard_deeplink_provider.ts @@ -19,12 +19,15 @@ 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 {FeatureFlagMetadataMap} from '../feature_flag/store/feature_flag_metadata'; +import { + FeatureFlagMetadata, + FeatureFlagMetadataMap, + FeatureFlagType, +} from '../feature_flag/store/feature_flag_metadata'; import { getEnabledExperimentalPlugins, getOverriddenFeatureFlags, } from '../feature_flag/store/feature_flag_selectors'; -import {FeatureFlags} from '../feature_flag/types'; import { isPluginType, isSampledPlugin, @@ -41,6 +44,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:'; @@ -108,39 +112,13 @@ export class DashboardDeepLinkProvider extends DeepLinkProvider { ), store.select(getOverriddenFeatureFlags).pipe( map((featureFlags) => { - return Object.entries(featureFlags) - .map(([featureFlag, featureValue]) => { - const key = - FeatureFlagMetadataMap[featureFlag as keyof FeatureFlags] - ?.queryParamOverride; - if (!key || !featureValue) { - 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 Array<{ - key: string; - value: string; - }>; + return featureFlagsToSerializableQueryParams( + featureFlags, + FeatureFlagMetadataMap as Record< + string, + FeatureFlagMetadata + > + ); }) ), store.select(selectors.getMetricsSettingOverrides).pipe( diff --git a/tensorboard/webapp/routes/feature_flag_serializer.ts b/tensorboard/webapp/routes/feature_flag_serializer.ts new file mode 100644 index 00000000000..eeeff35aac1 --- /dev/null +++ b/tensorboard/webapp/routes/feature_flag_serializer.ts @@ -0,0 +1,41 @@ +import {SerializableQueryParams} from '../app_routing/types'; +import {FeatureFlagMetadata} from '../feature_flag/store/feature_flag_metadata'; +import {FeatureFlags} from '../feature_flag/types'; + +export function featureFlagsToSerializableQueryParams( + overriddenFeatureFlags: Partial, + featureFlagMetadataMap: Record> +): SerializableQueryParams { + return Object.entries(overriddenFeatureFlags) + .map(([featureFlag, featureValue]) => { + const key = + featureFlagMetadataMap[featureFlag as keyof FeatureFlags] + ?.queryParamOverride; + if (!key || !featureValue) { + 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; +} 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..b1b7eb8acae --- /dev/null +++ b/tensorboard/webapp/routes/feature_flag_serializer_test.ts @@ -0,0 +1,68 @@ +import {featureFlagsToSerializableQueryParams} from './feature_flag_serializer'; + +describe('feature flag serializer', () => { + describe('featureFlagsToSerializableQueryParams', () => { + let featureFlagsMetadata = {}; + beforeEach(() => { + featureFlagsMetadata = { + featureA: { + displayName: 'featureA', + queryParamOverride: 'feature_a', + parseValue: (s: string) => s, + }, + featureB: { + displayName: 'featureB', + queryParamOverride: 'feature_b', + isArray: true, + parseValue: (s: string) => s, + }, + }; + }); + 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 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', + }, + ]); + }); + }); +}); From 9ddcbbc7f5371487ffa7eca354633ee8a268e73a Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Thu, 28 Jul 2022 12:53:41 -0700 Subject: [PATCH 09/12] address pr comments --- tensorboard/webapp/app_routing/location.ts | 5 +-- .../store/feature_flag_metadata.ts | 15 --------- .../routes/dashboard_deeplink_provider.ts | 14 +------- .../dashboard_deeplink_provider_test.ts | 24 +++++++------- .../webapp/routes/feature_flag_serializer.ts | 16 +++++++++- .../routes/feature_flag_serializer_test.ts | 32 +++++++++++++++++++ 6 files changed, 62 insertions(+), 44 deletions(-) diff --git a/tensorboard/webapp/app_routing/location.ts b/tensorboard/webapp/app_routing/location.ts index bf1cd56fa02..95057966453 100644 --- a/tensorboard/webapp/app_routing/location.ts +++ b/tensorboard/webapp/app_routing/location.ts @@ -48,9 +48,6 @@ const utils = { getHref() { return window.location.href; }, - getSearch() { - return window.location.search; - }, }; @Injectable() @@ -60,7 +57,7 @@ export class Location implements LocationInterface { } getSearch(): SerializableQueryParams { - const searchParams = new URLSearchParams(utils.getSearch()); + const searchParams = new URLSearchParams(window.location.search); const serializableSearchParams: SerializableQueryParams = []; // URLSearchParams is a Iterable but TypeScript does not know about that. diff --git a/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts b/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts index 800bf0005b4..9c01fe0845d 100644 --- a/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts +++ b/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts @@ -30,7 +30,6 @@ export type BaseFeatureFlagType = boolean | number | string | null | undefined; export type FeatureFlagType = BaseFeatureFlagType | Array; export type FeatureFlagMetadata = { - displayName: string; defaultValue?: T; queryParamOverride?: string; parseValue: (str: string) => T; @@ -52,71 +51,57 @@ export const FeatureFlagMetadataMap: { [FlagName in keyof FeatureFlags]: FeatureFlagMetadata; } = { scalarsBatchSize: { - displayName: 'scalarsBatchSize', queryParamOverride: SCALARS_BATCH_SIZE_PARAM_KEY, parseValue: parseInt, }, enabledColorGroup: { - displayName: 'enabledColorGroup', queryParamOverride: ENABLE_COLOR_GROUP_QUERY_PARAM_KEY, parseValue: parseBoolean, }, enabledColorGroupByRegex: { - displayName: 'enabledColorGroupByRegex', queryParamOverride: ENABLE_COLOR_GROUP_BY_REGEX_QUERY_PARAM_KEY, parseValue: parseBoolean, }, enabledExperimentalPlugins: { - displayName: 'enabledExperimentalPlugins', queryParamOverride: EXPERIMENTAL_PLUGIN_QUERY_PARAM_KEY, parseValue: (str: string) => [str], isArray: true, }, enabledLinkedTime: { - displayName: 'enabledLinkedTime', queryParamOverride: ENABLE_LINKED_TIME_PARAM_KEY, parseValue: parseBoolean, }, enabledCardWidthSetting: { - displayName: 'enabledCardWidthSetting', queryParamOverride: ENABLE_CARD_WIDTH_SETTING_PARAM_KEY, parseValue: parseBoolean, }, enabledScalarDataTable: { - displayName: 'enabledScalarDataTable', queryParamOverride: ENABLE_DATA_TABLE_PARAM_KEY, parseValue: parseBoolean, }, forceSvg: { - displayName: 'forceSvg', queryParamOverride: FORCE_SVG_RENDERER, parseValue: parseBoolean, }, enableDarkModeOverride: { - displayName: 'enableDarkModeOverride', parseValue: parseBooleanOrNull, }, defaultEnableDarkMode: { - displayName: 'defaultEnableDarkMode', queryParamOverride: ENABLE_DARK_MODE_QUERY_PARAM_KEY, parseValue: parseBoolean, }, isAutoDarkModeAllowed: { - displayName: 'isAutoDarkModeAllowed', parseValue: parseBoolean, }, inColab: { - displayName: 'inColab', defaultValue: false, queryParamOverride: 'tensorboardColab', parseValue: parseBoolean, }, metricsImageSupportEnabled: { - displayName: 'metricsImageSupportEnabled', parseValue: parseBoolean, }, enableTimeSeriesPromotion: { - displayName: 'enableTimeSeriesPromotion', parseValue: parseBoolean, }, }; diff --git a/tensorboard/webapp/routes/dashboard_deeplink_provider.ts b/tensorboard/webapp/routes/dashboard_deeplink_provider.ts index 21493fc728f..d7f0bafd74e 100644 --- a/tensorboard/webapp/routes/dashboard_deeplink_provider.ts +++ b/tensorboard/webapp/routes/dashboard_deeplink_provider.ts @@ -24,10 +24,7 @@ import { FeatureFlagMetadataMap, FeatureFlagType, } from '../feature_flag/store/feature_flag_metadata'; -import { - getEnabledExperimentalPlugins, - getOverriddenFeatureFlags, -} from '../feature_flag/store/feature_flag_selectors'; +import {getOverriddenFeatureFlags} from '../feature_flag/store/feature_flag_selectors'; import { isPluginType, isSampledPlugin, @@ -101,15 +98,6 @@ export class DashboardDeepLinkProvider extends DeepLinkProvider { return [{key: TAG_FILTER_KEY, value: filterText}]; }) ), - store.select(getEnabledExperimentalPlugins).pipe( - map((enabledExperimentalPlugins) => { - return enabledExperimentalPlugins.map((pluginName) => ({ - key: FeatureFlagMetadataMap.enabledExperimentalPlugins - .queryParamOverride!, - value: pluginName, - })); - }) - ), store.select(getOverriddenFeatureFlags).pipe( map((featureFlags) => { return featureFlagsToSerializableQueryParams( diff --git a/tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts b/tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts index 478582a8297..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([ @@ -378,9 +380,9 @@ describe('core deeplink provider', () => { }); store.refreshState(); - expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual( - [] - ); + expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([ + {key: 'enableColorGroup', value: 'false'}, + ]); store.overrideSelector(selectors.getOverriddenFeatureFlags, {}); store.refreshState(); @@ -405,9 +407,9 @@ describe('core deeplink provider', () => { }); store.refreshState(); - expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual( - [] - ); + expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([ + {key: 'enableColorGroupByRegex', value: 'false'}, + ]); store.overrideSelector(selectors.getOverriddenFeatureFlags, {}); store.refreshState(); diff --git a/tensorboard/webapp/routes/feature_flag_serializer.ts b/tensorboard/webapp/routes/feature_flag_serializer.ts index eeeff35aac1..eeb0d22cd92 100644 --- a/tensorboard/webapp/routes/feature_flag_serializer.ts +++ b/tensorboard/webapp/routes/feature_flag_serializer.ts @@ -1,3 +1,17 @@ +/* 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} from '../feature_flag/store/feature_flag_metadata'; import {FeatureFlags} from '../feature_flag/types'; @@ -11,7 +25,7 @@ export function featureFlagsToSerializableQueryParams( const key = featureFlagMetadataMap[featureFlag as keyof FeatureFlags] ?.queryParamOverride; - if (!key || !featureValue) { + if (!key || featureValue === undefined) { return []; } /** diff --git a/tensorboard/webapp/routes/feature_flag_serializer_test.ts b/tensorboard/webapp/routes/feature_flag_serializer_test.ts index b1b7eb8acae..90370752a82 100644 --- a/tensorboard/webapp/routes/feature_flag_serializer_test.ts +++ b/tensorboard/webapp/routes/feature_flag_serializer_test.ts @@ -1,3 +1,17 @@ +/* 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 {featureFlagsToSerializableQueryParams} from './feature_flag_serializer'; describe('feature flag serializer', () => { @@ -18,6 +32,7 @@ describe('feature flag serializer', () => { }, }; }); + it('should return empty list when no flags are overridden', () => { const serializableQueryParams = featureFlagsToSerializableQueryParams( {}, @@ -44,6 +59,23 @@ describe('feature flag serializer', () => { ]); }); + 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, From 566bcda27c7ad54fb153ef2ad275383118bd53cc Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Thu, 28 Jul 2022 15:15:25 -0700 Subject: [PATCH 10/12] change the definition of feature flag metadata default value, only persist feature flags with non default values --- tensorboard/webapp/feature_flag/store/BUILD | 1 + .../store/feature_flag_metadata.ts | 15 ++++++++++- .../feature_flag_store_config_provider.ts | 27 ++++++++----------- .../dashboard_deeplink_provider_test.ts | 12 ++++----- .../webapp/routes/feature_flag_serializer.ts | 13 ++++++--- .../routes/feature_flag_serializer_test.ts | 15 +++++++++++ .../tb_feature_flag_data_source.ts | 3 --- .../tb_feature_flag_data_source_test.ts | 4 +-- 8 files changed, 59 insertions(+), 31 deletions(-) diff --git a/tensorboard/webapp/feature_flag/store/BUILD b/tensorboard/webapp/feature_flag/store/BUILD index cb1fb338a14..dea1873e606 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", diff --git a/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts b/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts index 9c01fe0845d..433efa2db3e 100644 --- a/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts +++ b/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts @@ -30,7 +30,7 @@ export type BaseFeatureFlagType = boolean | number | string | null | undefined; export type FeatureFlagType = BaseFeatureFlagType | Array; export type FeatureFlagMetadata = { - defaultValue?: T; + defaultValue: T; queryParamOverride?: string; parseValue: (str: string) => T; isArray?: boolean; @@ -51,46 +51,57 @@ export const FeatureFlagMetadataMap: { [FlagName in keyof FeatureFlags]: FeatureFlagMetadata; } = { scalarsBatchSize: { + defaultValue: undefined, queryParamOverride: SCALARS_BATCH_SIZE_PARAM_KEY, parseValue: parseInt, }, enabledColorGroup: { + defaultValue: true, queryParamOverride: ENABLE_COLOR_GROUP_QUERY_PARAM_KEY, parseValue: parseBoolean, }, enabledColorGroupByRegex: { + defaultValue: true, queryParamOverride: ENABLE_COLOR_GROUP_BY_REGEX_QUERY_PARAM_KEY, parseValue: parseBoolean, }, enabledExperimentalPlugins: { + defaultValue: [], queryParamOverride: EXPERIMENTAL_PLUGIN_QUERY_PARAM_KEY, parseValue: (str: string) => [str], isArray: true, }, enabledLinkedTime: { + defaultValue: false, queryParamOverride: ENABLE_LINKED_TIME_PARAM_KEY, parseValue: parseBoolean, }, enabledCardWidthSetting: { + defaultValue: true, queryParamOverride: ENABLE_CARD_WIDTH_SETTING_PARAM_KEY, parseValue: parseBoolean, }, enabledScalarDataTable: { + defaultValue: false, queryParamOverride: ENABLE_DATA_TABLE_PARAM_KEY, parseValue: parseBoolean, }, forceSvg: { + defaultValue: false, queryParamOverride: FORCE_SVG_RENDERER, parseValue: parseBoolean, }, enableDarkModeOverride: { + defaultValue: null, parseValue: parseBooleanOrNull, }, defaultEnableDarkMode: { + defaultValue: false, queryParamOverride: ENABLE_DARK_MODE_QUERY_PARAM_KEY, parseValue: parseBoolean, }, isAutoDarkModeAllowed: { + defaultValue: true, parseValue: parseBoolean, }, inColab: { @@ -99,9 +110,11 @@ export const FeatureFlagMetadataMap: { parseValue: parseBoolean, }, metricsImageSupportEnabled: { + defaultValue: true, parseValue: parseBoolean, }, enableTimeSeriesPromotion: { + defaultValue: false, parseValue: parseBoolean, }, }; 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..039cc0f8287 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,21 @@ limitations under the License. ==============================================================================*/ import {InjectionToken} from '@angular/core'; import {StoreConfig} from '@ngrx/store'; +import {FeatureFlags} from '../types'; +import {FeatureFlagMetadataMap} from './feature_flag_metadata'; import {FeatureFlagState} from './feature_flag_types'; +const defaultFlags = Object.entries(FeatureFlagMetadataMap).reduce( + (map, [key, {defaultValue}]) => { + map[key] = defaultValue; + return map; + }, + {} as Record +) as FeatureFlags; + 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, flagOverrides: {}, }; diff --git a/tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts b/tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts index cf72c247a7b..a645dfdebd3 100644 --- a/tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts +++ b/tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts @@ -371,9 +371,9 @@ describe('core deeplink provider', () => { }); store.refreshState(); - expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([ - {key: 'enableColorGroup', value: 'true'}, - ]); + expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual( + [] + ); store.overrideSelector(selectors.getOverriddenFeatureFlags, { enabledColorGroup: false, @@ -398,9 +398,9 @@ describe('core deeplink provider', () => { }); store.refreshState(); - expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([ - {key: 'enableColorGroupByRegex', value: 'true'}, - ]); + expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual( + [] + ); store.overrideSelector(selectors.getOverriddenFeatureFlags, { enabledColorGroupByRegex: false, diff --git a/tensorboard/webapp/routes/feature_flag_serializer.ts b/tensorboard/webapp/routes/feature_flag_serializer.ts index eeb0d22cd92..e539b3d57c1 100644 --- a/tensorboard/webapp/routes/feature_flag_serializer.ts +++ b/tensorboard/webapp/routes/feature_flag_serializer.ts @@ -13,10 +13,15 @@ See the License for the specific language governing permissions and limitations under the License. ==============================================================================*/ import {SerializableQueryParams} from '../app_routing/types'; -import {FeatureFlagMetadata} from '../feature_flag/store/feature_flag_metadata'; +import { + FeatureFlagMetadata, + FeatureFlagType, +} from '../feature_flag/store/feature_flag_metadata'; import {FeatureFlags} from '../feature_flag/types'; -export function featureFlagsToSerializableQueryParams( +export function featureFlagsToSerializableQueryParams< + T extends FeatureFlagType +>( overriddenFeatureFlags: Partial, featureFlagMetadataMap: Record> ): SerializableQueryParams { @@ -25,7 +30,9 @@ export function featureFlagsToSerializableQueryParams( const key = featureFlagMetadataMap[featureFlag as keyof FeatureFlags] ?.queryParamOverride; - if (!key || featureValue === undefined) { + const defaultValue = + featureFlagMetadataMap[featureFlag as keyof FeatureFlags]?.defaultValue; + if (!key || featureValue === undefined || featureValue === defaultValue) { return []; } /** diff --git a/tensorboard/webapp/routes/feature_flag_serializer_test.ts b/tensorboard/webapp/routes/feature_flag_serializer_test.ts index 90370752a82..a1b1443485c 100644 --- a/tensorboard/webapp/routes/feature_flag_serializer_test.ts +++ b/tensorboard/webapp/routes/feature_flag_serializer_test.ts @@ -23,12 +23,14 @@ describe('feature flag serializer', () => { displayName: 'featureA', queryParamOverride: 'feature_a', parseValue: (s: string) => s, + defaultValue: 'feature_a_123', }, featureB: { displayName: 'featureB', queryParamOverride: 'feature_b', isArray: true, parseValue: (s: string) => s, + defaultValue: 'feature_b_456', }, }; }); @@ -96,5 +98,18 @@ describe('feature flag serializer', () => { }, ]); }); + + it('should not serialize feaure flags with default values', () => { + const serializableQueryParams = featureFlagsToSerializableQueryParams( + {featureA: 'feature_a_123', featureB: 'foo'} as any, + featureFlagsMetadata + ); + expect(serializableQueryParams).toEqual([ + { + key: 'feature_b', + value: 'foo', + }, + ]); + }); }); }); 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 fb3e73b5d6e..bc166f0c160 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 @@ -60,9 +60,6 @@ export class QueryParamsFeatureFlagDataSource return null; } const paramValues: T[] = params.getAll(queryParamOverride).map((value) => { - if (value === '' && flagMetadata.defaultValue !== undefined) { - return flagMetadata.defaultValue; - } return flagMetadata.parseValue(value); }); if (!paramValues.length) { 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 cde48c6c375..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`', () => { @@ -289,7 +289,7 @@ describe('tb_feature_flag_data_source', () => { ); expect(dataSource.getFeatures()).toEqual({ enabledExperimentalPlugins: ['a'], - inColab: false, + inColab: true, scalarsBatchSize: 16, }); }); From 1510275da37a218fbae25c25d3da4cde2f63f055 Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Fri, 29 Jul 2022 12:22:40 -0700 Subject: [PATCH 11/12] persist overridden features with default values --- .../routes/dashboard_deeplink_provider_test.ts | 12 ++++++------ .../webapp/routes/feature_flag_serializer.ts | 4 +--- .../webapp/routes/feature_flag_serializer_test.ts | 15 --------------- 3 files changed, 7 insertions(+), 24 deletions(-) diff --git a/tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts b/tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts index a645dfdebd3..cf72c247a7b 100644 --- a/tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts +++ b/tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts @@ -371,9 +371,9 @@ describe('core deeplink provider', () => { }); store.refreshState(); - expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual( - [] - ); + expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([ + {key: 'enableColorGroup', value: 'true'}, + ]); store.overrideSelector(selectors.getOverriddenFeatureFlags, { enabledColorGroup: false, @@ -398,9 +398,9 @@ describe('core deeplink provider', () => { }); store.refreshState(); - expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual( - [] - ); + expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([ + {key: 'enableColorGroupByRegex', value: 'true'}, + ]); store.overrideSelector(selectors.getOverriddenFeatureFlags, { enabledColorGroupByRegex: false, diff --git a/tensorboard/webapp/routes/feature_flag_serializer.ts b/tensorboard/webapp/routes/feature_flag_serializer.ts index e539b3d57c1..85e741fba56 100644 --- a/tensorboard/webapp/routes/feature_flag_serializer.ts +++ b/tensorboard/webapp/routes/feature_flag_serializer.ts @@ -30,9 +30,7 @@ export function featureFlagsToSerializableQueryParams< const key = featureFlagMetadataMap[featureFlag as keyof FeatureFlags] ?.queryParamOverride; - const defaultValue = - featureFlagMetadataMap[featureFlag as keyof FeatureFlags]?.defaultValue; - if (!key || featureValue === undefined || featureValue === defaultValue) { + if (!key || featureValue === undefined) { return []; } /** diff --git a/tensorboard/webapp/routes/feature_flag_serializer_test.ts b/tensorboard/webapp/routes/feature_flag_serializer_test.ts index a1b1443485c..90370752a82 100644 --- a/tensorboard/webapp/routes/feature_flag_serializer_test.ts +++ b/tensorboard/webapp/routes/feature_flag_serializer_test.ts @@ -23,14 +23,12 @@ describe('feature flag serializer', () => { displayName: 'featureA', queryParamOverride: 'feature_a', parseValue: (s: string) => s, - defaultValue: 'feature_a_123', }, featureB: { displayName: 'featureB', queryParamOverride: 'feature_b', isArray: true, parseValue: (s: string) => s, - defaultValue: 'feature_b_456', }, }; }); @@ -98,18 +96,5 @@ describe('feature flag serializer', () => { }, ]); }); - - it('should not serialize feaure flags with default values', () => { - const serializableQueryParams = featureFlagsToSerializableQueryParams( - {featureA: 'feature_a_123', featureB: 'foo'} as any, - featureFlagsMetadata - ); - expect(serializableQueryParams).toEqual([ - { - key: 'feature_b', - value: 'foo', - }, - ]); - }); }); }); From 23ea96374ff4c5509be85cb8668ad2f77d368811 Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Fri, 29 Jul 2022 14:39:53 -0700 Subject: [PATCH 12/12] address pr comments, breakout config provider and data source logic for reusability, move feature flag overrides to feature flag metadata --- tensorboard/webapp/feature_flag/store/BUILD | 1 - .../store/feature_flag_metadata.ts | 173 +++++++++--------- .../feature_flag_store_config_provider.ts | 16 +- tensorboard/webapp/routes/BUILD | 7 - .../webapp/routes/feature_flag_serializer.ts | 55 ++++++ .../routes/feature_flag_serializer_test.ts | 110 +++++++++-- tensorboard/webapp/webapp_data_source/BUILD | 1 + .../tb_feature_flag_data_source.ts | 41 ++--- .../tb_feature_flag_data_source_types.ts | 19 -- 9 files changed, 254 insertions(+), 169 deletions(-) diff --git a/tensorboard/webapp/feature_flag/store/BUILD b/tensorboard/webapp/feature_flag/store/BUILD index dea1873e606..6b6392322f4 100644 --- a/tensorboard/webapp/feature_flag/store/BUILD +++ b/tensorboard/webapp/feature_flag/store/BUILD @@ -29,7 +29,6 @@ tf_ng_module( ], deps = [ "//tensorboard/webapp/feature_flag:types", - "//tensorboard/webapp/webapp_data_source:feature_flag_types", "@npm//@angular/core", ], ) diff --git a/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts b/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts index 433efa2db3e..bd64ac3a0c4 100644 --- a/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts +++ b/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts @@ -12,17 +12,6 @@ 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 { - 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, -} from '../../webapp_data_source/tb_feature_flag_data_source_types'; import {FeatureFlags} from '../types'; export type BaseFeatureFlagType = boolean | number | string | null | undefined; @@ -32,10 +21,14 @@ export type FeatureFlagType = BaseFeatureFlagType | Array; export type FeatureFlagMetadata = { defaultValue: T; queryParamOverride?: string; - parseValue: (str: string) => T; + 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'; } @@ -47,74 +40,88 @@ export function parseBooleanOrNull(str: string): boolean | null { return parseBoolean(str); } -export const FeatureFlagMetadataMap: { - [FlagName in keyof FeatureFlags]: FeatureFlagMetadata; -} = { - scalarsBatchSize: { - defaultValue: undefined, - queryParamOverride: SCALARS_BATCH_SIZE_PARAM_KEY, - parseValue: parseInt, - }, - enabledColorGroup: { - defaultValue: true, - queryParamOverride: ENABLE_COLOR_GROUP_QUERY_PARAM_KEY, - parseValue: parseBoolean, - }, - enabledColorGroupByRegex: { - defaultValue: true, - queryParamOverride: ENABLE_COLOR_GROUP_BY_REGEX_QUERY_PARAM_KEY, - parseValue: parseBoolean, - }, - enabledExperimentalPlugins: { - defaultValue: [], - queryParamOverride: EXPERIMENTAL_PLUGIN_QUERY_PARAM_KEY, - parseValue: (str: string) => [str], - isArray: true, - }, - enabledLinkedTime: { - defaultValue: false, - queryParamOverride: ENABLE_LINKED_TIME_PARAM_KEY, - parseValue: parseBoolean, - }, - enabledCardWidthSetting: { - defaultValue: true, - queryParamOverride: ENABLE_CARD_WIDTH_SETTING_PARAM_KEY, - parseValue: parseBoolean, - }, - enabledScalarDataTable: { - defaultValue: false, - queryParamOverride: ENABLE_DATA_TABLE_PARAM_KEY, - parseValue: parseBoolean, - }, - forceSvg: { - defaultValue: false, - queryParamOverride: FORCE_SVG_RENDERER, - parseValue: parseBoolean, - }, - enableDarkModeOverride: { - defaultValue: null, - parseValue: parseBooleanOrNull, - }, - defaultEnableDarkMode: { - defaultValue: false, - queryParamOverride: ENABLE_DARK_MODE_QUERY_PARAM_KEY, - parseValue: parseBoolean, - }, - isAutoDarkModeAllowed: { - defaultValue: true, - parseValue: parseBoolean, - }, - inColab: { - defaultValue: false, - queryParamOverride: 'tensorboardColab', - parseValue: parseBoolean, - }, - metricsImageSupportEnabled: { - defaultValue: true, - parseValue: parseBoolean, - }, - enableTimeSeriesPromotion: { - defaultValue: false, - parseValue: parseBoolean, - }, -}; +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_store_config_provider.ts b/tensorboard/webapp/feature_flag/store/feature_flag_store_config_provider.ts index 039cc0f8287..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,21 +14,15 @@ limitations under the License. ==============================================================================*/ import {InjectionToken} from '@angular/core'; import {StoreConfig} from '@ngrx/store'; -import {FeatureFlags} from '../types'; -import {FeatureFlagMetadataMap} from './feature_flag_metadata'; +import { + FeatureFlagMetadataMap, + generateFeatureFlagDefaults, +} from './feature_flag_metadata'; import {FeatureFlagState} from './feature_flag_types'; -const defaultFlags = Object.entries(FeatureFlagMetadataMap).reduce( - (map, [key, {defaultValue}]) => { - map[key] = defaultValue; - return map; - }, - {} as Record -) as FeatureFlags; - export const initialState: FeatureFlagState = { isFeatureFlagsLoaded: false, - defaultFlags, + defaultFlags: generateFeatureFlagDefaults(FeatureFlagMetadataMap), flagOverrides: {}, }; diff --git a/tensorboard/webapp/routes/BUILD b/tensorboard/webapp/routes/BUILD index 38051e6ff43..75253f3456e 100644 --- a/tensorboard/webapp/routes/BUILD +++ b/tensorboard/webapp/routes/BUILD @@ -63,16 +63,9 @@ tf_ts_library( "feature_flag_serializer.ts", ], deps = [ - "//tensorboard/webapp:app_state", - "//tensorboard/webapp:selectors", - "//tensorboard/webapp/app_routing:location", "//tensorboard/webapp/app_routing:types", "//tensorboard/webapp/feature_flag:types", "//tensorboard/webapp/feature_flag/store:feature_flag_metadata", - "//tensorboard/webapp/webapp_data_source:feature_flag_types", - "@npm//@angular/core", - "@npm//@ngrx/store", - "@npm//rxjs", ], ) diff --git a/tensorboard/webapp/routes/feature_flag_serializer.ts b/tensorboard/webapp/routes/feature_flag_serializer.ts index 85e741fba56..b0dd23af3b0 100644 --- a/tensorboard/webapp/routes/feature_flag_serializer.ts +++ b/tensorboard/webapp/routes/feature_flag_serializer.ts @@ -15,6 +15,7 @@ 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'; @@ -58,3 +59,57 @@ export function featureFlagsToSerializableQueryParams< ({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 index 90370752a82..9eabf73ebde 100644 --- a/tensorboard/webapp/routes/feature_flag_serializer_test.ts +++ b/tensorboard/webapp/routes/feature_flag_serializer_test.ts @@ -12,27 +12,35 @@ 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 {featureFlagsToSerializableQueryParams} from './feature_flag_serializer'; +import { + FeatureFlagMetadata, + FeatureFlagMetadataMapType, +} from '../feature_flag/store/feature_flag_metadata'; +import { + featureFlagsToSerializableQueryParams, + getFeatureFlagValueFromSearchParams, + getOverriddenFeatureFlagValuesFromSearchParams, +} from './feature_flag_serializer'; describe('feature flag serializer', () => { - describe('featureFlagsToSerializableQueryParams', () => { - let featureFlagsMetadata = {}; - beforeEach(() => { - featureFlagsMetadata = { - featureA: { - displayName: 'featureA', - queryParamOverride: 'feature_a', - parseValue: (s: string) => s, - }, - featureB: { - displayName: 'featureB', - queryParamOverride: 'feature_b', - isArray: true, - parseValue: (s: string) => s, - }, - }; - }); + 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( {}, @@ -97,4 +105,70 @@ describe('feature flag serializer', () => { ]); }); }); + + 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 70966ad3a0f..f42b3fef2a6 100644 --- a/tensorboard/webapp/webapp_data_source/BUILD +++ b/tensorboard/webapp/webapp_data_source/BUILD @@ -99,6 +99,7 @@ tf_ng_module( ":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", 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 bc166f0c160..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 @@ -14,11 +14,11 @@ limitations under the License. ==============================================================================*/ import {Injectable} from '@angular/core'; import { - FeatureFlagMetadata, 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 {TBFeatureFlagDataSource} from './tb_feature_flag_data_source_types'; @@ -37,36 +37,17 @@ export class QueryParamsFeatureFlagDataSource // 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() : {}; - Object.entries(FeatureFlagMetadataMap).forEach( - ([flagName, flagMetadata]) => { - const featureValue = this.getFeatureValue( - flagMetadata as FeatureFlagMetadata - ); - if (featureValue !== null) { - const f = flagName as keyof FeatureFlags; - featureFlags[f] = featureValue; - } - } + const featuresFromMediaQuery: Partial< + Record + > = enableMediaQuery ? this.getPartialFeaturesFromMediaQuery() : {}; + const overriddenFeatures = getOverriddenFeatureFlagValuesFromSearchParams( + FeatureFlagMetadataMap, + this.queryParams.getParams() ); - return featureFlags as Partial; - } - - protected getFeatureValue(flagMetadata: FeatureFlagMetadata) { - const params = this.queryParams.getParams(); - const queryParamOverride = flagMetadata.queryParamOverride; - if (!queryParamOverride || !params.has(queryParamOverride)) { - return null; - } - const paramValues: T[] = params.getAll(queryParamOverride).map((value) => { - return flagMetadata.parseValue(value); - }); - if (!paramValues.length) { - return null; - } - - return flagMetadata.isArray ? paramValues.flat() : paramValues[0]; + return { + ...featuresFromMediaQuery, + ...overriddenFeatures, + } as Partial; } protected getPartialFeaturesFromMediaQuery(): { 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';