Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show "stale" flags #184

Closed
dabeeeenster opened this issue Jul 14, 2021 · 16 comments · Fixed by #3606
Closed

Show "stale" flags #184

dabeeeenster opened this issue Jul 14, 2021 · 16 comments · Fixed by #3606
Assignees
Labels
api Issue related to the REST API feature New feature or request front-end Issue related to the React Front End Dashboard priority-customer Customer requests tech-debt Technical debt issues
Milestone

Comments

@dabeeeenster
Copy link
Contributor

dabeeeenster commented Jul 14, 2021

This will help people identity flags that have not changed for a while and are good candidates for removal.

Add a button (somewhere! Not sure where - maybe by the current tags or by the search filter top right) to filter flags that haven't had a state change in N days (suggest N=30 initially but can be changed by the user).

The following events should be considered a state change:

  • Creation of flag
  • Modifying core flag state (boolean, config, MV)
  • Adding/removing/changing segment override
  • Adding/removing Identity override

UI

Add a "Last Modified" option here:

image

@dabeeeenster dabeeeenster added feature New feature or request front-end Issue related to the React Front End Dashboard api Issue related to the REST API labels Jul 14, 2021
@matthewelwell
Copy link
Contributor

Add a last_modified column to the features_featurestate table. Update it to the current timestamp whenever one of the events listed above is triggered for that particular featurestate.

I think the key thing really is when a feature state associated with a Feature changes. Sounds trivial but I think this is an important nuance. It actually makes this quite simple (I think) in that we just add an updated_at = models.DateTimeField(auto_now=True) to the FeatureState model and then add a SerializerMethodField to the relevant Feature serializer that just gets the max updated_at for all it's associated feature states (in a given environment perhaps since the view of a feature is always per environment) and then determines if that is in the last N days.

We should also look into whether we even need the updated_at field since we already store the history for each feature state change. This could probably be achieved by looking at the latest history record for all the relevant feature states. I think this would require an additional query to the above though.

@dabeeeenster
Copy link
Contributor Author

Yeah I guess just depends if we want to denormalise that bit of data. I'm sort of 50/50 on that. Maybe better to just compute it from the history.

@dabeeeenster
Copy link
Contributor Author

Update JSON to include number_of_days_since_last_updated or similar for list of flags

@gagantrivedi
Copy link
Member

Since we decided to implement this by adding updated_at on feature state, it will not work for deleting segment and identity overrides as we just delete the associated feature state in those cases.

@gagantrivedi gagantrivedi added this to the Audit Log Refactor milestone Sep 15, 2021
@dabeeeenster dabeeeenster added tech-debt Technical debt issues and removed tech-debt Technical debt issues labels Feb 15, 2022
@gagantrivedi
Copy link
Member

gagantrivedi commented Apr 22, 2022

I think what we are essentially trying to do here is something very similar to what audit log refactor will achieve, i.e: track state.
The current problem is where do we put update_at? We can't put that on feature state since it gets delete in certain cases, we can't put in on feature unless we are okay with loosing environment level granularity. Another option would be to soft-delete feature states, but that will come with its own drawbacks(like: overriding delete on the model to soft-delete will not work with CASCADE https://docs.djangoproject.com/en/dev/ref/models/fields/#django.db.models.CASCADE)

@matthewelwell
Copy link
Contributor

It would be good to apply rules to stale flags based on flag naming conventions. For example, if a flag begins with long-lived., then you may not want to alert users about it being stale.

@dabeeeenster
Copy link
Contributor Author

It would be good to apply rules to stale flags based on flag naming conventions. For example, if a flag begins with long-lived., then you may not want to alert users about it being stale.

Wouldnt tags be better for this?

@matthewelwell
Copy link
Contributor

Wouldnt tags be better for this?

Yes, perhaps.

@matthewelwell
Copy link
Contributor

I've been looking into this further and I think the correct solution for this actually puts us on the right path for #794 as well. My suggestion is that we have:

A new model entity: EnvironmentFeatureVersion. This includes a link to the environment, the feature and a timestamp (plus some other meta data). A change request can optionally be linked to it if change requests are required in a given environment.

Instead of the FE directly updating feature states, it should create a new EnvironmentFeatureVersion object and then add feature states to it as needed. These can be segment overrides, identity overrides and / or the environment default. Once the user is happy with the edits, they can release the version or attach it to a change request for approval.

Once this is in place, stale flags just becomes a case of checking when the last version was created and determining if that is longer than the period allowed for active flags.

Some outstanding questions:

  • How do we retrieve the flags for an environment with this separate version concept?
  • How do we retrieve the flags for an identity with this separate version concept?
  • How do we migrate all of the data to fit with this concept?
  • Does a version need to include ALL current active feature states for that feature / environment combination? (probably, yes) This likely means we will need to impose hard limits on the number of segment overrides and identity overrides (something we've discussed elsewhere already - see Enable per-identity overrides in Local Evaluation mode #1762)

Based on this, I believe that this no longer relates to the AuditLog refactor. As such, I'm going to remove the AuditLog refactor label.

Thoughts on this @dabeeeenster @kyle-ssg @gagantrivedi ?

@dabeeeenster
Copy link
Contributor Author

This sounds good to me. Feature versioning is a big improvement too that opens up other powerful features.

We could store denormalised data, with the latest version of EnvironmentFeatureVersion being copied into the existing data structure? That basically solves all the outstanding questions?

@dabeeeenster
Copy link
Contributor Author

@kyle-ssg other than the order by created data (and showing the created date when ordering by it) are there any other ideas you have in terms of surfacing this feature?

@matthewelwell
Copy link
Contributor

From speaking to @novakzaballa about this, the current plans are:

  1. Add a new configurable attribute to the project which stores the number of days without modification before a feature is considered stale
  2. In the feature list in an environment, show the date that a feature was last modified in a given environment (and add the ability to sort by this value)
  3. Add a new attribute to the project to enable 'automatic tagging of stale flags' - this will trigger a job that would grab all the flags for a project once per day and, if it hasn't been modified more recently than the period configured on the project, an automatically generated system tag will be added to the feature.

Optionally (I don't know if this is necessary if we apply the tag):

  1. Add a 'reports' section to the UI. The first of which could be a 'stale flags' report. This would just show those flags that are considered stale. The reason I marked this as optional is that I believe that the features view will be able to show this with the correct filters applied based on the above requirements so a 'report' isn't necessary.

@novakzaballa
Copy link
Contributor

@matthewelwell, while this is aligned with what we talked, my suggestion was slightly different.

  1. At project settings, Allow the user to set the days elapsed without changes to consider a flag stale. (totally aligned)

  2. Add a stale flags section to the UI. I think it is important for users, for enterprise users in particular, to have a way to go straight to the list of stale flags, in that list we could display the last modified date, and/or days elapsed, and optionally with a toggle, we could show all the flags so they can see flags close to be stale as well. I think that adding another column to the flags view and the ability to sort could be too much for that screen, but I think we can revisit this item after Akveo has a solution for that view.
    image

  3. For the Flags view I think that tagging the feature flag as stale should be enough, since we already offer an option to filter flags in that view by tag. We probably will need to mark some of our tags a system tags so they are included by default in every environment and can't be deleted, in fact, we already have some system tags as we mentioned.

@dabeeeenster
Copy link
Contributor Author

I like the idea of using tags to power this. After this work we will have > 1 "system tag" I believe? We should maybe display those in a different way somehow so people know that they are system generated?

Also agree with @novakzaballa this feature deserves a left hand nav item.

@matthewelwell
Copy link
Contributor

As discussed with @kyle-ssg, we will rely on the tag here. If we want a left hand nav item, the FE can simply use the existing endpoint and filter by the relevant tag. Tags themselves will include a new field which can be a shared identifier between the FE and the BE to allow for this. It will also allow the FE to implement some payment gating logic to prevent customers from using stale flags if their plan does not meet the requirements.

@matthewelwell
Copy link
Contributor

@kyle-ssg this is now present in staging. There are a few things the FE needs to handle:

  1. The tags endpoint returns 2 new fields is_system and type. The FE should probably show system tags in a different way to regular tags and prevent updates / deletes (although I still need to prevent this at the API level too). The type attribute should be used to base any custom logic. For example, the Stale tag returns type: "STALE", the FE can base it's decision to gate stale flags based on plan on this (but use it as an upsell opportunity). Regular tags will return with type: "NONE"
  2. Projects have a new attribute stale_flags_limit_days which should be configurable in the project settings (again dependent on plan).

At the moment, the 'Stale' tag is created when a project is created, but I'm wondering whether it should actually be created if the stale_flags_limit_days attribute is set to a numeric integer > 0. This shouldn't change anything on the FE, however.

@kyle-ssg kyle-ssg linked a pull request Mar 13, 2024 that will close this issue
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Issue related to the REST API feature New feature or request front-end Issue related to the React Front End Dashboard priority-customer Customer requests tech-debt Technical debt issues
Projects
Status: Done 2023
5 participants