Simplify how experimentalPlugin query param works so we can simplify the feature flag infrastructure. #5836
Conversation
infrastructure for feature flag metadata.
rileyajones
left a comment
rileyajones
left a comment
There was a problem hiding this comment.
This is a good cleanup, thanks. I doubt that the existing featureFlag appears in many links but do you think it could be worth adding some logic someplace (maybe in the datasource) that cleans it up (and ideally alerts the user) if the older style link is used?
| // Optional function that translates a feature flag value into a query param | ||
| // value. If unspecified then any query param value will be encoded using | ||
| // value.toString(). | ||
| encodeValue?: (value: T) => string; |
There was a problem hiding this comment.
It feels odd that this is required when T is string[] but I'm not sure it's actually worth making the typing more convoluted to enforce that. Could you leave a comment someplace noting it though?
There was a problem hiding this comment.
I made a nice discovery while investigating this: string[].toString() does exactly what we want. That is ['val1', 'val2', 'val3'].toString() will output 'val1,val2,val3'. This means we don't even need encodeValue and we can continue to rely on toString() to do all the encoding.
I chatted with Riley offline and we agreed that would be the right way forward.
No, it's not worth it. This flag is practically unused for over a year. I did check internal documentation. There is an example that uses the param but it only specifies a single value so no need to update it! |
| defaultValue: [], | ||
| queryParamOverride: 'experimentalPlugin', | ||
| parseValue: parseStringArray, | ||
| encodeValue: encodeStringArray, |
…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.
…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.
2 participants
When we introduced
FeatureFlagMetadatain #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
experimentalPluginquery 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,plugin3where we previously would have writtenexperimentalPlugin=plugin1&experimentalPlugin=plugin2&experimentalPlugin=plugin3.Once experimentalPlugin is just a single query parameter there is a path towards removing
isArrayand simplifying the framework. We removeisArray. enabledExperimentalPlugins specifies a function for parseValue that knows how to convert strings of type 'val1,val2,val3' into a string[]. 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.