Investigate Options for Feature Toggles

joeniquette commented 2 years ago

Issue:

During an implementation of a feature toggle which applies frontend redirect logic based on the currently logged in users email address our team along with the VA.gov profile team experienced race condition errors related to the redux state. Essentially a variable value was not being set correctly due to latency issues related to feature toggles and frontend storage.
Currently most pages make three separate feature toggle calls, none of which are cached
Feature toggle which raised this issue

Suggestions to resolve:

Audit all of the vets-website feature toggles to determine features that have existed for longer than six months.
- Report out to teams that maintain these to ask them to remove the feature toggles they no longer actually need.
Release guidance on how long a feature toggle should be used in production to encourage teams to use these as temporary solutions and not permanent ones.
Investigate options around migrating facilitating a caching mechanism for Redux
- Possible options are React-Query or Redux Tool Kit Query
- Ideally this type of change would allow flipper function calls to only happen once per user unless it was determined the feature toggle results are different.

pjhill commented 2 years ago

@CBonade is going to investigate the following --

[x] Get a list of all features configured in feature toggles
[x] Look for features that have been registered in feature toggles for more than 6 months
[ ] Ping teams that are responsible for these features about potentially unregistering them from feature toggles

CBonade commented 2 years ago

Full list of feature flags registered to vets website: https://github.com/department-of-veterans-affairs/vets-website/blob/main/src/platform/utilities/feature-toggles/featureFlagNames.js

List of ones that were added to vets-website 6 months or longer ago: https://docs.google.com/spreadsheets/d/1VKuJ7RO0Py8DaJo3Z4m5rYASD5H8UsZz02R3DQV-Dvo/edit?usp=sharing

Per a chat with Joe Niquette, holding on contacting any teams about these.

rmessina1010 commented 2 years ago

@alexandec, @theodur and I were looking into the Redux angle of this issue.

RTK and React Query are still on the table, but we would like to take a closer look at the existing codebase as is.

Bet 1: Given that: the current status of flipper function calls is maintained in the Redux store, that the flags affect features globally, and that each feature-flag is thus unique…

That the root cause of the race condition stems from the async process to obtain the feature-toggles flag from the flipper function (calls to the API). So if we could observe the status of the flipper call, using existing methods, we could leverage the Redux store to sync feature-toggles cross app. Specifically, we are looking to see if there is a way to leverage the FETCH_TOGGLE_VALUES_SUCCEEDED action. https://github.com/department-of-veterans-affairs/vets-website/blob/main/src/platform/utilities/feature-toggles/index.jsx#L52-L58

Bet 2: The race condition is NOT caused by the API call to obtain the feature-toggle flag value, but instead is caused by shared resources of the rendered components.

In this case, the approach would be to have the shared resources registered in, and observed from, the Redux store, failing that, to implement RTK/ React Query.

We are trying to get a deeper understanding and looking through src/applications/personalization, but it would really save us time if we could to get a link to the code specifically implementing the feature toggle in question. Could someone on vsa-authd-exp-frontend (@mdewey) reach out @alexandec or @theodur or myself(@rmessina1010)?

alexandec commented 2 years ago

I did some discovery into the cause of the race condition. It appears the issue is occurring in the applications/auth/containers/AuthApp.jsx component. The issue I'm seeing is that the AuthApp component kicks off the redirect process inside a componentDidMount lifecycle method. componentDidMount only gets called once, and the featureToggles API request may not have finished when the method was called. Any future updates of component props don't cause the method to run again. So the redirect code just runs once and gets whatever the redux store state is at the time it runs.

The component is using connect with mapStateToProps to map the redux store values onto component props. That means mapStateToProps will be called every time the redux store state changes, and in turn the component props will change as well. Initially the featureToggle values will not be present in the store, so they won't be available as component props. But once the featureToggles API request completes, the values are placed in the store, and they are then added to the component props via connect/mapStateToProps.

A potential fix would be to use the componentDidUpdate method instead of (or in addition to) componentDidMount. componentDidUpdate runs on every prop change, so it should run every time the redux store is updated. The code inside the method would check for the flag value and only proceed if it's present. Here's an example implementation (needs to be tested of course):

