isomerpages / isomercms-backend

A static website builder and host for the Singapore Government
5 stars 1 forks source link

chore(growthbook): handle refreshes at module level #1228

Closed timotheeg closed 5 months ago

timotheeg commented 7 months ago

Problem

Using autorefresh:true in growthbook.loadFeatures() is an anti pattern.

autorefresh:true translates to a subscribe on the instance, which means the flags may be updated during the lifetime of a request.

In recent version of growthbook, autorefresh:true has been deprecated in favour of a more explicit constructor setting subscribeToChanges:true, with a documented behaviour warning for middlewares in documentation:

    // Important: make sure this is set to `false`, otherwise features may change in the middle of a request
    subscribeToChanges: false,

In addition, when the first request which loads the featureflag is in a middleware (typically the health endpoint at deployment), then the SSE call is created in context of the request, creating abnormally huge traces lasting days with thousands of spans. Example here or here

image

Solution

  1. Upgrade growthbook to 0.36 to have have the new subscribeToChanges property available
  2. Create a growthbook instance at startup time
  3. load the feature immediately and subscribe to changes
  4. Make the growthbook instances created in middleware NOT use subscribeToChanges:true

Bonus: Simplify the promise chain just a bit

Breaking Changes

Tests

timotheeg commented 6 months ago

@chin, so with growthbook, there's a shared internal cache for a given upstream. Such that even if multiple GB instances are created in multiple concurrent requests, they use the same set of feature flags.

If the cache is stale, then a given GB instance calling loadFeatures() will make a HTTP call to fetch the flags, which will take some time on that request.

The idea is that we want the cache to be fresh for all (or most) of the incoming requests. So we need to have one instance in charge of refreshing the internal shared cache. That's what the module level instance is for. Its job is not to be accessed for flag values, its job is to be long lived (life of the node process), and update the shared cache.

In the middleware we still create GB instances, but these are short lived, just for the duration of the requests. They will reference an immutable set of flags constructed from the cache. So during the lifetime of a request, the flag values should be consistent, which is what we want if a given flag is accessed multiple times at different times over the life of the request.

The latest growthbook makes the subscription model more explicit, so I'm wondering if I shouldn't just add the upgrade as part of this PR too 🤔

timotheeg commented 6 months ago

Just an update I dropped the ball on this. I want to upgrade growthbook to use the latest so the sync option is more obvious. I'll do that by end of the week.

timotheeg commented 5 months ago

Upgraded GrowthBook to 0.36 and used the new more explicit instantiation parameter subscribeToChanges:true|false, which makes it clearer why the module level instance exists.

Also fixed the logging to log errors correctly.

@seaerchin