Refactor featureFlags proposal

nickoferrall commented 3 months ago

Problems

Today, featureFlags are an array of enums in both the organization and user tables. Soon, we may want to add a team feature flag too, e.g. Team Insights.

Some flags exist in both tables, e.g. noAISummary.

Feature flags stay in the app until we decide to remove them. zoomTranscription and standupAISummary, for example, have been in the app for over a year. This leads to our experience using the Parabol org to be quite different to the experience of all other users.

Proposal

Create a featureFlag table with the following properties:

id (string)
featureName (enum)
isEnabled (boolean)
description (string, optional)
userId (string, optional)
orgId (string, optional)
teamId (string, optional)
expirationDate (timestamp, optional)
createdAt (timestamp)
updatedAt (timestamp)

Devs are encouraged to set an expiration date for the feature flag to force us to make a decision on a feature, but it's an optional field, so we don't have to include it if we're not sure.

In the server, we can check if the feature flag exists like so:

const Organization: OrganizationResolvers = {
  featureFlag: async ({ id: orgId }, { name }, { dataLoader }) => {
    const flags = await dataLoader.get('featureFlagsByOrgId').load(orgId);
    const flag = flags.find((flag) => flag.featureName === name);
    return !!(flag && flag.isEnabled && (!flag.expirationDate || new Date(flag.expirationDate) > new Date()));
  },

And in the client:

const organization = useFragment(
  graphql`
    fragment RetroDiscussionThreadHeader_organization on Organization {
      publicTeams: featureFlag(name: PUBLIC_TEAMS)
      relatedDiscussions: featureFlag(name: RELATED_DISCUSSIONS)
    }
  `,
);

const hasPublicTeamsFlag = organization.publicTeams;
const hasRelatedDiscussionsFlag = organization.relatedDiscussions;

Let me know what you think!

Dschoordsch commented 3 months ago

Why have userId and teamId if the interface only allows querying for orgId? Having featureName being an enum is a pain if we want to remove a flag again at some point.

nickoferrall commented 3 months ago

Why have userId and teamId if the interface only allows querying for orgId?

I was thinking we'd have a featureFlag field in Organization, User, and Team, but making it an interface instead makes sense.

Having featureName being an enum is a pain if we want to remove a flag again at some point.

Yeah, would you prefer it to be a string? My concern is that it'd be easy to make a typo

Dschoordsch commented 3 months ago

Yeah, would you prefer it to be a string? My concern is that it'd be easy to make a typo

I think we need to run a migration in any case, so we could add a check on a string rather than a full blown enum type. But maybe there are cleaner ways?

mattkrick commented 3 months ago

I really like querying a single flag!
For preventing typos, we could keep an array of strings in the app to check against, that way we don't have to migrate the DB
IMO this table isn't fully normalized. 1 user has many feature flags. 1 feature flag has many users, that's M:N. By denormalizing, we're saying each user/team/org has a unique feature flag unto themselves, ie different expiration dates, descriptions, is this what we want? IMO, a feature flag is an experiment with a defined start/stop date, and if the expirationDate is a function of the team it is assigned to, we're breaking that. I'd say we need a FeatureFlag table and then a FeatureFlagOwner crosstable where we keep userId, teamId, orgId, but I could be swayed either way.
In the FeatureFlag table, I'd add a scope of User, Team, Organization. 1 flag has 1 scope.
In the crosstable, I'd make the owner FK mutually exclusive, (ie only userId, teamId, XOR orgId can be non-null). Which one is decided by the featureFlag.scope.
I'd get rid of isEnabled as we already have expirationDate
i'd opt for renaming it to expiresAt just to keep naming consistent (At suffix is a DateTime). I'd also make it non-null because we want some teeth to it!

nickoferrall commented 3 months ago

A FeatureFlagOwner crosstable makes sense to me! The description and expirationDate should always be the same, so that'll prevent redundant data.

Updated Proposal

Create a FeatureFlag table:

id (string) 
featureName (string) 
scope (enum) -- 'User', 'Team', 'Organization'
description (string, optional)
expiresAt (timestamp) -- non-null
createdAt (timestamp)
updatedAt (timestamp)

Create a FeatureFlagOwner crosstable:

id (string) 
featureFlagId (string)
userId (string, optional) -- mutually exclusive with teamId and orgId
teamId (string, optional) -- mutually exclusive with userId and orgId
orgId (string, optional) -- mutually exclusive with userId and orgId
createdAt (timestamp)
updatedAt (timestamp)

We could just have an ownerId in the crosstable, saving two columns, but specifying orgId, teamId, and userId is more explicit.

In the server:

const Organization: OrganizationResolvers = {
  featureFlag: async ({ id: orgId }, { name }, { dataLoader }) => {
    if (!validFeatureFlagNames.includes(name)) {
      return standardError(...);
    }
    const flags = await dataLoader.get('featureFlagsByOwnerId').load(orgId);
    const flag = flags.find((flag) => flag.featureName === name);
    return !!(flag && new Date(flag.expiresAt) > new Date());
  }
};

const User: UserResolvers = {
  featureFlag: async ({ id: userId }, { name }, { dataLoader }) => {
    if (!validFeatureFlagNames.includes(name)) {
      return standardError(...);
    }
    const flags = await dataLoader.get('featureFlagsByOwnerId').load(userId);
    const flag = flags.find((flag) => flag.featureName === name);
    return !!(flag && new Date(flag.expiresAt) > new Date());
  }
};

In the featureFlagsByOwnerId dataLoader, we'd join the two tables, and check where scope === User && userId === ownerId or scope === Organization && orgId === ownerId, etc.

In the client:

const organization = useFragment(
  graphql`
    fragment RetroDiscussionThreadHeader_organization on Organization {
      publicTeams: featureFlag(name: "publicTeams")
      relatedDiscussions: featureFlag(name: "relatedDiscussions")
    }
  `,
);

const hasPublicTeamsFlag = organization.publicTeams;
const hasRelatedDiscussionsFlag = organization.relatedDiscussions;

nickoferrall commented 3 months ago

Hey @mattkrick, it'd be great to get your thoughts on this! I can then implement it for insights

nickoferrall commented 3 months ago

Hey @mattkrick, bumping this one 🙏

mattkrick commented 3 months ago

thanks for the ping! this looks good. we can probably get rid of updatedAt and id on the cross table since we won't use any lookups by id & these rows are going to be inserted or deleted only.

On the crosstable, i like the thought of keeping the FKs separate. It's explicit and doesn't require GUIDs. On the other hand, we have GUIDs, it'll require 3 indexes. I think your solution is a good one!

One note-- the query will be a little expensive, so make sure to run the explain on it to make sure you're using all the indexes you can. For example, on TeamMember you'll have to include and "isNotRemoved" = true in order to use the index since we only index on active team members.

ParabolInc / parabol