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

fix: bootstrap posthog feature flags #320

Merged
merged 5 commits into from
Oct 31, 2024
Merged

Conversation

codeincontext
Copy link
Collaborator

We use posthog to set feature flags

We can use feature flags for:

  • Giving specific users access to a features
    • internal, beta users, etc
  • Toggling features independently of code releases
    • merging work to features in development, instead of ling-lived feature branches
    • merging work that hasn't been QA'ed yet
    • disabling features which have failed QA or had a problem in production

Problem

When we tried a feature flag for download-all, we found that they weren't working if you hadn't given cookie consent. I spoke to posthog and they said that feature flags should work with opt_out_capturing enabled. It sounds like a quirk of the way we're interacting with the consent banner

Solution

The most robust way to load flags from posthog is to bootstrap them from the server. Then they will be immediately available on the frontend rather than relying on a frontend call

We don't really want to delay page load with the posthog call. We could cache it in KV for X hours, but then we would need to clear the cache when toggling a flag. Another option is local flag evaluation

With local flag evaluation, the posthog client periodically fetches the definitions for the flags, and will evaluate them for each user on the server. Because we're using edge functions, we need to cache those definitions so I've created a fetch client to handle that

If we also want to bootstrap flags that look at properties like the user's email address then we will need a follow up to make that available when we bootstrap. I have ideas for that, but one step at a time...

Copy link

vercel bot commented Oct 30, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
oak-ai-lesson-assistant ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 31, 2024 11:32am

@codeincontext codeincontext force-pushed the feat/feature-flag-bootstrap branch from 6ac8fbf to 4dbe327 Compare October 30, 2024 13:57
@codeincontext codeincontext changed the title Bootstrap posthog feature flags fix: bootstrap posthog feature flags Oct 30, 2024
@codeincontext codeincontext requested a review from stefl October 30, 2024 13:59
Copy link

github-actions bot commented Oct 30, 2024

Playwright test results

passed  13 passed
skipped  1 skipped

Details

report  Open report ↗︎
stats  14 tests across 13 suites
duration  1 minute, 59 seconds
commit  c44e1f1

Skipped tests

No persona › tests/auth.test.ts › authenticate through Clerk UI


const setKv = async (response: Response) => {
const value = await response.text();
await kv.set(KV_KEY, value, { ex: ONE_MINUTE });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need kv.set<some type>?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is, I don't know if it matters to us what the shape is if we're just caching it. Posthog's own code that calls fetch doesn't actually put a type on it either, so it could be a bit of extra effort for something that we don't need to validate and could drift from the type

https://github.com/PostHog/posthog-js-lite/blob/b853833c126ba713eb25696c96a3f0a6193ea2f0/posthog-node/src/feature-flags.ts#L422

};

const getKv = async () => {
const value = await kv.get(KV_KEY);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here I think if we use kv.get<some type> we can validate it matches the type? I'm not sure where I saw this, but perhaps look up whether this is possible?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did have a look, but I haven't seen it in the docs. I guess for runtime validation it would need to be an argument, and then it could use something like zod. But that could be extra effort for what we actually need

};

export const featureFlagsPollingInterval =
process.env.NODE_ENV === "production"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another candidate for a isEnvironment helper?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly, though I wonder if we could get a similar result if we have stricter types on the env vars

} from "./featureFlagEvaluation";

const host = process.env.NEXT_PUBLIC_POSTHOG_HOST as string;
const apiKey = process.env.NEXT_PUBLIC_POSTHOG_API_KEY || "*";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sonar prefers ??

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually do too, but I wonder if there's a greater chance of an empty string on an environment variable?

Copy link

@codeincontext codeincontext merged commit b2eba2a into main Oct 31, 2024
13 checks passed
@codeincontext codeincontext deleted the feat/feature-flag-bootstrap branch October 31, 2024 11:41
@oak-machine-user
Copy link
Collaborator

🎉 This PR is included in version 1.14.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants