Adds mechanism to include feature flags in Polymer HTTP requests #5727

bmd3k · 2022-05-26T16:21:06Z

This adds a mechanism that dumps the client-side FeatureFlag object into each HTTP request that originates from the TB Polymer code base.

(Googlers, see: http://go/tb-client-feature-flags-in-data-provider for the motivation and high level design.)

See #5718 for the corresponding change for Angular and #5721 for server-side support.

The data is dumped into a custom header. The format is effectively:
X-TensorBoard-Feature-Flags: JSON.stringify(currentFeatureFlags)

We implement this directly in the RequestManager object, through which all our Polymer HTTP requests are routed.

Additional things worth noting:

I added a const for the header name to be shared between the HttpInterceptor and the RequestManager.
I added a new angular<->polymer interop object, tf-feature-flags, modeled after some of the other interop objects we've added in the past. This interop object should now be the source of truth for accessing feature flags from the polymer code base. It is populated by feature_flags_effects soon after we fetch feature flag overrides from the data source.
This will send ALL feature flags in the request header rather than a curated subset of them, just like in Adds mechanism to include feature flags in Angular HTTP requests. #5718. In a subsequent PR, we'll add support to mark certain feature flags as "sendToServer" and only send that subset.

…est-manager

Add a client_feature_flags property to the DataProvider's RequestContext object. Also add a middleware that extracts the feature flags from the HTTP request and sets them on the RequestContext. client_feature_flags represents feature flags set in the TB webapp client. They will be injected into many HTTP requests originating from the webapp and will be of the form: X-TensorBoard-Feature-Flags: JSON.stringify(currentFeatureFlags) We expose these values as a Dict in the RequestContext object, with string keys and values of arbitrary types (but, in practice, mostly booleans and sometimes arrays of strings). Usage of client_feature_flags should be aware of the key names and the value types. (Googlers, see: http://go/tb-client-feature-flags-in-data-provider for motivation, more details, and high level design.) #5718 and #5727 are complimentary PRs that set the feature flags in requests originating from the Angular and Polymer code bases, respectively.

nfelt · 2022-06-03T18:46:53Z

@bmd3k for when you're back - is this ready for review? (nobody is currently assigned)

bmd3k · 2022-06-06T11:35:23Z

@bmd3k for when you're back - is this ready for review? (nobody is currently assigned)

Thanks! Assigned to James since he reviewed similar changes for HTTP requests from Angular.

JamesHollyer

Ok these comments are mostly just questions. I am confused about the scope and ownership of the _featureFlags object. I decided not to approve yet because I don't think I can properly review without understanding a bit more of how this works.

JamesHollyer · 2022-06-06T22:42:19Z

tensorboard/components/tf_feature_flags/feature-flags.ts

+// base. In practice they are set one time soon after application load and never
+// again for the lifetime of the application.
+let _featureFlags: FeatureFlags | null;
+initializeFeatureFlags();


This seems strange. So _featureFlags is initialized by this call in the application but this is called manually in tests? Is this s typical structure in the polymer code?

I'm not sure if it is typical. Without testing I would have likely just merged lines 21 to 26 into the following one line:

let _featureFlags: FeatureFlags | null = null;

But I added the initializeFeatureFlags() function to facilitate testing. Unfortunately we don't really test much of the polymer code so I just picked a pattern that worked fo rme.

JamesHollyer · 2022-06-06T22:49:41Z

tensorboard/components/tf_feature_flags/tf-feature-flags-polymer.ts

+ */
+@customElement('tf-feature-flags')
+class TfFeatureFlags extends PolymerElement {
+  override _template = null;


This means that this is an element with no UI component?

Honestly I don't know. I'm following the pattern established for similar files, like:
https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/components/tf_globals/globals-polymer.ts

Yes, that's what it means. The more above-board way to do this would be the static method described here:
https://polymer-library.polymer-project.org/3.0/docs/devguide/dom-template#elements-with-no-shadow-dom

Overriding _template looks like it's doing that at a lower level, probably because that's the legacy approach: https://github.com/Polymer/polymer/blob/1e8b246d01ea99adba305ea04c45d26da31f68f1/lib/mixins/element-mixin.js#L476-L515

tensorboard/components/tf_feature_flags/tf-feature-flags-polymer.ts

JamesHollyer · 2022-06-06T22:51:02Z

tensorboard/components/tf_feature_flags/tf-feature-flags-polymer.ts

+@customElement('tf-feature-flags')
+class TfFeatureFlags extends PolymerElement {
+  override _template = null;
+  tf_feature_flags = tf_feature_flags;


What does this even do? Assigning a variable to itself?

Again, sorry, honestly I don't know. I'm following the pattern established for similar files, like:
https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/components/tf_globals/globals-polymer.ts

This might look confusing but syntactically it's not assigning a variable to itself; it's defining a property on the element and assigning its value to be the imported module from import * as tf_feature_flags from './feature-flags';

You can see how this mechanism works a bit better with the existing tf_globals code:

We create a tf-globals custom element that basically just holds a tf_globals property that represents the symbols from the Polymer static utilities module: https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/components/tf_globals/globals-polymer.ts

We define a type for this property so that the TypeScript code on the Angular side can access it in a typesafe manner: https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/webapp/tb_polymer_interop_types.ts;l=30-32;drc=046667d00df2316eefcda9a4ef058e83221bad77

In the Angular code where we actually need to call the tf_globals static utilities, we create a custom element, then access its tf_globals property: https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/webapp/deeplink/hash.ts;l=30-35;drc=367839f30de3968eff5fecb8b38e1d5782434b6b

tensorboard/components/tf_feature_flags/tf-feature-flags-polymer.ts

JamesHollyer · 2022-06-06T23:00:43Z

tensorboard/webapp/feature_flag/effects/feature_flag_effects.ts

 import {State} from '../store/feature_flag_types';

 const effectsInitialized = createAction('[FEATURE FLAG] Effects Init');

 @Injectable()
 export class FeatureFlagEffects {
+  // Ngrx assumes all Effect classes have properties that inherit from the base
+  // JS Object. `tf_feature_flags` does not, so we wrap it.
+  private readonly tfFeatureFlags = {


does this mean that this object is owned by the FeatureFlagEffects class?

Does that |initializeFeatureFlags| function get called when creating this element? Is that what the self assigned variable does?

This is also quite a bit of a mystery to me but I believe this just exposes the interop leayer (tf-feature-flags) to the effects class. I'm not certain at what point, exactly, the class gets initialized.

I'm following the pattern established here:
https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/webapp/core/effects/core_effects.ts;l=90;drc=414434e1b632a569da5a4921db400de0750857b0

tensorboard/webapp/feature_flag/effects/feature_flag_effects.ts

bmd3k

You will probably find my responses unsatisfying. The understanding of how or why this interop mechanism works is lost with Stephan. I'm following the pattern established by other Angular<->Polymer interop components, a list of which you can find here, if you'd like to dig more into it:

https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/webapp/tb_polymer_interop_types.ts

bmd3k · 2022-06-07T20:24:50Z

tensorboard/components/tf_feature_flags/feature-flags.ts

+// base. In practice they are set one time soon after application load and never
+// again for the lifetime of the application.
+let _featureFlags: FeatureFlags | null;
+initializeFeatureFlags();


I'm not sure if it is typical. Without testing I would have likely just merged lines 21 to 26 into the following one line:

let _featureFlags: FeatureFlags | null = null;

But I added the initializeFeatureFlags() function to facilitate testing. Unfortunately we don't really test much of the polymer code so I just picked a pattern that worked fo rme.

bmd3k · 2022-06-08T16:13:57Z

tensorboard/components/tf_feature_flags/tf-feature-flags-polymer.ts

+ */
+@customElement('tf-feature-flags')
+class TfFeatureFlags extends PolymerElement {
+  override _template = null;


Honestly I don't know. I'm following the pattern established for similar files, like:
https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/components/tf_globals/globals-polymer.ts

bmd3k · 2022-06-08T16:14:11Z

tensorboard/components/tf_feature_flags/tf-feature-flags-polymer.ts

+@customElement('tf-feature-flags')
+class TfFeatureFlags extends PolymerElement {
+  override _template = null;
+  tf_feature_flags = tf_feature_flags;


Again, sorry, honestly I don't know. I'm following the pattern established for similar files, like:
https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/components/tf_globals/globals-polymer.ts

bmd3k · 2022-06-08T16:17:00Z

tensorboard/webapp/feature_flag/effects/feature_flag_effects.ts

 import {State} from '../store/feature_flag_types';

 const effectsInitialized = createAction('[FEATURE FLAG] Effects Init');

 @Injectable()
 export class FeatureFlagEffects {
+  // Ngrx assumes all Effect classes have properties that inherit from the base
+  // JS Object. `tf_feature_flags` does not, so we wrap it.
+  private readonly tfFeatureFlags = {


This is also quite a bit of a mystery to me but I believe this just exposes the interop leayer (tf-feature-flags) to the effects class. I'm not certain at what point, exactly, the class gets initialized.

I'm following the pattern established here:
https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/webapp/core/effects/core_effects.ts;l=90;drc=414434e1b632a569da5a4921db400de0750857b0

JamesHollyer

Thanks for the help understanding these pieces Nick. This LGTM.

JamesHollyer · 2022-06-06T22:42:19Z

tensorboard/components/tf_feature_flags/feature-flags.ts

+// base. In practice they are set one time soon after application load and never
+// again for the lifetime of the application.
+let _featureFlags: FeatureFlags | null;
+initializeFeatureFlags();


This seems strange. So _featureFlags is initialized by this call in the application but this is called manually in tests? Is this s typical structure in the polymer code?

…ow#5721) Add a client_feature_flags property to the DataProvider's RequestContext object. Also add a middleware that extracts the feature flags from the HTTP request and sets them on the RequestContext. client_feature_flags represents feature flags set in the TB webapp client. They will be injected into many HTTP requests originating from the webapp and will be of the form: X-TensorBoard-Feature-Flags: JSON.stringify(currentFeatureFlags) We expose these values as a Dict in the RequestContext object, with string keys and values of arbitrary types (but, in practice, mostly booleans and sometimes arrays of strings). Usage of client_feature_flags should be aware of the key names and the value types. (Googlers, see: http://go/tb-client-feature-flags-in-data-provider for motivation, more details, and high level design.) tensorflow#5718 and tensorflow#5727 are complimentary PRs that set the feature flags in requests originating from the Angular and Polymer code bases, respectively.

…orflow#5727) This adds a mechanism that dumps the client-side FeatureFlag object into each HTTP request that originates from the TB Polymer code base. (Googlers, see: http://go/tb-client-feature-flags-in-data-provider for the motivation and high level design.) See tensorflow#5718 for the corresponding change for Angular and tensorflow#5721 for server-side support. The data is dumped into a custom header. The format is effectively: X-TensorBoard-Feature-Flags: JSON.stringify(currentFeatureFlags) We implement this directly in the RequestManager object, through which all our Polymer HTTP requests are routed. Additional things worth noting: * I added a const for the header name to be shared between the HttpInterceptor and the RequestManager. * I added a new angular<->polymer interop object, tf-feature-flags, modeled after some of the other interop objects we've added in the past. This interop object should now be the source of truth for accessing feature flags from the polymer code base. It is populated by feature_flags_effects soon after we fetch feature flag overrides from the data source. * This will send ALL feature flags in the request header rather than a curated subset of them, just like in tensorflow#5718. In a subsequent PR, we'll add support to mark certain feature flags as "sendToServer" and only send that subset.

…ow#5721) Add a client_feature_flags property to the DataProvider's RequestContext object. Also add a middleware that extracts the feature flags from the HTTP request and sets them on the RequestContext. client_feature_flags represents feature flags set in the TB webapp client. They will be injected into many HTTP requests originating from the webapp and will be of the form: X-TensorBoard-Feature-Flags: JSON.stringify(currentFeatureFlags) We expose these values as a Dict in the RequestContext object, with string keys and values of arbitrary types (but, in practice, mostly booleans and sometimes arrays of strings). Usage of client_feature_flags should be aware of the key names and the value types. (Googlers, see: http://go/tb-client-feature-flags-in-data-provider for motivation, more details, and high level design.) tensorflow#5718 and tensorflow#5727 are complimentary PRs that set the feature flags in requests originating from the Angular and Polymer code bases, respectively.

…orflow#5727) This adds a mechanism that dumps the client-side FeatureFlag object into each HTTP request that originates from the TB Polymer code base. (Googlers, see: http://go/tb-client-feature-flags-in-data-provider for the motivation and high level design.) See tensorflow#5718 for the corresponding change for Angular and tensorflow#5721 for server-side support. The data is dumped into a custom header. The format is effectively: X-TensorBoard-Feature-Flags: JSON.stringify(currentFeatureFlags) We implement this directly in the RequestManager object, through which all our Polymer HTTP requests are routed. Additional things worth noting: * I added a const for the header name to be shared between the HttpInterceptor and the RequestManager. * I added a new angular<->polymer interop object, tf-feature-flags, modeled after some of the other interop objects we've added in the past. This interop object should now be the source of truth for accessing feature flags from the polymer code base. It is populated by feature_flags_effects soon after we fetch feature flag overrides from the data source. * This will send ALL feature flags in the request header rather than a curated subset of them, just like in tensorflow#5718. In a subsequent PR, we'll add support to mark certain feature flags as "sendToServer" and only send that subset.

bmd3k added 4 commits May 20, 2022 13:14

Add tf-feature-flags and set feature flags in RequestManager.

15fe0aa

Merge remote-tracking branch 'upstream/master' into client-flags-requ…

d6f531f

…est-manager

Use constant for http header name.

a79470b

Some small modifications.

abb4835

bmd3k requested a review from JamesHollyer June 6, 2022 11:33

JamesHollyer reviewed Jun 6, 2022

View reviewed changes

bmd3k commented Jun 8, 2022

View reviewed changes

JamesHollyer approved these changes Jun 13, 2022

View reviewed changes

bmd3k requested a review from nfelt June 13, 2022 15:57

nfelt approved these changes Jun 13, 2022

View reviewed changes

bmd3k merged commit b044c72 into tensorflow:master Jun 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds mechanism to include feature flags in Polymer HTTP requests #5727

Adds mechanism to include feature flags in Polymer HTTP requests #5727

bmd3k commented May 26, 2022

nfelt commented Jun 3, 2022

bmd3k commented Jun 6, 2022

JamesHollyer left a comment

JamesHollyer Jun 6, 2022

bmd3k Jun 7, 2022

JamesHollyer Jun 6, 2022

bmd3k Jun 8, 2022

nfelt Jun 8, 2022

JamesHollyer Jun 6, 2022

bmd3k Jun 8, 2022

nfelt Jun 8, 2022

JamesHollyer Jun 6, 2022

bmd3k Jun 8, 2022

bmd3k left a comment

bmd3k Jun 7, 2022

bmd3k Jun 8, 2022

bmd3k Jun 8, 2022

bmd3k Jun 8, 2022

JamesHollyer left a comment

JamesHollyer Jun 6, 2022

Adds mechanism to include feature flags in Polymer HTTP requests #5727

Adds mechanism to include feature flags in Polymer HTTP requests #5727

Conversation

bmd3k commented May 26, 2022

nfelt commented Jun 3, 2022

bmd3k commented Jun 6, 2022

JamesHollyer 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

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

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

JamesHollyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment