feat(distributed-cache): Add distributed cache support - #310
Conversation
dustinbyrne
left a comment
There was a problem hiding this comment.
hey @avasenin, thanks for putting this together! i've used an agent to compare this to other implementations and have some findings below. i'm around if you want any of these taken off your plate. otherwise i'll leave it to you!
Share the server-controlled minimal_flag_called_events gate through the cached payload so that followers emit the same $feature_flag_called events as the instance that fetched the definitions. Validate provider data at the cache boundary: definitions without a flags key, and multivariate variants without a rollout percentage, are logged and treated as a cache miss instead of erasing the definitions already loaded or panicking during preprocessing. Order and bound provider cleanup. shutdownPoller now waits for the polling loop to return before calling Shutdown, and uses the client context rather than its own 30 second budget, so a short CloseWithContext deadline is honoured. Force keyed literals on FlagDefinitionCacheData, add the changeset, and fix the instance count in the example. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
… guide Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
…ments Co-Authored-By: Claude <noreply@anthropic.com>
…annels Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
|
hey @dustinbyrne . i think i finished with all comments. let me know what do u think |
dustinbyrne
left a comment
There was a problem hiding this comment.
thanks @avasenin! this is looking great. a couple of additional items as i'm looking through this. again, happy to take anything off your plate if you wish.
lastly - and i missed this on this first look - we require commit signing in order to merge to main. can you enable that and rebase/force push to sign your commits?
| poller.Logger.Warnf("[FEATURE FLAGS] Polling loop did not stop before the shutdown deadline: %s", ctx.Err()) | ||
| } | ||
|
|
||
| poller.shutdownCacheProvider(ctx) |
There was a problem hiding this comment.
Keep provider cleanup ordered after active polling work
When ctx.Done() wins the select above, this still calls provider Shutdown even though the poller has not finished. Provider operations receive context.Background(), so client cancellation cannot stop them.
A public-client regression with a context-aware provider and a 20 ms CloseWithContext deadline reproduces Shutdown overlapping an active ShouldFetchFlagDefinitions, followed by GetFlagDefinitions after cleanup returns. Depending on the provider, that can use a closed connection or acquire a lock after cleanup already attempted to release it.
Consider keeping the caller's wait bounded while cancelling provider work, preventing further provider calls, and ensuring cleanup runs only after active polling finishes. Add a regression combining an active provider operation with an expiring shutdown deadline; the existing tests cover ordering and timeout separately.
| type FlagDefinitionCacheData struct { | ||
| _ struct{} | ||
|
|
||
| Flags []FeatureFlag `json:"flags"` |
There was a problem hiding this comment.
Preserve the complete shared-cache payload rather than the evaluator model
Using []FeatureFlag makes the cache payload a projection of Go's evaluator model. That type omits fields such as flag id and version: I reproduced both disappearing when endpoint-shaped JSON is decoded into FlagDefinitionCacheData and marshalled again, as the file-cache example does. A different SDK or SDK version reading the same cache may need fields this evaluator does not use.
Consider using json.RawMessage at the provider boundary and retaining the original endpoint-shaped definition payload for publication, with typed decoding kept inside the SDK for validation and evaluation. Serializing the existing typed model into RawMessage would still lose those fields. Add round-trip coverage preserving id, version, and unknown metadata, including nested fields.
💡 Motivation and Context
Our tech stack is ruby/go and we would like to use https://posthog.com/docs/feature-flags/local-evaluation/distributed-environments
This PR adds support of this feature for go sdk.
💚 How did you test it?
Run tests full suite, please i used example to test out that its working as expected, fetch flags definitions and re-use it and next fetch if needed
📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)