componentDidUpdate() {
  const flagIsFetched = typeof this.props.redirectToMyVA === 'boolean';

  if (flagIsFetched && (!this.state.hasError || hasSession())) {
    this.validateSession();
  }
}

This implementation assumes that the redirectToMyVA prop is initially undefined until the flag is fetched, then either true or false after the fetch completes. Another option would be to check the featureToggles.loading value in the redux store. That value is true while flags are loading and false when loading has completed.

Hope this helps! Let me know if you have any other questions.

alexandec commented 2 years ago

Also, we've addressed the feature_toggles endpoint issue where it was fetched multiple times. @timwright12's PR removed two extra calls, so now we're down to a single call. Since the first call already populated the redux store with feature toggle values, the additional calls were not needed.

rmessina1010 commented 1 year ago

I. Redundant calls https://github.com/department-of-veterans-affairs/vets-website/pull/21989 This issue was solved, as it was found that the call to the API was found that redundant calls to the API were being made in the header and footer ( these calls were subsequently removed). This issue can be considered closed.

II. Caching https://github.com/department-of-veterans-affairs/vets-website/pull/22270/commits 1) having decided that the best course of action would be to store the results of the API call in sessionStorage 2) To this end we used Tim’s PR which was failing a lot of (inconsistent) cypress and unit tests 3) An issue in the destructuring of the return data was resolved, ( and also the return act as a promise to mimic the way the original data was retrieved), this solved all the unit tests issues, several cypress suites still fail. a) These mainly consist of accessibility issues and cy.await b) App. affected seems to be debt-letters, (applications/debt-letters/tests/e2e/cdp-alert.cypress.spec.js …) CypressError: Timed out retrying after 5000ms:cy.wait()timed out waiting5000msfor the 1st request to the route:features. No request ever occurred. d) this failed local as well as CI

I think the error stems from here: applications/debt-letters/tests/e2e/cdp-alert.cypress.spec.js

 cy.intercept('GET', '/v0/feature_toggles*', mockFeatureToggles).as(
      'features',
    );

I was not able to follow up on this, but my most current theory is that the intercept fails when that API is not called ( due to the API call being skipped if data was present in session storage).

III. Auxiliary custom hooks. https://github.com/department-of-veterans-affairs/feature-toggle-POC-RTT-RM An independent demo repo was made, but it originally incorporated redux-store persist, I did not have time to switch it sessionStorage, in addition these hooks would need to consider the shape of the data from the API call/sessionStorage. Altho, my advice would be to create a reduced version of the data simplified to a single object with key-value pairs like { feature:status, … } the hooks can then be ported as utility components/methods for platform/utilities/feature-toggles.

rmessina1010 commented 1 year ago

Addendum: My last commit confirms that many that the cy.wait() errors are caused by cy.intercept('GET', '/v0/feature_toggles*'... Making the test conditional and checking to see if a sessionStorage object has been set; intercepting the endpoint only if it has not resolves the issue for that particular test.

e.g.:

    if (!sessionStorage.getItem('vaFeatureToggles')) {
        cy.intercept('GET', '/v0/feature_toggles?*', mockFeatureToggles).as(
        'features',
    );
    }

    cy.wait(['@debts']);
    if (!sessionStorage.getItem('vaFeatureToggles')) {
      cy.wait(['@features']);
    }

Note that for thoroughness the session storage object should be retrieved and tested. A cursory search (cy\.intercept\('GET', '\/v0\/feature_toggles\?\*',)there are about 32 similar tests throughout vets-website that would require correction.

npeterson54 commented 1 year ago

Theo created https://app.zenhub.com/workspaces/platform-tech-team-3-6335ab9b1901b99243ce7601/issues/department-of-veterans-affairs/va.gov-team/45395 to address the caching issues. closing this discovery out.

department-of-veterans-affairs / va.gov-team

Investigate Options for Feature Toggles #44437

Issue:

Suggestions to resolve: