Stop removing experiment related query parameters #5717

rileyajones · 2022-05-20T17:33:08Z

Motivation for features / changes
Navigating to TensorBoard used to result in all provided query parameters where being removed when the application state was persisted to the query parameters. This would lead to refreshed of the page not having the experiment value set correctly.
Technical description of changes
Unfortunately there is neither a one to one mapping of query parameter overrides to experiment nor do the overrides exactly reflect the experiment they are overriding. i.e. enabledLinkTime is overridden by the parameter enableLinkTime. To resolve this issue I manually created a mapping experiment to their query parameter overrides which I then checked when the application state is persisted to the query parameters.
Screenshots of UI changes
No UI change
Detailed steps to verify changes work correctly (as executed by you)
Visit the dashboard with an experiment overridden via a query parameter and note that it is not removed.

Note that prior to this enableColorGroupByRegex and enableColorGroup where both explicitly handled in a similar way.

Alternate designs / implementations considered
I considered two other approaches

The manual mapping could be avoided by changing the query parameter overrides (or else adding additional parameters to overrides). This could result in unexpected behavior for existing users unaware of the change but would be easier to maintain as more experiments are added.
Instead of replacing the all the query parameters, we could just update or remove those that had changed. This would be a bigger change to the application though and would risk introducing other issues.

bmd3k

For posterity, some of the things we discussed offline:

There is a more general need to associate metadata with individual feature flags. This is a good opportunity to build the foundation for that.
Riley suggested some changes to FeatureFlags type in general and will play around with that.
There is the consideration that specific TensorBoard instances (like TensorBoard.dev, TensorBoard.corp) may extend the FeatureFlags type to include additional feature flags.

tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source.ts

tensorboard/webapp/routes/feature_flag_serializer.ts

tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source.ts

tensorboard/webapp/routes/dashboard_deeplink_provider.ts

bmd3k · 2022-07-14T14:41:08Z

tensorboard/webapp/routes/feature_flag_serializer.ts

