Skip to content

Commit

Permalink
Feature: Update the showFlags feature to allow filtering (tensorflow#…
Browse files Browse the repository at this point in the history
…6245)

## Motivation for features / changes
As we add more feature flags it's starting to get hard to find them.
This is particularly true when trying to help someone else find a
feature flag in the modal.

The `enableShowFlags` feature was a weird edge case that wasn't being
fully utilized. Now it supports string values. When a string value is
supplied it filters the feature flags shown.

## Technical description of changes
I changed the name and value of `enableShowFlags: boolean` to
`showFlags: string`.
I then updated the `feature_flag_page_container` to filter the feature
flags rendered based on the value of the `showFlags` feature.

## Screenshots of UI changes
Modal With a Filter

![image](https://user-images.githubusercontent.com/78179109/225770098-5460517e-8946-42f2-8f6a-b26980b63ffc.png)

Modal without a filter

![image](https://user-images.githubusercontent.com/78179109/225770279-034c0eb7-a566-4376-b265-39d6adae4b9d.png)

## Detailed steps to verify changes work correctly (as executed by you)
1) Start tensorboard
2) Navigate to localhost:6006
3) Assert the feature flags modal does not appear
4) Navigate to localhost:6006?showFlags
5) Assert the feature flags modal appears
6) Assert the no filter is applied
7) Navigate to localhost:6006?showFlags=enable
8) Assert the feature flags modal appears and has a filter applied

## Alternate designs / implementations considered
Adding the filter could automatically toggle a feature?
Adding an input field to filter things would make sense...
  • Loading branch information
