Local Feature Flag Evaluation #6
Conversation
EDsCODE
commented
Jul 28, 2022
•
EDsCODE
commented
- GetAllFlags
- Adjust IsFeatureEnabled and GetFeatureFlag to share logic
- Local Evaluation supported
- api/feature_flag updated to local_evaluation endpoint
- New suite of tests according to (Feature Flag Spec for integration libraries posthog#10459 (comment))
|
|
||
| var i float64 | ||
| switch t := val.(type) { | ||
| case int: |
There was a problem hiding this comment.
just wondering is there any reason we can't consolidate these case statements since we're doing float64(t) to all of them?
There was a problem hiding this comment.
Not sure if this is different in go, but if different cases require the same code, you can put them together like so:
case int:
case int8:
case int16:
i = float64(t)
return i, nil
|
TODO:
|
There was a problem hiding this comment.
RE: error handling levels. I'm not a go expert but given how it's standard to emit errors back to the top to be handled, I've added a few error logs where it's implicitly handled but otherwise I send the error up the chain so that the featureflag functions will return them
feature_flags_test.go
Outdated
|
|
||
| } | ||
|
|
||
| func TestCaptureMultipleUsersDoesntOutOfMemory(t *testing.T) { |
There was a problem hiding this comment.
I'm unsure how to test this
There was a problem hiding this comment.
Not aware of the intricacies here, but the test is testing that the size of the hash doesn't blow up arbitrarily with new users coming in. (As for long running server-side apps, there's a risk of this running out of memory).
In node, I just check that the distinctIdHasSentFeatureFlagCalls array never exceeds MAX_SIZE, when 100*MAX_SIZE new users are seen:
There was a problem hiding this comment.
I'm testing the sizelimitedmap separately as a unit test. Doesn't seem feasible to test as an integration test since there's no way to expose the map without making it part of the public interface on client which isn't good practice as it's unnecessary to the user
| @@ -245,21 +259,28 @@ func (c *client) ReloadFeatureFlags() error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (c *client) GetFeatureFlag(flagKey string, distinctId string, defaultValue interface{}) (interface{}, error) { | |||
| func (c *client) GetFeatureFlag(flagConfig FeatureFlagPayload) (interface{}, error) { | |||
| if err := flagConfig.validate(); err != nil { | |||
There was a problem hiding this comment.
Major change is that we accept a featureflag payload and validate it on invocation. This way users don't need to fill in every argument everytime
There was a problem hiding this comment.
I'm fine either way, but in node the way I structured this was to have required parameters outside the payload, and everything else inside. So, it looks something like:
// getFeatureFlag(
// key: string,
// distinctId: string,
// options?: {
// groups?: Record<string, string>
// personProperties?: Record<string, string>
// groupProperties?: Record<string, Record<string, string>>
// onlyEvaluateLocally?: boolean
// sendFeatureFlagEvents?: boolean
// }
// ): Promise<string | boolean | undefined>
It's nice because people who don't care about local eval / overriding server properties don't have to worry about payload keys.
Upto you though, your library, your call 😂
Co-authored-by: Neil Kakkar <neilkakkar@gmail.com>
| i = float64(t) | ||
| case uint64: | ||
| i = float64(t) | ||
| default: |
There was a problem hiding this comment.
just checking, not sure about go syntax: This doesn't always go to default right? (it would in other langs, because there's no break/return in the cases above)
posthog.go
Outdated
| @@ -37,17 +38,20 @@ type Client interface { | |||
| Enqueue(Message) error | |||
| // | |||
| // Method returns if a feature flag is on for a given user based on their distinct ID | |||
| IsFeatureEnabled(string, string, bool) (bool, error) | |||
| IsFeatureEnabled(FeatureFlagPayload) (bool, error) | |||
There was a problem hiding this comment.
remember to mention this breaking change in the changelog :)