+                  queryParamOverride in currentQueryParams) {
+                queryParams.push({
+                  key: queryParamOverride,
+                  value: currentQueryParams[queryParamOverride],


So the strategy seems to be: If we determine a query param in the current URL represents a feature flag, then we copy the query param and its value into the new URL. The strategy doesn't actually depend on feature flag state.

I think we should consider depending on feature flag state to generate this part of the URL. You could use the data from getOverriddenFeatureFlags() to write the query params.

Some reasoning:

This better aligns with the idea that the URL is a projection of state (that is, it is fully constructed from internal state).

With the upcoming support for feature flags in local storage, the URL will include those overrides after page load. I think that is desirable behavior.

Simply making the URL a reflection of the feature flag state would be easier to do and thus would simplify the code. I was previously under the impression that reflecting localStorage was considered undesirable.

Having the URL reflect the localStorage overrides would be a good reminder to the user that they are in some special state.

It will help us quickly troubleshoot problems like "why can't I see feature A after feature A has launched?" when user forgot they set enableFeatureA=false in local storage. I am certain this will happen all the time.

This makes lots of sense to me. That also ensures that sharing a link will result in the other person seeing the same thing. I've gone ahead and made this change and will be pushing a new commit quite soon.

tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source.ts

tensorboard/webapp/webapp_data_source/tb_feature_flag_query_parameters.ts

tensorboard/webapp/routes/feature_flag_serializer.ts

tensorboard/webapp/webapp_data_source/tb_feature_flag_query_parameters.ts

…he url

…ture flags in the query params

tensorboard/webapp/app_routing/location.ts

bmd3k · 2022-07-27T19:37:54Z

tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts

+
+export type FeatureFlagMetadata<T> = {
+  displayName: string;
+  defaultValue?: T;


Is there a need for defaultValue? inColab, the one property that uses it, has its default defined here:

http://google3/third_party/tensorboard/webapp/feature_flag/store/feature_flag_store_config_provider.ts;l=28;rcl=453264239

Which is how every other flag defines a default value.

This might not be a good variable name. This is the value when the param is present but no value is given. TBH I don't like that there is any inconsistency here but I didn't want to effect existing behavior if possible.

In this case ?inColab SHOULD parse to false while others e.g. ?enableColorGroup would parse to true.

Given the confusing naming and strange UX surrounding this I am inclined to change the behavior of inColab when the value is not set. LMK if you'd rather I leave it as is and change the variable name.

Yes, I'm fine to change the behavior of inColab. I did a quick survey of usage internally and it looks like usage is always "tensorboardColab=true" and not just "tensorboardColab".

I'm going to update inColab. I've noticed that the url can get a bit cluttered if you start enabling lots of feature flags in the UI so I'm going to preempt the issue and only serialize feature flags with non default values.

bmd3k · 2022-07-27T19:47:12Z

tensorboard/webapp/routes/dashboard_deeplink_provider.ts

@@ -126,7 +97,52 @@ export class DashboardDeepLinkProvider extends DeepLinkProvider {
          return [{key: TAG_FILTER_KEY, value: filterText}];
        })
      ),
-      this.getFeatureFlagStates(store),
+      store.select(getEnabledExperimentalPlugins).pipe(


L100-L108, handling experimentalPlugin in its own section seems redundant with L109-L145 and leads to twice as many experimentalPlugin query params as you want.

For example, if you load with the following:

http://localhost:6006/?experimentalPlugin=blah&experimentalPlugin=blue

The end result will be:

http://localhost:6006/?experimentalPlugin=blah&experimentalPlugin=blue&experimentalPlugin=blah&experimentalPlugin=blue

🤯 in a previous version I didn't have a solution to array types. Looks like I somehow missed this after cleaning that up! Fantastic catch.

tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts

bmd3k · 2022-07-28T13:55:22Z

tensorboard/webapp/routes/feature_flag_serializer.ts

+    .flat()
+    .filter(
+      ({key, value}) => key && value !== undefined
+    ) as SerializableQueryParams;


Is there any need to put as SerializableQueryParams? The return value of the function is already specified as SerializableQueryParams so the compiler should already check this.

For some reason TypeScript wouldn't believe that key and value were always defined even after the filter.

tensorboard/webapp/routes/feature_flag_serializer.ts

tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts

…rsist feature flags with non default values

rileyajones · 2022-07-28T22:20:07Z

tensorboard/webapp/feature_flag/store/feature_flag_store_config_provider.ts

+  {} as Record<string, any>
+) as FeatureFlags;


This is a pretty gross cast... The issue is that Object.entries (and all the other Object methods) drops the typing. This cast seems preferable to casting the values but I am open to alternatives.

I COULD just enumerate the feature flags below but my current approach seems to scale better as more feature flags are added.

No problem for me. If you look hard enough there are several grosser secret casts in the FeatureFlag code base.

rileyajones · 2022-07-28T22:21:33Z

tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts

-        {key: 'enableColorGroup', value: 'true'},
-      ]);
+      expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual(
+        []


enableColorGroup defaults to true so persisting it is redundant.

rileyajones · 2022-07-28T22:23:35Z

tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source_test.ts

@@ -215,7 +289,7 @@ describe('tb_feature_flag_data_source', () => {
        );
        expect(dataSource.getFeatures()).toEqual({
          enabledExperimentalPlugins: ['a'],
-          inColab: false,
+          inColab: true,


Calling out that this behavior IS changing

Yes, that's fine. Thanks.

tensorboard/webapp/feature_flag/store/feature_flag_store_config_provider.ts

bmd3k · 2022-07-29T18:52:53Z

tensorboard/webapp/feature_flag/store/feature_flag_store_config_provider.ts

+  {} as Record<string, any>
+) as FeatureFlags;


No problem for me. If you look hard enough there are several grosser secret casts in the FeatureFlag code base.

tensorboard/webapp/routes/feature_flag_serializer.ts

bmd3k · 2022-07-29T19:07:34Z

tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source.ts

-    }
+    const featureFlags: Partial<Record<keyof FeatureFlags, FeatureFlagType>> =
+      enableMediaQuery ? this.getPartialFeaturesFromMediaQuery() : {};
+    Object.entries(FeatureFlagMetadataMap).forEach(


Can this also be refactored into something reusable? I think you were already intending on reusing getFeatureValue() but maybe we don't have to stop there. Especially given how this mangles all type information, would be nice to just have to write that code once.

tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source.ts

bmd3k · 2022-07-29T19:32:22Z

tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source_test.ts

@@ -215,7 +289,7 @@ describe('tb_feature_flag_data_source', () => {
        );
        expect(dataSource.getFeatures()).toEqual({
          enabledExperimentalPlugins: ['a'],
-          inColab: false,
+          inColab: true,


Yes, that's fine. Thanks.

tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source_test.ts

…or reusability, move feature flag overrides to feature flag metadata

bmd3k

Thanks!

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

* Stop removing query parameters when persisting application state to the url * simplify query param serialization logic * move feature flag overrides to feature flag metadata

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

* Stop removing query parameters when persisting application state to the url * simplify query param serialization logic * move feature flag overrides to feature flag metadata

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

bmd3k self-requested a review May 25, 2022 11:27

bmd3k reviewed May 25, 2022

View reviewed changes

rileyajones force-pushed the query-params-bug branch from db6b03d to f262ce9 Compare June 22, 2022 19:48

rileyajones force-pushed the query-params-bug branch from f262ce9 to 0d9d8a6 Compare June 29, 2022 22:53

rileyajones force-pushed the query-params-bug branch 2 times, most recently from bfce175 to 6e74acc Compare July 7, 2022 20:55

rileyajones commented Jul 8, 2022

View reviewed changes

tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source.ts Outdated Show resolved Hide resolved

rileyajones marked this pull request as draft July 8, 2022 19:40

rileyajones force-pushed the query-params-bug branch 6 times, most recently from 926121b to c00f9ed Compare July 11, 2022 20:11

rileyajones marked this pull request as ready for review July 11, 2022 21:30

rileyajones mentioned this pull request Jul 11, 2022

Show flags page as modal when showFlags query parameter is set #5800

Merged

rileyajones force-pushed the query-params-bug branch from c00f9ed to 5dc2e03 Compare July 11, 2022 23:41

bmd3k self-requested a review July 12, 2022 15:17

rileyajones marked this pull request as draft July 12, 2022 17:28

rileyajones force-pushed the query-params-bug branch from 5dc2e03 to b13aabf Compare July 12, 2022 23:42

rileyajones commented Jul 13, 2022

View reviewed changes

tensorboard/webapp/routes/feature_flag_serializer.ts Outdated Show resolved Hide resolved

rileyajones commented Jul 13, 2022

View reviewed changes

tensorboard/webapp/routes/feature_flag_serializer.ts Outdated Show resolved Hide resolved

rileyajones commented Jul 13, 2022

View reviewed changes

tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source.ts Outdated Show resolved Hide resolved

bmd3k reviewed Jul 14, 2022

View reviewed changes

rileyajones force-pushed the query-params-bug branch 2 times, most recently from 3db9e81 to d0a7292 Compare July 18, 2022 20:34

rileyajones marked this pull request as ready for review July 18, 2022 22:44

rileyajones added 3 commits July 25, 2022 10:09

Stop removing query parameters when persisting application state to t…

a1d51cb

…he url

address pr comments

444ad48

simplify query param serialization logic

f1b2717

rileyajones added 2 commits July 25, 2022 10:09

trying to fix some tests

260b105

rewrite tests to avoid using returnValues which for some reason fails

5325f80

rileyajones force-pushed the query-params-bug branch from d0a7292 to f2dac24 Compare July 25, 2022 22:18

serialize all overridden feature flags instead of just overridden fea…

efa2aa3

…ture flags in the query params

rileyajones force-pushed the query-params-bug branch from f2dac24 to efa2aa3 Compare July 26, 2022 19:02

break feature flag serialization logic back out into it's own file

6f6a14d

rileyajones force-pushed the query-params-bug branch from 3caf67c to 6f6a14d Compare July 27, 2022 22:58

bmd3k self-requested a review July 28, 2022 14:18

bmd3k reviewed Jul 28, 2022

View reviewed changes

rileyajones force-pushed the query-params-bug branch 2 times, most recently from e6bbffa to 157e299 Compare July 28, 2022 19:30

address pr comments

9ddcbbc

rileyajones force-pushed the query-params-bug branch from 157e299 to 9ddcbbc Compare July 28, 2022 19:53

change the definition of feature flag metadata default value, only pe…

566bcda

…rsist feature flags with non default values

rileyajones force-pushed the query-params-bug branch from 9a62e92 to 566bcda Compare July 28, 2022 22:15

rileyajones commented Jul 28, 2022

View reviewed changes

rileyajones requested a review from bmd3k July 29, 2022 16:38

persist overridden features with default values

1510275

bmd3k approved these changes Jul 29, 2022

View reviewed changes

address pr comments, breakout config provider and data source logic f…

23ea963

…or reusability, move feature flag overrides to feature flag metadata

bmd3k approved these changes Aug 1, 2022

View reviewed changes

rileyajones merged commit bbef530 into tensorflow:master Aug 1, 2022

This was referenced Aug 1, 2022

Bug Fix: Updating typing surrounding feature_flag_serializer_test.ts #5833

Merged

Bug Fix: Only override forceSvg from localStorage when the key is present #5834

Merged

bmd3k mentioned this pull request Aug 2, 2022

Simplify how experimentalPlugin query param works so we can simplify the feature flag infrastructure. #5836

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop removing experiment related query parameters #5717

Stop removing experiment related query parameters #5717

rileyajones commented May 20, 2022

bmd3k left a comment

bmd3k Jul 14, 2022

rileyajones Jul 25, 2022

bmd3k Jul 25, 2022

rileyajones Jul 25, 2022

bmd3k Jul 27, 2022

rileyajones Jul 28, 2022

bmd3k Jul 28, 2022 •

edited

rileyajones Jul 28, 2022

bmd3k Jul 27, 2022

rileyajones Jul 28, 2022

bmd3k Jul 28, 2022

rileyajones Jul 28, 2022

rileyajones Jul 28, 2022

bmd3k Jul 29, 2022

rileyajones Jul 28, 2022

rileyajones Jul 28, 2022

bmd3k Jul 29, 2022

bmd3k Jul 29, 2022

bmd3k Jul 29, 2022

bmd3k Jul 29, 2022

bmd3k left a comment

Stop removing experiment related query parameters #5717

Stop removing experiment related query parameters #5717

Conversation

rileyajones commented May 20, 2022

bmd3k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmd3k Jul 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmd3k left a comment

Choose a reason for hiding this comment

bmd3k Jul 28, 2022 •

edited