rileyajones authored Mar 17, 2023
1 parent 6f28538 commit eaa8353
Show file tree
Hide file tree
Showing 11 changed files with 134 additions and 31 deletions.
1 change: 1 addition & 0 deletions tensorboard/webapp/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,7 @@ tf_ng_web_test_suite(
"//tensorboard/webapp/core/views:test_lib",
"//tensorboard/webapp/customization:customization_test_lib",
"//tensorboard/webapp/deeplink:deeplink_test_lib",
"//tensorboard/webapp/feature_flag/views:views_test",
"//tensorboard/webapp/header:test_lib",
"//tensorboard/webapp/metrics:integration_test",
"//tensorboard/webapp/metrics:test_lib",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,10 +110,10 @@ export const FeatureFlagMetadataMap: FeatureFlagMetadataMapType<FeatureFlags> =
defaultValue: true,
queryParamOverride: null,
},
enableShowFlags: {
defaultValue: false,
showFlags: {
defaultValue: undefined,
queryParamOverride: 'showFlags',
parseValue: parseBoolean,
parseValue: (str) => str,
},
allowRangeSelection: {
defaultValue: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ export const getIsDataTableEnabled = createSelector(
export const getShowFlagsEnabled = createSelector(
getFeatureFlags,
(flags: FeatureFlags): boolean => {
return flags.enableShowFlags;
return flags.showFlags !== undefined;
}
);

Expand Down
4 changes: 2 additions & 2 deletions tensorboard/webapp/feature_flag/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ export interface FeatureFlags {
forceSvg: boolean;
// Whether to enable the "sticky" data table in scalar cards.
enabledScalarDataTable: boolean;
// If enabled causes the feature flags modal to appear.
enableShowFlags: boolean;
// If defined causes the feature flags modal to appear.
showFlags: string | undefined;
// Adds check box in settings which allows users to enter step selection range.
allowRangeSelection: boolean;
// In Linked Time, if enabled, show a prospective fob user to turn on the feature or select a step.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,12 @@ export class FeatureFlagModalTriggerContainer implements OnInit {
// dialog from appearing again after the page is refreshed.
this.store.dispatch(
featureFlagOverridesReset({
flags: ['enableShowFlags'],
flags: ['showFlags'],
})
);
// Reload the page so that the application restarts with stable
// feature flag values.
// Wait one tick before reloading the page so the 'enableShowFlags'
// Wait one tick before reloading the page so the 'showFlags'
// reset has a chance to be reflected in the URL before page reload.
setTimeout(() => {
util.reloadWindow();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ describe('feature_flag_modal_trigger_container', () => {
rootLoader = TestbedHarnessEnvironment.documentRootLoader(fixture);
}

it('creates modal when enableShowFlags is true', async () => {
it('creates modal when showFlags is true', async () => {
store.overrideSelector(getDefaultFeatureFlags, {} as FeatureFlags);
store.overrideSelector(getOverriddenFeatureFlags, {});
store.overrideSelector(getShowFlagsEnabled, true);
Expand All @@ -90,7 +90,7 @@ describe('feature_flag_modal_trigger_container', () => {
expect(dialog).toBeDefined();
});

it('does not create modal when enableShowFlags is false', async () => {
it('does not create modal when showFlags is false', async () => {
store.overrideSelector(getDefaultFeatureFlags, {} as FeatureFlags);
store.overrideSelector(getOverriddenFeatureFlags, {});
store.overrideSelector(getShowFlagsEnabled, false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {FeatureFlagPageModule} from './feature_flag_module';

/**
* Provides the wrapper component that triggers the opening of the feature flag modal.
* The modal appears based on the value of the enableShowFlags feature flag.
* The modal appears based on the value of the showFlags feature flag.
*/
@NgModule({
declarations: [FeatureFlagModalTriggerContainer],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@ <h2 class="warning">WARNING: EXPERIMENTAL FEATURES AHEAD!</h2>
Only flags with non default values are sent to the backend.
</div>
</ng-container>
<ng-container *ngIf="showFlagsFilter">
<div class="message">
Feature Flags are filtered to only show features containing
"{{showFlagsFilter}}"
</div>
</ng-container>
<table class="feature-flag-table">
<ng-container *ngFor="let flagStatus of featureFlagStatuses;">
<tr>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ export class FeatureFlagPageComponent {

@Input() hasFlagsSentToServer: boolean = false;

@Input() showFlagsFilter: string | undefined;

@Output() flagChanged = new EventEmitter<FeatureFlagStatusEvent>();

@Output() allFlagsReset = new EventEmitter();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,20 @@ import {
template: `<feature-flag-page-component
[featureFlagStatuses]="featureFlags$ | async"
[hasFlagsSentToServer]="hasFlagsSentToServer$ | async"
[showFlagsFilter]="showFlagsFilter$ | async"
(flagChanged)="onFlagChanged($event)"
(allFlagsReset)="onAllFlagsReset()"
></feature-flag-page-component>`,
})
export class FeatureFlagPageContainer {
constructor(private readonly store: Store<State>) {}

readonly showFlagsFilter$ = this.store.select(getOverriddenFeatureFlags).pipe(
map((overriddenFeatureFlags) => {
return overriddenFeatureFlags.showFlags?.toLowerCase();
})
);

readonly hasFlagsSentToServer$: Observable<boolean> = this.store
.select(getFeatureFlagsMetadata)
.pipe(
Expand All @@ -66,27 +73,40 @@ export class FeatureFlagPageContainer {
this.store.select(getOverriddenFeatureFlags).pipe(
withLatestFrom(
this.store.select(getDefaultFeatureFlags),
this.store.select(getFeatureFlagsMetadata)
this.store.select(getFeatureFlagsMetadata),
this.showFlagsFilter$
),
map(([overriddenFeatureFlags, defaultFeatureFlags, flagMetadata]) => {
return Object.entries(defaultFeatureFlags).map(
([flagName, defaultValue]) => {
const status = getFlagStatus(
flagName as keyof FeatureFlags,
overriddenFeatureFlags
);
const metadata = flagMetadata[flagName as keyof FeatureFlags];
return {
flag: flagName,
defaultValue,
status,
sendToServerWhenOverridden: (
metadata as AdvancedFeatureFlagMetadata<FeatureFlagType>
).sendToServerWhenOverridden,
} as FeatureFlagStatus<keyof FeatureFlags>;
}
);
})
map(
([
overriddenFeatureFlags,
defaultFeatureFlags,
flagMetadata,
showFlagsFilter,
]) => {
return Object.entries(defaultFeatureFlags)
.filter(([flagName]) => {
if (!showFlagsFilter) {
return true;
}
return flagName.toLowerCase().includes(showFlagsFilter);
})
.map(([flagName, defaultValue]) => {
const status = getFlagStatus(
flagName as keyof FeatureFlags,
overriddenFeatureFlags
);
const metadata = flagMetadata[flagName as keyof FeatureFlags];
return {
flag: flagName,
defaultValue,
status,
sendToServerWhenOverridden: (
metadata as AdvancedFeatureFlagMetadata<FeatureFlagType>
).sendToServerWhenOverridden,
} as FeatureFlagStatus<keyof FeatureFlags>;
});
}
)
);

onFlagChanged({flag, status}: FeatureFlagStatusEvent) {
Expand Down
74 changes: 74 additions & 0 deletions tensorboard/webapp/feature_flag/views/feature_flag_page_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -234,4 +234,78 @@ describe('feature_flag_page_container', () => {
expect(component.formatFlagValue('')).toEqual('');
});
});

describe('filters flags based on the value of showFlags feature', () => {
beforeEach(() => {
store.overrideSelector(getDefaultFeatureFlags, {
enabledLinkedTime: true,
enabledProspectiveFob: true,
inColab: false,
allowRangeSelection: true,
} as FeatureFlags);
});

it('shows all flags when value is undefined', () => {
store.overrideSelector(getOverriddenFeatureFlags, {
showFlags: undefined,
});
createComponent();
const component = getComponent();

const rows = component.querySelectorAll('tr');
expect(rows.length).toEqual(4);
});

it('shows all flags when value is empty string', () => {
store.overrideSelector(getOverriddenFeatureFlags, {
showFlags: '',
});
createComponent();
const component = getComponent();

const rows = component.querySelectorAll('tr');
expect(rows.length).toEqual(4);
});

it('only shows flags whose name includes filter', () => {
store.overrideSelector(getOverriddenFeatureFlags, {
showFlags: 'enable',
});
createComponent();
const component = getComponent();

expect(component.querySelectorAll('tr').length).toEqual(2);

store.overrideSelector(getOverriddenFeatureFlags, {
showFlags: 'linked',
});
store.refreshState();
fixture.detectChanges();
expect(component.querySelectorAll('tr').length).toEqual(1);
});

it('shows message when flags are filtered', () => {
store.overrideSelector(getOverriddenFeatureFlags, {
showFlags: 'enable',
});

createComponent();
const component = getComponent();
expect(component.innerText).toContain(
'Feature Flags are filtered to only show features containing'
);
});

it('does not show message when flags are not filtered', () => {
store.overrideSelector(getOverriddenFeatureFlags, {
showFlags: undefined,
});

createComponent();
const component = getComponent();
expect(component.innerText).not.toContain(
'Feature Flags are filtered to only show features containing'
);
});
});
});

0 comments on commit eaa8353

Please sign in to comment.