SPIKE: Audit feature toggles for opportunities to retire

jilladams commented 2 years ago

Description

Request from @RachalCassity: Console Services Team is evaluating site performance, and identified that feature toggles (Flipper) in vets-api and vets-website repos.

Feature toggles endpoint is called multiple times per page, and causes Sentry errors / site latency.

Proposed list of Ownership:

https://dvagov-my.sharepoint.com/:x:/g/personal/jill_adams1_va_gov/EWJYp5QIYQ5Bng7ynlMX9hIBxXjsVBNANaugiektKZ1fcQ (shared with DaveC)
Duped doc as Google sheet: https://docs.google.com/spreadsheets/d/1yQb5oDshNj7y0nvtF3PaZ9OLhtTuZ2aO8t4_n51YBWU/edit#gid=237722104&fvid=1552255965

Existing flags:

CMS Feature toggles follow up work

https://prod.cms.va.gov/admin/config/system/feature_toggle

FEATURE_ALL_HUB_SIDE_NAVS - not enabled, always false. Remove conditional checks in content-build and delete in CMS.
FEATURE_SINGLE_VALUE_FIELD_LINK - relates to promo blocks. Remove conditional checks in content-build and delete in CMS.
Complete, no-op
FEATURE_HEADER_MEGAMENU - was enabled in CMS, but doesn't show up in content-build. Steve deleted from Prod.
FEATURE_IMAGE_STYLE_23 - disabled in CMS, doesn't appear in content-build. Steve deleted from Prod.
FEATURE_FOOTER_MENU - disabled in CMS, doesn't appear in content-build. Steve deleted from Prod.
FEATURE_CHANGE_LEADERSHIP_LINK- disabled in CMS, doesn't appear in content-build. Steve deleted from Prod.
FEATURE_RECURRING_EVENTS - disabled in CMS, doesn't appear in content-build. We know this logic was removed during Recurring events 2022Q2 bug bashing. Steve deleted from Prod.

@swirtSJW to follow up with a commit to remove these flags from code if they don't effectively disappear from config based on DB delete, based on #5855

Feature flippers follow up work

https://api.va.gov/flipper/features

Audit of PW owned flags or ones we need to verify: https://docs.google.com/spreadsheets/d/1yQb5oDshNj7y0nvtF3PaZ9OLhtTuZ2aO8t4_n51YBWU/edit#gid=237722104&fvid=1552255965 (Public Websites filter view enabled)

Acceptance criteria

[X] Schedule sync session with Dave C, Public Websites FE / PM, Facilities FE / PM to audit feature toggles, identify utility / ownership
[X] https://api.va.gov/flipper/features
[X] https://prod.cms.va.gov/admin/config/system/feature_toggle
[x] Confirm ? Flippers with Dave
[x] Assess 2 CMS flags for effort to remove content-build code prior to deleting CMS flag
[x] Assess PW flags in Flipper audit sheet & determine which flags can be removed (Column F), Notes in Column G
[ ] If a flipper is not being called from anywhere in code, it can be removed.
[ ] If a flipper is being called at all, identify in doc and do not remove. Describe usage, so we can understand whether or not it can be removed.
- [ ] Summarize tasks, risks, and effort of removing.
- [ ] FE team + Product + Dave will review findings to decide

jilladams commented 2 years ago

The original list that Rachal provided in https://docs.google.com/spreadsheets/d/1LZjJIKZ-cteRxqwFIAbnAR2Sa32liP-5Y3WQ_UqZCKk/edit#gid=0 are all actually Facilities toggles, and have been handed off to Roger / David Shannon in Slack.

jilladams commented 2 years ago

@RWEagans @dsinla FYI that in PW refinement today, Dave suggested that PW, Facilities, and Dave do the requested audit of feature flags as a joint meeting, since he holds so much history / context. I'll own scheduling the meeting.

swirtSJW commented 2 years ago

ryguyk commented 2 years ago

Estimated at 5 initially, but then changed to 3 to align with this being only a spike. The implementation will be separate. Some of this discovery work has already been done prior to this estimate, but it seems like that fits into the total of 3. There shouldn't be too much left to do, but there's some.

apisandipas commented 1 year ago

The spreadsheet list in the AC has been updated with an indication of which flags can be deprecated / removed and some notes on the effort level for each.

jilladams commented 1 year ago

Thanks! - looks like many many flags are either not mentioned in the code (vets-website / content-build) at all, or only mentioned in the flags file, so those could be removed. There are a couple I would love to get @ryguyk 's eyes on (in this sheet, the "Can be removed?"/"Notes" columns for Brian's assessment) just to measure twice before we cut once.

jilladams commented 1 year ago

Checked in with @ryguyk via Slack to confirm if this can close. He flagged some marked Removable that need a 2nd look -- he will close loop tomorrow. Added to Sprint 71. FYI, @davidconlon / @wesrowe

jilladams commented 1 year ago

Brief meeting with Ryan, Josh, Wes, me:

Ryan flagged that the original pass at this SPIKE mostly validated where the snake case version of the flag name appears in code, e.g. sign_in_service_enabled. However: in most cases, this snake case is mapped to a camelCase version of the flag in vets-website/src/platform/utilities/feature-toggles/featureFlagNames.js, e.g. signInServiceEnabled.
Searching for the camelCase versions of the name then often returns many more mentions / uses of the flags/related logic than we originally found.

So in light of that, Step 1: I've updated the Sheet to flag which items need re-review for usage / how widely spread they are.

Step 2: How do we know for sure it's ours to remove?

When we find where flags are referenced, do we own those files? Who does? Let's make note of it.
If we find again that some of the flags are really not referenced anywhere else, they definitively can be removed.

Step 3: if it's used & we own the files: how do we decide if something that's widely spread could actually be removed?

Has it been actually flipped? In how long? (e.g. if a flipper has been enabled for 3 years, we could account for that in the code and remove the flipper, etc.) Flipper doesn't keep history, but: we could git blame to see tickets & understand how old the flipper is / state of the feature, etc.
- If we can aggregate those notes in the sheet, PM / DM will need to assess the history to understand in order to say yay / nay & cut follow up tickets.

Step 4: actual removal (will happen in separate tickets in this epic) Some flippers that have been on for ages, e.g. show_events_v2, can probably be removed. However to do that, need to:

Remove the code that would be run if the flipper were off
Make flipper = on behavior the default state
Then can remove the flipper This is removing tech debt.

For flippers that have been off for ages, it's a little more unclear why, so we'll need to interrogate what it's doing, if anything.

Moving from Review/QA back to Stretch, so we can review in an upcoming sprint, likely after Homepage work settles.

apisandipas commented 1 year ago

Google Sheet has been updated to reflect audit with “camel cased” flag names.

jilladams commented 1 year ago

Bueno! Let's you, me, Ryan, Wes do a quick meeting on it to review the outcome, talk priority / lift, so Wes & I can start cutting tickets to remove the ones we can. Could you schedule?

jilladams commented 1 year ago

Reviewed with Ryan, sheet updated with a few additional notes. Closing, and action items from here are:

Jill create tickets in this epic to stub out tickets for the Yes to remove items.
Ryan (and Bryan as needed / helpful) to add dev-facing notes to those

jilladams commented 1 year ago

Tickets created in the epic.

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