feat(flags): Add FeatureFlagStatusIndicator to detail view for flags #26412
Conversation
…ct-stale-flags
…ct-stale-flags
…ct-stale-flags
…ct-stale-flags
…ct-stale-flags
…ct-stale-flags
…ct-stale-flags
…ct-stale-flags
… into detect-stale-flags
|
Size Change: +214 B (+0.02%) Total Size: 1.11 MB ℹ️ View Unchanged
|
… into stale-flag-ui
| export enum FeatureFlagStatus { | ||
| ACTIVE = 'active', | ||
| INACTIVE = 'inactive', | ||
| STALE = 'stale', | ||
| DELETED = 'deleted', | ||
| UNKNOWN = 'unknown', | ||
| } | ||
|
|
||
| export interface FeatureFlagStatusResponse { | ||
| status: FeatureFlagStatus | ||
| reason: string | ||
| } |
There was a problem hiding this comment.
how is inactive differentiated from disabled in this case? Are all disabled flags also inactive? If so, I feel like we shouldn't show an indicator if that's the case – I only care about a flag being inactive/stale if I have the flag enabled in my app.
There was a problem hiding this comment.
If a flag is disabled (to be precise, flag.active = false), then the endpoint currently would only determine it to be inactive if there have been no recent evaluations. active = false does not imply inactive or stale.
I did this intentionally; a good example of why this feels correct to me is if a flag exists in staging and production projects but is only enabled in staging, you don't want to make the flag show up as stale in the prod project, unless it isn't being evaluated at all which would indicate that you aren't actually referencing the flag in code at all then (in this case we would call it "inactive", because we don't actually know for sure it is "stale", we only know it is unused).
# - STALE: The feature flag has been fully rolled out to users. Its evaluations can not vary.
# - INACTIVE: The feature flag is not being actively evaluated. STALE takes precedence over INACTIVE.
| return ( | ||
| <LemonBanner type="warning"> | ||
| <div> | ||
| <span className="font-bold">This feature flag is {flagStatus.status.toLowerCase()}</span> | ||
| {flagStatus.reason ? ` - ${flagStatus.reason}.` : ''} | ||
| </div> | ||
| <div className="text-xs"> | ||
| {flagStatus.status === FeatureFlagStatus.STALE && | ||
| ' Make sure to remove any references to this flag in your code before deleting it.'} | ||
| {flagStatus.status === FeatureFlagStatus.INACTIVE && | ||
| ' It is probably not being used in your code, but be sure to remove any references to this flag before deleting it.'} | ||
| </div> | ||
| </LemonBanner> | ||
| ) | ||
| } |
There was a problem hiding this comment.
I'll preface this by saying I think the general shape of this work makes sense, the API is sensible, and I think we're really close to something useful here. That said, I'm not sure if what we have here quite nails it, and I think we should try sanding this experience a bit to work towards something more useful. Here's my 2 cents, would love to hear your input as well:
- I think that providing a banner about a stale flag without giving users a clear way to act on it leads to UI clutter. Instead, I'd suggest adding a some sort of staleness visual indicator (like a tag) next to the enabled/disabled toggle, with a tooltip explaining what it is and what it means. The banner feels too heavy.
- re: the tooltip, I think it would be cool if this tooltip linked out to some docs that explain what staleness is and how it's calculated, users might be confused (this can be in a separate PR). The less non-necessary information we surface on first glance at the UI (while still making any context easily discoverable), the better IMO.
- do you plan on making staleness status something that appears as a column in the "all flags" view that can then be filtered on? IMO that feels like a necessary addition to this change (unless this change is going to go out behind a feature flag, which it doesn't appear to be at the moment), since opening up a flag with no staleness indicator in the list only to see this banner feels jarring
- As a user, I want to do what to do with my stale flag, like, I see this banner (or tooltip, or whatever) – so now what? Feels like we should tell them what to do to deal with it (and this could also be outlinked in the docs, maybe we have a flow like staleness indicator tooltip -> "this flag is [stale](link to staleness definition) and will always evaluate to $response. [Here's how you can deal with it](link to docs on recommendations on managing stale flags)" Here's how Harness does their docs on this (we can beat this IMO).
There was a problem hiding this comment.
I think that providing a banner about a stale flag without giving users a clear way to act on it leads to UI clutter. Instead, I'd suggest adding a some sort of staleness visual indicator (like a tag) next to the enabled/disabled toggle, with a tooltip explaining what it is and what it means. The banner feels too heavy.
That's fair - I think I was erring towards heavier because I wanted to make sure we get feedback on false positives (saying something's stale when it's not) as quickly as possible so that we can iterate on this functionality faster (automate stale flag cleanup, etc.)
re: the tooltip, I think it would be cool if this tooltip linked out to some docs that explain what staleness is and how it's calculated, users might be confused (this can be in a separate PR). The less non-necessary information we surface on first glance at the UI (while still making any context easily discoverable), the better IMO.
Love that, I think some docs explaining staleness similar to the ones you linked below https://developer.harness.io/docs/feature-flags/use-ff/ff-creating-flag/manage-stale-flags/ makes a lot of sense
do you plan on making staleness status something that appears as a column in the "all flags" view that can then be filtered on? IMO that feels like a necessary addition to this change (unless this change is going to go out behind a feature flag, which it doesn't appear to be at the moment), since opening up a flag with no staleness indicator in the list only to see this banner feels jarring
The only reason I didn't want to do this right away was that staleness requires CH queries, which of course is unprecedented for GET /.../feature_flags/. I think a cron or scheduled task that denormalizes the FeatureFlagStatusChecker.get_status result into a PG column so that we can sanely render this into the all flags view would be the clear next step here! This scheduled task, imo should also be responsible for an (opt-in) automated archival process, but that might be a second next step
As a user, I want to do what to do with my stale flag, like, I see this banner (or tooltip, or whatever) – so now what? Feels like we should tell them what to do to deal with it (and this could also be outlinked in the docs, maybe we have a flow like staleness indicator tooltip -> "this flag is [stale](link to staleness definition) and will always evaluate to $response. [Here's how you can deal with it](link to docs on recommendations on managing stale flags)" Here's how Harness does their docs on this (we can beat this IMO).
Yeah, Harness has a lot of cool automations around "ready for cleanup" status. I think realistically the things what we can / should do right now are:
- Give users a way to update the status manually. If it's truly not stale and they don't want anyone deleting it; they should have the power to indicate that much like Harness's "Mark as not stale" functionality. Plus, this gives us a built-in way of quantifying how often false positives are happening.
- Introduce
archivedto flags in addition todeleted. Put archived flags in their own section on the FF main page. In that section, allow admins to enable auto-archival moving forward. This prevent breaking changes from occurring if the flag suddenly gets checked again (I am assuming here that deleted flags are already excluded from /decide responses, but haven't checked that yet).- Of course, the end goal is to help them quickly / easily delete stale flags. This would actually help their /decide response latency go down by some amount depending on how complex some of the stale flags are. I think a nice "Bulk delete archived flags" which does one last check for recent flag calls would be a nice convenience here.
- I think achieving this last step here will drive people to actually deal with their stale flags more. It is synonymous to a gmail spam box. You have control over what goes there, and you have a one-click way of cleaning it out too.
There was a problem hiding this comment.
Personally, I think doing anything in the territory of code references is a little out of scope for now. We can go very shallow (if we know your github / gitlab repo's URL, we can deep link you to a code search for the flag key), or very deep (use polygot piranha like Harness does), but in either case we're getting into the unprecedented world of git integration, and I think we'll want to consider that it's own batch of work with its own RFC
There was a problem hiding this comment.
super fair, and thanks for the responses!
… into detect-stale-flags
havenbarnes
changed the title
📸 UI snapshots have been updated4 snapshot changes in total. 0 added, 4 modified, 0 deleted:
Triggered by this commit. |
| @@ -85,7 +85,7 @@ def is_flag_fully_rolled_out(self, flag: FeatureFlag) -> tuple[bool, FeatureFlag | |||
| ) | |||
| if multivariate and is_multivariate_flag_fully_rolled_out: | |||
| return True, f'This flag will always use the variant "{fully_rolled_out_variant_name}"' | |||
| elif self.is_boolean_flag_fully_rolled_out(flag): | |||
| elif not multivariate and self.is_boolean_flag_fully_rolled_out(flag): | |||
There was a problem hiding this comment.
Meant to push this to #26340, caught an edge case (the new test checks for this)
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
There was a problem hiding this comment.
I don't want to block your work so far (and I think this is good enough to ship), but I do think at some point in the nearish future (maybe once we really have our head wrapped around how we want this banner to look and feel), it might be worth adding a snapshot test of some sort for this feature, given that it's an easy thing to add and will give our UI an extra layer of diligence in CI.
Problem
#16497
Follow up to #26340
Changes
Adds a banner to detail view which shows users that a flag is stale or inactive and explains how this was determined (see Loom below)
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
Yes
How did you test this code?
https://www.loom.com/share/75653b58b4b740d2830e4f8b8aef9820