From d15691cf270a2c9c0c44a60bf82a410fe21b5c28 Mon Sep 17 00:00:00 2001 From: Roman Zaynetdinov Date: Thu, 8 Feb 2024 12:05:51 +0100 Subject: [PATCH 1/6] Initialize properties map when sending feature flags (#26) --- posthog.go | 5 +++++ posthog_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/posthog.go b/posthog.go index 4c2aa0d..79ca2f7 100644 --- a/posthog.go +++ b/posthog.go @@ -189,6 +189,11 @@ func (c *client) Enqueue(msg Message) (err error) { if err != nil { c.Errorf("unable to get feature variants - %s", err) } + + if m.Properties == nil { + m.Properties = NewProperties() + } + for feature, variant := range featureVariants { propKey := fmt.Sprintf("$feature/%s", feature) m.Properties[propKey] = variant diff --git a/posthog_test.go b/posthog_test.go index 3b7c77a..f0bf009 100644 --- a/posthog_test.go +++ b/posthog_test.go @@ -929,3 +929,41 @@ func TestDisabledFlag(t *testing.T) { t.Errorf("flag listed in /decide/ response should have value 'false'") } } + +func TestCaptureSendFlags(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(fixture("test-api-feature-flag.json"))) + })) + defer server.Close() + + client, _ := NewWithConfig("Csyjlnlun3OzyNJAafdlv", Config{ + Endpoint: server.URL, + Verbose: true, + Logger: t, + BatchSize: 1, + now: mockTime, + uid: mockId, + + PersonalApiKey: "some very secret key", + }) + defer client.Close() + + // Without this call client.Close hangs forever + // Ref: https://github.com/PostHog/posthog-go/issues/28 + client.IsFeatureEnabled( + FeatureFlagPayload{ + Key: "simpleFlag", + DistinctId: "hey", + }, + ) + + err := client.Enqueue(Capture{ + Event: "Download", + DistinctId: "123456", + SendFeatureFlags: true, + }) + + if err != nil { + t.Fatal(err) + } +} From abe6962c309e2ad804f013fc0f40c29e56f39a3f Mon Sep 17 00:00:00 2001 From: Roman Zaynetdinov Date: Thu, 8 Feb 2024 12:15:51 +0100 Subject: [PATCH 2/6] Log flag name when failed to compute it locally (#32) --- featureflags.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/featureflags.go b/featureflags.go index 012d1de..732a9ba 100644 --- a/featureflags.go +++ b/featureflags.go @@ -201,7 +201,7 @@ func (poller *FeatureFlagsPoller) GetFeatureFlag(flagConfig FeatureFlagPayload) } if err != nil { - poller.Errorf("Unable to compute flag locally - %s", err) + poller.Errorf("Unable to compute flag locally (%s) - %s", featureFlag.Key, err) } if (err != nil || result == nil) && !flagConfig.OnlyEvaluateLocally { @@ -226,7 +226,7 @@ func (poller *FeatureFlagsPoller) GetAllFlags(flagConfig FeatureFlagPayloadNoKey for _, storedFlag := range featureFlags { result, err := poller.computeFlagLocally(storedFlag, flagConfig.DistinctId, flagConfig.Groups, flagConfig.PersonProperties, flagConfig.GroupProperties) if err != nil { - poller.Errorf("Unable to compute flag locally - %s", err) + poller.Errorf("Unable to compute flag locally (%s) - %s", storedFlag.Key, err) fallbackToDecide = true } else { response[storedFlag.Key] = result From 4944045455b4fe4d69b1382475f2d602db4ebb9a Mon Sep 17 00:00:00 2001 From: Roman Zaynetdinov Date: Wed, 21 Feb 2024 15:58:34 +0200 Subject: [PATCH 3/6] Support multiple cohorts in local evaluation (#34) --- .github/workflows/unit-tests.yml | 4 +- feature_flags_test.go | 110 ++++++++- featureflags.go | 208 ++++++++++++++++-- .../test-complex-cohorts-locally.json | 67 ++++++ ...test-complex-cohorts-negation-locally.json | 68 ++++++ go.mod | 8 +- go.sum | 1 + 7 files changed, 430 insertions(+), 36 deletions(-) create mode 100644 fixtures/feature_flag/test-complex-cohorts-locally.json create mode 100644 fixtures/feature_flag/test-complex-cohorts-negation-locally.json diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index d0b14c0..1bddd8a 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -8,10 +8,10 @@ jobs: name: Build runs-on: ubuntu-latest steps: - - name: Set up Go 1.15 + - name: Set up Go 1.18 uses: actions/setup-go@v1 with: - go-version: 1.15 + go-version: 1.18 - name: Check out source code uses: actions/checkout@v1 diff --git a/feature_flags_test.go b/feature_flags_test.go index 790553b..184de5d 100644 --- a/feature_flags_test.go +++ b/feature_flags_test.go @@ -12,7 +12,7 @@ import ( ) func TestMatchPropertyValue(t *testing.T) { - property := Property{ + property := FlagProperty{ Key: "Browser", Value: "Chrome", Operator: "exact", @@ -28,9 +28,8 @@ func TestMatchPropertyValue(t *testing.T) { } - func TestMatchPropertyInvalidOperator(t *testing.T) { - property := Property{ + property := FlagProperty{ Key: "Browser", Value: "Chrome", Operator: "is_unknown", @@ -51,7 +50,7 @@ func TestMatchPropertyInvalidOperator(t *testing.T) { } func TestMatchPropertySlice(t *testing.T) { - property := Property{ + property := FlagProperty{ Key: "Browser", Value: []interface{}{"Chrome"}, Operator: "exact", @@ -67,7 +66,7 @@ func TestMatchPropertySlice(t *testing.T) { } func TestMatchPropertyNumber(t *testing.T) { - property := Property{ + property := FlagProperty{ Key: "Number", Value: 5, Operator: "gt", @@ -85,7 +84,7 @@ func TestMatchPropertyNumber(t *testing.T) { t.Error("Value is not a match") } - property = Property{ + property = FlagProperty{ Key: "Number", Value: 5, Operator: "lt", @@ -103,7 +102,7 @@ func TestMatchPropertyNumber(t *testing.T) { t.Error("Value is not a match") } - property = Property{ + property = FlagProperty{ Key: "Number", Value: 5, Operator: "gte", @@ -121,7 +120,7 @@ func TestMatchPropertyNumber(t *testing.T) { t.Error("Value is not a match") } - property = Property{ + property = FlagProperty{ Key: "Number", Value: 5, Operator: "lte", @@ -144,7 +143,7 @@ func TestMatchPropertyRegex(t *testing.T) { shouldMatch := []interface{}{"value.com", "value2.com"} - property := Property{ + property := FlagProperty{ Key: "key", Value: "\\.com$", Operator: "regex", @@ -175,7 +174,7 @@ func TestMatchPropertyRegex(t *testing.T) { } // invalid regex - property = Property{ + property = FlagProperty{ Key: "key", Value: "?*", Operator: "regex", @@ -195,7 +194,7 @@ func TestMatchPropertyRegex(t *testing.T) { // non string value - property = Property{ + property = FlagProperty{ Key: "key", Value: 4, Operator: "regex", @@ -217,7 +216,7 @@ func TestMatchPropertyRegex(t *testing.T) { func TestMatchPropertyContains(t *testing.T) { shouldMatch := []interface{}{"value", "value2", "value3", "value4", "343tfvalue5"} - property := Property{ + property := FlagProperty{ Key: "key", Value: "valUe", Operator: "icontains", @@ -3146,3 +3145,90 @@ func TestMultivariateFlagConsistency(t *testing.T) { } } } + +func TestComplexCohortsLocally(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(fixture("feature_flag/test-complex-cohorts-locally.json"))) // Don't return anything for local eval + })) + defer server.Close() + + client, _ := NewWithConfig("Csyjlnlun3OzyNJAafdlv", Config{ + PersonalApiKey: "some very secret key", + Endpoint: server.URL, + }) + defer client.Close() + + payload := FeatureFlagPayload{ + Key: "beta-feature", + DistinctId: "some-distinct-id", + PersonProperties: NewProperties().Set("region", "UK"), + } + + isMatch, err := client.IsFeatureEnabled(payload) + if err != nil { + t.Fatal(err) + } + if isMatch != false { + t.Error("Should not match") + } + + payload.PersonProperties = NewProperties().Set("region", "USA").Set("other", "thing") + isMatch, _ = client.IsFeatureEnabled(payload) + if isMatch != true { + t.Error("Should match") + } + + // even though 'other' property is not present, the cohort should still match since it's an OR condition + payload.PersonProperties = NewProperties().Set("region", "USA").Set("nation", "UK") + isMatch, _ = client.IsFeatureEnabled(payload) + if isMatch != true { + t.Error("Should match") + } +} + +func TestComplexCohortsWithNegationLocally(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(fixture("feature_flag/test-complex-cohorts-negation-locally.json"))) // Don't return anything for local eval + })) + defer server.Close() + + client, _ := NewWithConfig("Csyjlnlun3OzyNJAafdlv", Config{ + PersonalApiKey: "some very secret key", + Endpoint: server.URL, + }) + defer client.Close() + + payload := FeatureFlagPayload{ + Key: "beta-feature", + DistinctId: "some-distinct-id", + PersonProperties: NewProperties().Set("region", "UK"), + } + + isMatch, err := client.IsFeatureEnabled(payload) + if err != nil { + t.Fatal(err) + } + if isMatch != false { + t.Error("Should not match") + } + + // even though 'other' property is not present, the cohort should still match since it's an OR condition + payload.PersonProperties = NewProperties().Set("region", "USA").Set("nation", "UK") + isMatch, _ = client.IsFeatureEnabled(payload) + if isMatch != true { + t.Error("Should match") + } + + // # since 'other' is negated, we return False. Since 'nation' is not present, we can't tell whether the flag should be true or false, so go to decide + payload.PersonProperties = NewProperties().Set("region", "USA").Set("other", "thing") + _, err = client.IsFeatureEnabled(payload) + if err != nil { + t.Error("Expected to fail") + } + + payload.PersonProperties = NewProperties().Set("region", "USA").Set("other", "thing2") + isMatch, _ = client.IsFeatureEnabled(payload) + if isMatch != true { + t.Error("Should match") + } +} diff --git a/featureflags.go b/featureflags.go index 732a9ba..9e301da 100644 --- a/featureflags.go +++ b/featureflags.go @@ -25,6 +25,7 @@ type FeatureFlagsPoller struct { shutdown chan bool forceReload chan bool featureFlags []FeatureFlag + cohorts map[string]PropertyGroup groups map[string]string personalApiKey string projectApiKey string @@ -45,9 +46,9 @@ type FeatureFlag struct { } type Filter struct { - AggregationGroupTypeIndex *uint8 `json:"aggregation_group_type_index"` - Groups []PropertyGroup `json:"groups"` - Multivariate *Variants `json:"multivariate"` + AggregationGroupTypeIndex *uint8 `json:"aggregation_group_type_index"` + Groups []FeatureFlagCondition `json:"groups"` + Multivariate *Variants `json:"multivariate"` } type Variants struct { @@ -59,17 +60,25 @@ type FlagVariant struct { Name string `json:"name"` RolloutPercentage *uint8 `json:"rollout_percentage"` } -type PropertyGroup struct { - Properties []Property `json:"properties"` - RolloutPercentage *uint8 `json:"rollout_percentage"` - Variant *string `json:"variant"` + +type FeatureFlagCondition struct { + Properties []FlagProperty `json:"properties"` + RolloutPercentage *uint8 `json:"rollout_percentage"` + Variant *string `json:"variant"` } -type Property struct { +type FlagProperty struct { Key string `json:"key"` Operator string `json:"operator"` Value interface{} `json:"value"` Type string `json:"type"` + Negation bool `json:"negation"` +} + +type PropertyGroup struct { + Type string `json:"type"` + // []PropertyGroup or []FlagProperty + Values []any `json:"values"` } type FlagVariantMeta struct { @@ -79,8 +88,9 @@ type FlagVariantMeta struct { } type FeatureFlagsResponse struct { - Flags []FeatureFlag `json:"flags"` - GroupTypeMapping *map[string]string `json:"group_type_mapping"` + Flags []FeatureFlag `json:"flags"` + GroupTypeMapping *map[string]string `json:"group_type_mapping"` + Cohorts map[string]PropertyGroup `json:"cohorts"` } type DecideRequestData struct { @@ -173,6 +183,7 @@ func (poller *FeatureFlagsPoller) fetchNewFeatureFlags() { } poller.mutex.Lock() poller.featureFlags = newFlags + poller.cohorts = featureFlagsResponse.Cohorts if featureFlagsResponse.GroupTypeMapping != nil { poller.groups = *featureFlagsResponse.GroupTypeMapping } @@ -182,6 +193,7 @@ func (poller *FeatureFlagsPoller) fetchNewFeatureFlags() { func (poller *FeatureFlagsPoller) GetFeatureFlag(flagConfig FeatureFlagPayload) (interface{}, error) { featureFlags := poller.GetFeatureFlags() + cohorts := poller.cohorts featureFlag := FeatureFlag{Key: ""} @@ -197,7 +209,14 @@ func (poller *FeatureFlagsPoller) GetFeatureFlag(flagConfig FeatureFlagPayload) var err error if featureFlag.Key != "" { - result, err = poller.computeFlagLocally(featureFlag, flagConfig.DistinctId, flagConfig.Groups, flagConfig.PersonProperties, flagConfig.GroupProperties) + result, err = poller.computeFlagLocally( + featureFlag, + flagConfig.DistinctId, + flagConfig.Groups, + flagConfig.PersonProperties, + flagConfig.GroupProperties, + cohorts, + ) } if err != nil { @@ -219,12 +238,20 @@ func (poller *FeatureFlagsPoller) GetAllFlags(flagConfig FeatureFlagPayloadNoKey response := map[string]interface{}{} featureFlags := poller.GetFeatureFlags() fallbackToDecide := false + cohorts := poller.cohorts if len(featureFlags) == 0 { fallbackToDecide = true } else { for _, storedFlag := range featureFlags { - result, err := poller.computeFlagLocally(storedFlag, flagConfig.DistinctId, flagConfig.Groups, flagConfig.PersonProperties, flagConfig.GroupProperties) + result, err := poller.computeFlagLocally( + storedFlag, + flagConfig.DistinctId, + flagConfig.Groups, + flagConfig.PersonProperties, + flagConfig.GroupProperties, + cohorts, + ) if err != nil { poller.Errorf("Unable to compute flag locally (%s) - %s", storedFlag.Key, err) fallbackToDecide = true @@ -249,7 +276,14 @@ func (poller *FeatureFlagsPoller) GetAllFlags(flagConfig FeatureFlagPayloadNoKey return response, nil } -func (poller *FeatureFlagsPoller) computeFlagLocally(flag FeatureFlag, distinctId string, groups Groups, personProperties Properties, groupProperties map[string]Properties) (interface{}, error) { +func (poller *FeatureFlagsPoller) computeFlagLocally( + flag FeatureFlag, + distinctId string, + groups Groups, + personProperties Properties, + groupProperties map[string]Properties, + cohorts map[string]PropertyGroup, +) (interface{}, error) { if flag.EnsureExperienceContinuity != nil && *flag.EnsureExperienceContinuity { return nil, &InconclusiveMatchError{"Flag has experience continuity enabled"} } @@ -275,9 +309,9 @@ func (poller *FeatureFlagsPoller) computeFlagLocally(flag FeatureFlag, distinctI } focusedGroupProperties := groupProperties[groupName] - return matchFeatureFlagProperties(flag, groups[groupName].(string), focusedGroupProperties) + return matchFeatureFlagProperties(flag, groups[groupName].(string), focusedGroupProperties, cohorts) } else { - return matchFeatureFlagProperties(flag, distinctId, personProperties) + return matchFeatureFlagProperties(flag, distinctId, personProperties, cohorts) } } @@ -318,14 +352,19 @@ func getVariantLookupTable(flag FeatureFlag) []FlagVariantMeta { return lookupTable } -func matchFeatureFlagProperties(flag FeatureFlag, distinctId string, properties Properties) (interface{}, error) { +func matchFeatureFlagProperties( + flag FeatureFlag, + distinctId string, + properties Properties, + cohorts map[string]PropertyGroup, +) (interface{}, error) { conditions := flag.Filters.Groups isInconclusive := false // # Stable sort conditions with variant overrides to the top. This ensures that if overrides are present, they are // # evaluated first, and the variant override is applied to the first matching condition. // conditionsCopy := make([]PropertyGroup, len(conditions)) - sortedConditions := append([]PropertyGroup{}, conditions...) + sortedConditions := append([]FeatureFlagCondition{}, conditions...) sort.SliceStable(sortedConditions, func(i, j int) bool { iValue := 1 @@ -343,7 +382,7 @@ func matchFeatureFlagProperties(flag FeatureFlag, distinctId string, properties for _, condition := range sortedConditions { - isMatch, err := isConditionMatch(flag, distinctId, condition, properties) + isMatch, err := isConditionMatch(flag, distinctId, condition, properties, cohorts) if err != nil { if _, ok := err.(*InconclusiveMatchError); ok { isInconclusive = true @@ -371,11 +410,24 @@ func matchFeatureFlagProperties(flag FeatureFlag, distinctId string, properties return false, nil } -func isConditionMatch(flag FeatureFlag, distinctId string, condition PropertyGroup, properties Properties) (bool, error) { +func isConditionMatch( + flag FeatureFlag, + distinctId string, + condition FeatureFlagCondition, + properties Properties, + cohorts map[string]PropertyGroup, +) (bool, error) { if len(condition.Properties) > 0 { for _, prop := range condition.Properties { + var isMatch bool + var err error + + if prop.Type == "cohort" { + isMatch, err = matchCohort(prop, properties, cohorts) + } else { + isMatch, err = matchProperty(prop, properties) + } - isMatch, err := matchProperty(prop, properties) if err != nil { return false, err } @@ -397,7 +449,110 @@ func isConditionMatch(flag FeatureFlag, distinctId string, condition PropertyGro return true, nil } -func matchProperty(property Property, properties Properties) (bool, error) { +func matchCohort(property FlagProperty, properties Properties, cohorts map[string]PropertyGroup) (bool, error) { + cohortId := fmt.Sprint(property.Value) + propertyGroup, ok := cohorts[cohortId] + if !ok { + return false, fmt.Errorf("Can't match cohort: cohort %s not found", cohortId) + } + + return matchPropertyGroup(propertyGroup, properties, cohorts) +} + +func matchPropertyGroup(propertyGroup PropertyGroup, properties Properties, cohorts map[string]PropertyGroup) (bool, error) { + groupType := propertyGroup.Type + values := propertyGroup.Values + + if len(values) == 0 { + // empty groups are no-ops, always match + return true, nil + } + + errorMatchingLocally := false + + for _, value := range values { + switch prop := value.(type) { + case map[string]any: + if _, ok := prop["values"]; ok { + // PropertyGroup + matches, err := matchPropertyGroup(PropertyGroup{ + Type: getSafeProp[string](prop, "type"), + Values: getSafeProp[[]any](prop, "values"), + }, properties, cohorts) + if err != nil { + if _, ok := err.(*InconclusiveMatchError); ok { + errorMatchingLocally = true + } else { + return false, err + } + } + + if groupType == "AND" { + if !matches { + return false, nil + } + } else { + // OR group + if matches { + return true, nil + } + } + } else { + // FlagProperty + var matches bool + var err error + flagProperty := FlagProperty{ + Key: getSafeProp[string](prop, "key"), + Operator: getSafeProp[string](prop, "operator"), + Value: getSafeProp[any](prop, "value"), + Type: getSafeProp[string](prop, "type"), + Negation: getSafeProp[bool](prop, "negation"), + } + if prop["type"] == "cohort" { + matches, err = matchCohort(flagProperty, properties, cohorts) + } else { + matches, err = matchProperty(flagProperty, properties) + } + + if err != nil { + if _, ok := err.(*InconclusiveMatchError); ok { + errorMatchingLocally = true + } else { + return false, err + } + } + + negation := flagProperty.Negation + if groupType == "AND" { + // if negated property, do the inverse + if !matches && !negation { + return false, nil + } + if matches && negation { + return false, nil + } + } else { + // OR group + if matches && !negation { + return true, nil + } + if !matches && negation { + return true, nil + } + } + } + } + } + + if errorMatchingLocally { + return false, &InconclusiveMatchError{msg: "Can't match cohort without a given cohort property value"} + } + + // if we get here, all matched in AND case, or none matched in OR case + return groupType == "AND", nil +} + +func matchProperty(property FlagProperty, properties Properties) (bool, error) { key := property.Key operator := property.Operator value := property.Value @@ -665,6 +820,7 @@ func (poller *FeatureFlagsPoller) localEvaluationFlags(headers [][2]string) (*ht } searchParams := url.Query() searchParams.Add("token", poller.projectApiKey) + searchParams.Add("send_cohorts", "true") url.RawQuery = searchParams.Encode() return poller.request("GET", url, []byte{}, headers) @@ -778,3 +934,13 @@ func (poller *FeatureFlagsPoller) getFeatureFlagVariant(featureFlag FeatureFlag, } return result, nil } + +func getSafeProp[T any](properties map[string]any, key string) T { + switch v := properties[key].(type) { + case T: + return v + default: + var defaultValue T + return defaultValue + } +} diff --git a/fixtures/feature_flag/test-complex-cohorts-locally.json b/fixtures/feature_flag/test-complex-cohorts-locally.json new file mode 100644 index 0000000..ec7b846 --- /dev/null +++ b/fixtures/feature_flag/test-complex-cohorts-locally.json @@ -0,0 +1,67 @@ +{ + "flags": [ + { + "id": 1, + "name": "Beta Feature", + "key": "beta-feature", + "is_simple_flag": false, + "active": true, + "rollout_percentage": 100, + "filters": { + "groups": [ + { + "properties": [ + { + "key": "region", + "operator": "exact", + "value": [ + "USA" + ], + "type": "person" + }, + { + "key": "id", + "value": 98, + "type": "cohort" + } + ], + "rollout_percentage": 100 + } + ] + } + } + ], + "cohorts": { + "1": { + "type": "AND", + "values": [ + { + "key": "other", + "operator": "exact", + "value": [ + "thing" + ], + "type": "person" + } + ] + }, + "98": { + "type": "OR", + "values": [ + { + "key": "id", + "value": 1, + "type": "cohort" + }, + { + "key": "nation", + "operator": "exact", + "value": [ + "UK" + ], + "type": "person" + } + ] + } + } +} diff --git a/fixtures/feature_flag/test-complex-cohorts-negation-locally.json b/fixtures/feature_flag/test-complex-cohorts-negation-locally.json new file mode 100644 index 0000000..72b4a57 --- /dev/null +++ b/fixtures/feature_flag/test-complex-cohorts-negation-locally.json @@ -0,0 +1,68 @@ +{ + "flags": [ + { + "id": 1, + "name": "Beta Feature", + "key": "beta-feature", + "is_simple_flag": false, + "active": true, + "rollout_percentage": 100, + "filters": { + "groups": [ + { + "properties": [ + { + "key": "region", + "operator": "exact", + "value": [ + "USA" + ], + "type": "person" + }, + { + "key": "id", + "value": 98, + "type": "cohort" + } + ], + "rollout_percentage": 100 + } + ] + } + } + ], + "cohorts": { + "1": { + "type": "AND", + "values": [ + { + "key": "other", + "operator": "exact", + "value": [ + "thing" + ], + "type": "person", + "negation": true + } + ] + }, + "98": { + "type": "OR", + "values": [ + { + "key": "id", + "value": 1, + "type": "cohort" + }, + { + "key": "nation", + "operator": "exact", + "value": [ + "UK" + ], + "type": "person" + } + ] + } + } +} diff --git a/go.mod b/go.mod index 80a0fcd..036d7d3 100644 --- a/go.mod +++ b/go.mod @@ -1,8 +1,14 @@ module github.com/posthog/posthog-go -go 1.15 +go 1.18 require ( github.com/google/uuid v1.3.0 github.com/urfave/cli v1.22.5 ) + +require ( + github.com/cpuguy83/go-md2man/v2 v2.0.0-20190314233015-f79a8a8ca69d // indirect + github.com/russross/blackfriday/v2 v2.0.1 // indirect + github.com/shurcooL/sanitized_anchor_name v1.0.0 // indirect +) diff --git a/go.sum b/go.sum index ecba18a..15093c4 100644 --- a/go.sum +++ b/go.sum @@ -3,6 +3,7 @@ github.com/cpuguy83/go-md2man/v2 v2.0.0-20190314233015-f79a8a8ca69d h1:U+s90UTSY github.com/cpuguy83/go-md2man/v2 v2.0.0-20190314233015-f79a8a8ca69d/go.mod h1:maD7wRr/U5Z6m/iR4s+kqSMx2CaBsrgA7czyZG/E6dU= github.com/google/uuid v1.3.0 h1:t6JiXgmwXMjEs8VusXIJk2BXHsn+wx8BZdTaoZ5fu7I= github.com/google/uuid v1.3.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/russross/blackfriday/v2 v2.0.1 h1:lPqVAte+HuHNfhJ/0LC98ESWRz8afy9tM/0RK8m9o+Q= github.com/russross/blackfriday/v2 v2.0.1/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= From f35c554222b38a3b12e9a697b4905c308949c618 Mon Sep 17 00:00:00 2001 From: Roman Zaynetdinov Date: Thu, 14 Mar 2024 17:57:07 +0200 Subject: [PATCH 4/6] Allow to configure when next flags polling happens (#36) We want to poll at the same time from all instances. --- config.go | 4 ++++ featureflags.go | 25 ++++++++++++++++++++----- posthog.go | 10 +++++++++- 3 files changed, 33 insertions(+), 6 deletions(-) diff --git a/config.go b/config.go index 280f81a..dd0740c 100644 --- a/config.go +++ b/config.go @@ -30,6 +30,10 @@ type Config struct { // Interval at which to fetch new feature flags, 5min by default DefaultFeatureFlagsPollingInterval time.Duration + // Calculate when feature flags should be polled next. Setting this property + // will override DefaultFeatureFlagsPollingInterval. + NextFeatureFlagsPollingTick func() time.Duration + // The HTTP transport used by the client, this allows an application to // redefine how requests are being sent at the HTTP level (for example, // to change the connection pooling policy). diff --git a/featureflags.go b/featureflags.go index 9e301da..cc31f58 100644 --- a/featureflags.go +++ b/featureflags.go @@ -20,7 +20,6 @@ import ( const LONG_SCALE = 0xfffffffffffffff type FeatureFlagsPoller struct { - ticker *time.Ticker // periodic ticker loaded chan bool shutdown chan bool forceReload chan bool @@ -34,6 +33,7 @@ type FeatureFlagsPoller struct { http http.Client mutex sync.RWMutex fetchedFlagsSuccessfullyOnce bool + nextPollTick func() time.Duration } type FeatureFlag struct { @@ -113,9 +113,21 @@ func (e *InconclusiveMatchError) Error() string { return e.msg } -func newFeatureFlagsPoller(projectApiKey string, personalApiKey string, errorf func(format string, args ...interface{}), endpoint string, httpClient http.Client, pollingInterval time.Duration) *FeatureFlagsPoller { +func newFeatureFlagsPoller( + projectApiKey string, + personalApiKey string, + errorf func(format string, args ...interface{}), + endpoint string, + httpClient http.Client, + pollingInterval time.Duration, + nextPollTick func() time.Duration, +) *FeatureFlagsPoller { + + if nextPollTick == nil { + nextPollTick = func() time.Duration { return pollingInterval } + } + poller := FeatureFlagsPoller{ - ticker: time.NewTicker(pollingInterval), loaded: make(chan bool), shutdown: make(chan bool), forceReload: make(chan bool), @@ -126,6 +138,7 @@ func newFeatureFlagsPoller(projectApiKey string, personalApiKey string, errorf f http: httpClient, mutex: sync.RWMutex{}, fetchedFlagsSuccessfullyOnce: false, + nextPollTick: nextPollTick, } go poller.run() @@ -136,16 +149,18 @@ func (poller *FeatureFlagsPoller) run() { poller.fetchNewFeatureFlags() for { + timer := time.NewTimer(poller.nextPollTick()) select { case <-poller.shutdown: close(poller.shutdown) close(poller.forceReload) close(poller.loaded) - poller.ticker.Stop() + timer.Stop() return case <-poller.forceReload: + timer.Stop() poller.fetchNewFeatureFlags() - case <-poller.ticker.C: + case <-timer.C: poller.fetchNewFeatureFlags() } } diff --git a/posthog.go b/posthog.go index 79ca2f7..f38aed7 100644 --- a/posthog.go +++ b/posthog.go @@ -111,7 +111,15 @@ func NewWithConfig(apiKey string, config Config) (cli Client, err error) { } if len(c.PersonalApiKey) > 0 { - c.featureFlagsPoller = newFeatureFlagsPoller(c.key, c.Config.PersonalApiKey, c.Errorf, c.Endpoint, c.http, c.DefaultFeatureFlagsPollingInterval) + c.featureFlagsPoller = newFeatureFlagsPoller( + c.key, + c.Config.PersonalApiKey, + c.Errorf, + c.Endpoint, + c.http, + c.DefaultFeatureFlagsPollingInterval, + c.NextFeatureFlagsPollingTick, + ) } go c.loop() From 036dfa9f3555896033d24afad26585c2f0e2b2a4 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Fri, 15 Mar 2024 13:09:56 +0000 Subject: [PATCH 5/6] fix(flags): Add configurable flag timeouts (#37) --- .github/workflows/unit-tests.yml | 2 +- config.go | 14 +- examples/capture.go | 3 +- examples/featureflags.go | 23 ++- feature_flags_test.go | 156 +++++++++++++++++- featureflags.go | 41 +++-- .../test-flag-group-properties.json | 26 ++- posthog.go | 1 + 8 files changed, 234 insertions(+), 32 deletions(-) diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index 1bddd8a..6a2f55b 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -20,4 +20,4 @@ jobs: run: go build . - name: Test - run: go test -v . \ No newline at end of file + run: go test -timeout 30s -v . \ No newline at end of file diff --git a/config.go b/config.go index dd0740c..4624d69 100644 --- a/config.go +++ b/config.go @@ -27,10 +27,13 @@ type Config struct { // timer triggers. Interval time.Duration - // Interval at which to fetch new feature flags, 5min by default + // Interval at which to fetch new feature flag definitions, 5min by default DefaultFeatureFlagsPollingInterval time.Duration - // Calculate when feature flags should be polled next. Setting this property + // Timeout for fetching feature flags, 3 seconds by default + FeatureFlagRequestTimeout time.Duration + + // Calculate when feature flag definitions should be polled next. Setting this property // will override DefaultFeatureFlagsPollingInterval. NextFeatureFlagsPollingTick func() time.Duration @@ -98,6 +101,9 @@ const DefaultInterval = 5 * time.Second // Specifies the default interval at which to fetch new feature flags const DefaultFeatureFlagsPollingInterval = 5 * time.Minute +// Specifies the default timeout for fetching feature flags +const DefaultFeatureFlagRequestTimeout = 3 * time.Second + // This constant sets the default batch size used by client instances if none // was explicitly set. const DefaultBatchSize = 250 @@ -139,6 +145,10 @@ func makeConfig(c Config) Config { c.DefaultFeatureFlagsPollingInterval = DefaultFeatureFlagsPollingInterval } + if c.FeatureFlagRequestTimeout == 0 { + c.FeatureFlagRequestTimeout = DefaultFeatureFlagRequestTimeout + } + if c.Transport == nil { c.Transport = http.DefaultTransport } diff --git a/examples/capture.go b/examples/capture.go index d5bc303..9b46913 100644 --- a/examples/capture.go +++ b/examples/capture.go @@ -2,8 +2,9 @@ package main import ( "fmt" - "github.com/posthog/posthog-go" "time" + + "github.com/posthog/posthog-go" ) func TestCapture() { diff --git a/examples/featureflags.go b/examples/featureflags.go index 355319f..72a9705 100644 --- a/examples/featureflags.go +++ b/examples/featureflags.go @@ -8,11 +8,14 @@ import ( ) func TestIsFeatureEnabled() { - client, _ := posthog.NewWithConfig("phc_X8B6bhR1QgQKP1WdpFLN82LxLxgZ7WPXDgJyRyvIpib", posthog.Config{ - Interval: 30 * time.Second, - BatchSize: 100, - Verbose: true, - PersonalApiKey: "phx_vXZ7AOnFjDrCxfWLyo9V6P0SWLLfXT2d5euy3U0nRGk", + client, _ := posthog.NewWithConfig("phc_36WfBWNJEQcYotMZ7Ui7EWzqKLbIo2LWJFG5fIg1EER", posthog.Config{ + Interval: 30 * time.Second, + BatchSize: 100, + Verbose: true, + PersonalApiKey: "phx_n79cT52OfsxAWDhZs9j3w67aRoBCZ7l5ksRRKmAi5nr", + Endpoint: "http://localhost:8000", + DefaultFeatureFlagsPollingInterval: 5 * time.Second, + FeatureFlagRequestTimeout: 3 * time.Second, }) defer client.Close() @@ -22,9 +25,10 @@ func TestIsFeatureEnabled() { DistinctId: "hello", }) + fmt.Println("boolResult:", boolResult) + if boolErr != nil || boolResult == nil { fmt.Println("error:", boolErr) - return } // Simple flag @@ -32,9 +36,10 @@ func TestIsFeatureEnabled() { Key: "simple-test", DistinctId: "hello", }) + + fmt.Println("simpleResult:", simpleResult) if simpleErr != nil || simpleResult == false { fmt.Println("error:", simpleErr) - return } // Multivariate flag @@ -42,9 +47,9 @@ func TestIsFeatureEnabled() { Key: "multivariate-test", DistinctId: "hello", }) + fmt.Println("variantResult:", variantResult) if variantErr != nil || variantResult != "variant-value" { fmt.Println("error:", variantErr) - return } // Multivariate + simple flag @@ -52,8 +57,8 @@ func TestIsFeatureEnabled() { Key: "multivariate-simple-test", DistinctId: "hello", }) + fmt.Println("variantResult:", variantResult) if variantErr != nil || variantResult == true { fmt.Println("error:", variantErr) - return } } diff --git a/feature_flags_test.go b/feature_flags_test.go index 184de5d..c2aab19 100644 --- a/feature_flags_test.go +++ b/feature_flags_test.go @@ -1,12 +1,15 @@ package posthog import ( + "bytes" "encoding/json" "fmt" + "log" "net/http" "net/http/httptest" "reflect" "strings" + "time" "testing" ) @@ -613,9 +616,13 @@ func TestFeatureFlagNullComeIntoPlayOnlyWhenDecideErrorsOut(t *testing.T) { defer server.Close() + // TODO: Make this nicer, right now if all local evaluation requests fail, we block + // on waiting for atleast one request to happen before returning flags, + // which can be suboptimal client, _ := NewWithConfig("Csyjlnlun3OzyNJAafdlv", Config{ - PersonalApiKey: "some very secret key", - Endpoint: server.URL, + PersonalApiKey: "some very secret key", + Endpoint: server.URL, + DefaultFeatureFlagsPollingInterval: 5 * time.Second, }) defer client.Close() @@ -3232,3 +3239,148 @@ func TestComplexCohortsWithNegationLocally(t *testing.T) { t.Error("Should match") } } + +func TestFlagWithTimeoutExceeded(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.HasPrefix(r.URL.Path, "/decide") { + time.Sleep(1 * time.Second) + w.Write([]byte(fixture("test-decide-v2.json"))) + } else if strings.HasPrefix(r.URL.Path, "/api/feature_flag/local_evaluation") { + w.Write([]byte(fixture("feature_flag/test-flag-group-properties.json"))) + } else if strings.HasPrefix(r.URL.Path, "/batch/") { + // Ignore batch requests + } else { + t.Error("Unknown request made by library") + } + })) + defer server.Close() + + client, _ := NewWithConfig("Csyjlnlun3OzyNJAafdlv", Config{ + PersonalApiKey: "some very secret key", + Endpoint: server.URL, + FeatureFlagRequestTimeout: 10 * time.Millisecond, + }) + defer client.Close() + + isMatch, err := client.IsFeatureEnabled( + FeatureFlagPayload{ + Key: "enabled-flag", + DistinctId: "-", + }, + ) + + if err == nil { + t.Error("Expected error") + } + if !strings.Contains(err.Error(), "context deadline exceeded") { + t.Error("Expected context deadline exceeded error") + } + if isMatch != nil { + t.Error("Flag shouldn't match") + } + + // get all flags with no local evaluation possible + variants, err := client.GetAllFlags( + FeatureFlagPayloadNoKey{ + DistinctId: "-", + Groups: Groups{"company": "posthog"}, + }, + ) + + if err == nil { + t.Error("Expected error") + } + if !strings.Contains(err.Error(), "context deadline exceeded") { + t.Error("Expected context deadline exceeded error") + } + + if variants == nil || len(variants) != 0 { + t.Error("Flag shouldn't match") + } + + // get all flags with partial local evaluation possible + variants, err = client.GetAllFlags( + FeatureFlagPayloadNoKey{ + DistinctId: "-", + Groups: Groups{"company": "posthog"}, + PersonProperties: NewProperties().Set("region", "USA"), + }, + ) + + if err == nil { + t.Error("Expected error") + } + if !strings.Contains(err.Error(), "context deadline exceeded") { + t.Error("Expected context deadline exceeded error") + } + + if variants == nil || len(variants) != 1 || variants["simple-flag"] != true { + t.Error("should return locally evaluated flag") + } + + // get all flags with full local evaluation possible + variants, err = client.GetAllFlags( + FeatureFlagPayloadNoKey{ + DistinctId: "-", + Groups: Groups{"company": "posthog"}, + PersonProperties: NewProperties().Set("region", "USA"), + GroupProperties: map[string]Properties{"company": NewProperties().Set("name", "Project Name 1")}, + }, + ) + + if err != nil { + t.Error("Unexpected error") + } + fmt.Println(variants) + + if variants == nil || len(variants) != 2 || variants["simple-flag"] != true || variants["group-flag"] != true { + t.Error("should return locally evaluated flag") + } +} + +func TestFlagDefinitionsWithTimeoutExceeded(t *testing.T) { + + // create buffer to write logs to + var buf bytes.Buffer + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.HasPrefix(r.URL.Path, "/decide") { + w.Write([]byte(fixture("test-decide-v2.json"))) + } else if strings.HasPrefix(r.URL.Path, "/api/feature_flag/local_evaluation") { + time.Sleep(11 * time.Second) + w.Write([]byte(fixture("feature_flag/test-flag-group-properties.json"))) + } else if strings.HasPrefix(r.URL.Path, "/batch/") { + // Ignore batch requests + } else { + t.Error("Unknown request made by library") + } + })) + defer server.Close() + + client, _ := NewWithConfig("Csyjlnlun3OzyNJAafdlv", Config{ + PersonalApiKey: "some very secret key", + Endpoint: server.URL, + FeatureFlagRequestTimeout: 10 * time.Millisecond, + Logger: StdLogger(log.New(&buf, "posthog-test", log.LstdFlags)), + }) + defer client.Close() + + _, err := client.IsFeatureEnabled( + FeatureFlagPayload{ + Key: "enabled-flag", + DistinctId: "-", + }, + ) + if err != nil { + t.Error("Unexpected error") + } + + output := buf.String() + if !strings.Contains(output, "Unable to fetch feature flags") { + t.Error("Expected error fetching flags") + } + + if !strings.Contains(output, "context deadline exceeded") { + t.Error("Expected timeout error fetching flags") + } +} diff --git a/featureflags.go b/featureflags.go index cc31f58..1a794bc 100644 --- a/featureflags.go +++ b/featureflags.go @@ -2,6 +2,7 @@ package posthog import ( "bytes" + "context" "crypto/sha1" "encoding/json" "errors" @@ -34,6 +35,7 @@ type FeatureFlagsPoller struct { mutex sync.RWMutex fetchedFlagsSuccessfullyOnce bool nextPollTick func() time.Duration + flagTimeout time.Duration } type FeatureFlag struct { @@ -121,6 +123,7 @@ func newFeatureFlagsPoller( httpClient http.Client, pollingInterval time.Duration, nextPollTick func() time.Duration, + flagTimeout time.Duration, ) *FeatureFlagsPoller { if nextPollTick == nil { @@ -139,6 +142,7 @@ func newFeatureFlagsPoller( mutex: sync.RWMutex{}, fetchedFlagsSuccessfullyOnce: false, nextPollTick: nextPollTick, + flagTimeout: flagTimeout, } go poller.run() @@ -169,7 +173,8 @@ func (poller *FeatureFlagsPoller) run() { func (poller *FeatureFlagsPoller) fetchNewFeatureFlags() { personalApiKey := poller.personalApiKey headers := [][2]string{{"Authorization", "Bearer " + personalApiKey + ""}} - res, err := poller.localEvaluationFlags(headers) + res, cancel, err := poller.localEvaluationFlags(headers) + defer cancel() if err != nil || res.StatusCode != http.StatusOK { poller.loaded <- false poller.Errorf("Unable to fetch feature flags", err) @@ -193,9 +198,7 @@ func (poller *FeatureFlagsPoller) fetchNewFeatureFlags() { poller.loaded <- true } newFlags := []FeatureFlag{} - for _, flag := range featureFlagsResponse.Flags { - newFlags = append(newFlags, flag) - } + newFlags = append(newFlags, featureFlagsResponse.Flags...) poller.mutex.Lock() poller.featureFlags = newFlags poller.cohorts = featureFlagsResponse.Cohorts @@ -242,7 +245,7 @@ func (poller *FeatureFlagsPoller) GetFeatureFlag(flagConfig FeatureFlagPayload) result, err = poller.getFeatureFlagVariant(featureFlag, flagConfig.Key, flagConfig.DistinctId, flagConfig.Groups, flagConfig.PersonProperties, flagConfig.GroupProperties) if err != nil { - return nil, nil + return nil, err } } @@ -746,7 +749,7 @@ func interfaceToFloat(val interface{}) (float64, error) { case uint64: i = float64(t) default: - errMessage := "Argument not orderable" + errMessage := "argument not orderable" return 0.0, errors.New(errMessage) } @@ -815,18 +818,18 @@ func (poller *FeatureFlagsPoller) GetFeatureFlags() []FeatureFlag { return poller.featureFlags } -func (poller *FeatureFlagsPoller) decide(requestData []byte, headers [][2]string) (*http.Response, error) { - localEvaluationEndpoint := "decide/?v=2" +func (poller *FeatureFlagsPoller) decide(requestData []byte, headers [][2]string) (*http.Response, context.CancelFunc, error) { + decideEndpoint := "decide/?v=2" - url, err := url.Parse(poller.Endpoint + "/" + localEvaluationEndpoint + "") + url, err := url.Parse(poller.Endpoint + "/" + decideEndpoint + "") if err != nil { poller.Errorf("creating url - %s", err) } - return poller.request("POST", url, requestData, headers) + return poller.request("POST", url, requestData, headers, poller.flagTimeout) } -func (poller *FeatureFlagsPoller) localEvaluationFlags(headers [][2]string) (*http.Response, error) { +func (poller *FeatureFlagsPoller) localEvaluationFlags(headers [][2]string) (*http.Response, context.CancelFunc, error) { localEvaluationEndpoint := "api/feature_flag/local_evaluation" url, err := url.Parse(poller.Endpoint + "/" + localEvaluationEndpoint + "") @@ -838,11 +841,13 @@ func (poller *FeatureFlagsPoller) localEvaluationFlags(headers [][2]string) (*ht searchParams.Add("send_cohorts", "true") url.RawQuery = searchParams.Encode() - return poller.request("GET", url, []byte{}, headers) + return poller.request("GET", url, []byte{}, headers, time.Duration(10)*time.Second) } -func (poller *FeatureFlagsPoller) request(method string, url *url.URL, requestData []byte, headers [][2]string) (*http.Response, error) { - req, err := http.NewRequest(method, url.String(), bytes.NewReader(requestData)) +func (poller *FeatureFlagsPoller) request(method string, url *url.URL, requestData []byte, headers [][2]string, timeout time.Duration) (*http.Response, context.CancelFunc, error) { + ctx, cancel := context.WithTimeout(context.Background(), timeout) + + req, err := http.NewRequestWithContext(ctx, method, url.String(), bytes.NewReader(requestData)) if err != nil { poller.Errorf("creating request - %s", err) } @@ -862,7 +867,7 @@ func (poller *FeatureFlagsPoller) request(method string, url *url.URL, requestDa poller.Errorf("sending request - %s", err) } - return res, err + return res, cancel, err } func (poller *FeatureFlagsPoller) ForceReload() { @@ -888,9 +893,13 @@ func (poller *FeatureFlagsPoller) getFeatureFlagVariants(distinctId string, grou poller.Errorf(errorMessage) return nil, errors.New(errorMessage) } - res, err := poller.decide(requestDataBytes, headers) + res, cancel, err := poller.decide(requestDataBytes, headers) + defer cancel() if err != nil || res.StatusCode != http.StatusOK { errorMessage = "Error calling /decide/" + if err != nil { + errorMessage += " - " + err.Error() + } poller.Errorf(errorMessage) return nil, errors.New(errorMessage) } diff --git a/fixtures/feature_flag/test-flag-group-properties.json b/fixtures/feature_flag/test-flag-group-properties.json index 4b42d65..a956466 100644 --- a/fixtures/feature_flag/test-flag-group-properties.json +++ b/fixtures/feature_flag/test-flag-group-properties.json @@ -1,5 +1,5 @@ { - "count": 1, + "count": 2, "next": null, "previous": null, "flags": [ @@ -28,6 +28,30 @@ "active": true, "is_simple_flag": false, "rollout_percentage": null + }, + { + "id": 719, + "name": "", + "key": "simple-flag", + "filters": { + "groups": [ + { + "properties": [ + { + "key": "region", + "operator": "exact", + "value": ["USA"], + "type": "person" + } + ], + "rollout_percentage": 100 + } + ] + }, + "deleted": false, + "active": true, + "is_simple_flag": true, + "rollout_percentage": null } ], "group_type_mapping" : {"0": "company", "1": "project"} diff --git a/posthog.go b/posthog.go index f38aed7..16a8c02 100644 --- a/posthog.go +++ b/posthog.go @@ -119,6 +119,7 @@ func NewWithConfig(apiKey string, config Config) (cli Client, err error) { c.http, c.DefaultFeatureFlagsPollingInterval, c.NextFeatureFlagsPollingTick, + c.FeatureFlagRequestTimeout, ) } From 87b23fe111030fa2da718b191cc8d570fd14f5fe Mon Sep 17 00:00:00 2001 From: Roman Zaynetdinov Date: Wed, 27 Mar 2024 13:25:32 +0200 Subject: [PATCH 6/6] Do not push to the loaded channel when flags' request fails (#38) --- feature_flags_test.go | 55 +++++++++++++++++++++++++--- featureflags.go | 85 +++++++++++++++++++++---------------------- posthog.go | 2 +- 3 files changed, 91 insertions(+), 51 deletions(-) diff --git a/feature_flags_test.go b/feature_flags_test.go index c2aab19..58d56a4 100644 --- a/feature_flags_test.go +++ b/feature_flags_test.go @@ -9,6 +9,7 @@ import ( "net/http/httptest" "reflect" "strings" + "sync/atomic" "time" "testing" @@ -616,13 +617,9 @@ func TestFeatureFlagNullComeIntoPlayOnlyWhenDecideErrorsOut(t *testing.T) { defer server.Close() - // TODO: Make this nicer, right now if all local evaluation requests fail, we block - // on waiting for atleast one request to happen before returning flags, - // which can be suboptimal client, _ := NewWithConfig("Csyjlnlun3OzyNJAafdlv", Config{ - PersonalApiKey: "some very secret key", - Endpoint: server.URL, - DefaultFeatureFlagsPollingInterval: 5 * time.Second, + PersonalApiKey: "some very secret key", + Endpoint: server.URL, }) defer client.Close() @@ -3384,3 +3381,49 @@ func TestFlagDefinitionsWithTimeoutExceeded(t *testing.T) { t.Error("Expected timeout error fetching flags") } } + +func TestFetchFlagsFails(t *testing.T) { + // This test verifies that even in presence of HTTP errors flags continue to be fetched. + var called uint32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if atomic.LoadUint32(&called) == 0 { + // Load initial flags successfully + w.Write([]byte(fixture("feature_flag/test-simple-flag.json"))) + } else { + // Fail all next requests + w.WriteHeader(http.StatusInternalServerError) + } + atomic.AddUint32(&called, 1) + + })) + defer server.Close() + + client, _ := NewWithConfig("Csyjlnlun3OzyNJAafdlv", Config{ + PersonalApiKey: "some very secret key", + Endpoint: server.URL, + }) + defer client.Close() + + _, err := client.GetFeatureFlags() + if err != nil { + t.Error("Should not fail", err) + } + client.ReloadFeatureFlags() + client.ReloadFeatureFlags() + + _, err = client.GetAllFlags(FeatureFlagPayloadNoKey{ + DistinctId: "my-id", + }) + if err != nil { + t.Error("Should not fail", err) + } + + // Wait for the last request to complete + <-time.After(50 * time.Millisecond) + + const expectedCalls = 3 + actualCalls := atomic.LoadUint32(&called) + if actualCalls != expectedCalls { + t.Error("Expected to be called", expectedCalls, "times but got", actualCalls) + } +} diff --git a/featureflags.go b/featureflags.go index 1a794bc..87c04a8 100644 --- a/featureflags.go +++ b/featureflags.go @@ -21,21 +21,20 @@ import ( const LONG_SCALE = 0xfffffffffffffff type FeatureFlagsPoller struct { - loaded chan bool - shutdown chan bool - forceReload chan bool - featureFlags []FeatureFlag - cohorts map[string]PropertyGroup - groups map[string]string - personalApiKey string - projectApiKey string - Errorf func(format string, args ...interface{}) - Endpoint string - http http.Client - mutex sync.RWMutex - fetchedFlagsSuccessfullyOnce bool - nextPollTick func() time.Duration - flagTimeout time.Duration + loaded chan bool + shutdown chan bool + forceReload chan bool + featureFlags []FeatureFlag + cohorts map[string]PropertyGroup + groups map[string]string + personalApiKey string + projectApiKey string + Errorf func(format string, args ...interface{}) + Endpoint string + http http.Client + mutex sync.RWMutex + nextPollTick func() time.Duration + flagTimeout time.Duration } type FeatureFlag struct { @@ -131,18 +130,17 @@ func newFeatureFlagsPoller( } poller := FeatureFlagsPoller{ - loaded: make(chan bool), - shutdown: make(chan bool), - forceReload: make(chan bool), - personalApiKey: personalApiKey, - projectApiKey: projectApiKey, - Errorf: errorf, - Endpoint: endpoint, - http: httpClient, - mutex: sync.RWMutex{}, - fetchedFlagsSuccessfullyOnce: false, - nextPollTick: nextPollTick, - flagTimeout: flagTimeout, + loaded: make(chan bool), + shutdown: make(chan bool), + forceReload: make(chan bool), + personalApiKey: personalApiKey, + projectApiKey: projectApiKey, + Errorf: errorf, + Endpoint: endpoint, + http: httpClient, + mutex: sync.RWMutex{}, + nextPollTick: nextPollTick, + flagTimeout: flagTimeout, } go poller.run() @@ -151,6 +149,7 @@ func newFeatureFlagsPoller( func (poller *FeatureFlagsPoller) run() { poller.fetchNewFeatureFlags() + close(poller.loaded) for { timer := time.NewTimer(poller.nextPollTick()) @@ -158,7 +157,6 @@ func (poller *FeatureFlagsPoller) run() { case <-poller.shutdown: close(poller.shutdown) close(poller.forceReload) - close(poller.loaded) timer.Stop() return case <-poller.forceReload: @@ -176,27 +174,21 @@ func (poller *FeatureFlagsPoller) fetchNewFeatureFlags() { res, cancel, err := poller.localEvaluationFlags(headers) defer cancel() if err != nil || res.StatusCode != http.StatusOK { - poller.loaded <- false poller.Errorf("Unable to fetch feature flags", err) return } defer res.Body.Close() resBody, err := ioutil.ReadAll(res.Body) if err != nil { - poller.loaded <- false poller.Errorf("Unable to fetch feature flags", err) return } featureFlagsResponse := FeatureFlagsResponse{} err = json.Unmarshal([]byte(resBody), &featureFlagsResponse) if err != nil { - poller.loaded <- false poller.Errorf("Unable to unmarshal response from api/feature_flag/local_evaluation", err) return } - if !poller.fetchedFlagsSuccessfullyOnce { - poller.loaded <- true - } newFlags := []FeatureFlag{} newFlags = append(newFlags, featureFlagsResponse.Flags...) poller.mutex.Lock() @@ -205,12 +197,14 @@ func (poller *FeatureFlagsPoller) fetchNewFeatureFlags() { if featureFlagsResponse.GroupTypeMapping != nil { poller.groups = *featureFlagsResponse.GroupTypeMapping } - poller.fetchedFlagsSuccessfullyOnce = true poller.mutex.Unlock() } func (poller *FeatureFlagsPoller) GetFeatureFlag(flagConfig FeatureFlagPayload) (interface{}, error) { - featureFlags := poller.GetFeatureFlags() + featureFlags, err := poller.GetFeatureFlags() + if err != nil { + return nil, err + } cohorts := poller.cohorts featureFlag := FeatureFlag{Key: ""} @@ -224,7 +218,6 @@ func (poller *FeatureFlagsPoller) GetFeatureFlag(flagConfig FeatureFlagPayload) } var result interface{} - var err error if featureFlag.Key != "" { result, err = poller.computeFlagLocally( @@ -254,7 +247,10 @@ func (poller *FeatureFlagsPoller) GetFeatureFlag(flagConfig FeatureFlagPayload) func (poller *FeatureFlagsPoller) GetAllFlags(flagConfig FeatureFlagPayloadNoKey) (map[string]interface{}, error) { response := map[string]interface{}{} - featureFlags := poller.GetFeatureFlags() + featureFlags, err := poller.GetFeatureFlags() + if err != nil { + return nil, err + } fallbackToDecide := false cohorts := poller.cohorts @@ -808,14 +804,15 @@ func _hash(key string, distinctId string, salt string) (float64, error) { return float64(value) / LONG_SCALE, nil } -func (poller *FeatureFlagsPoller) GetFeatureFlags() []FeatureFlag { - // ensure flags are loaded on the first call - - if !poller.fetchedFlagsSuccessfullyOnce { - <-poller.loaded +func (poller *FeatureFlagsPoller) GetFeatureFlags() ([]FeatureFlag, error) { + // When channel is open this will block. When channel is closed it will immediately exit. + _, closed := <-poller.loaded + if closed && poller.featureFlags == nil { + // There was an error with initial flag fetching + return nil, fmt.Errorf("Flags were not successfully fetched yet") } - return poller.featureFlags + return poller.featureFlags, nil } func (poller *FeatureFlagsPoller) decide(requestData []byte, headers [][2]string) (*http.Response, context.CancelFunc, error) { diff --git a/posthog.go b/posthog.go index 16a8c02..8af24ea 100644 --- a/posthog.go +++ b/posthog.go @@ -305,7 +305,7 @@ func (c *client) GetFeatureFlags() ([]FeatureFlag, error) { c.Errorf(errorMessage) return nil, errors.New(errorMessage) } - return c.featureFlagsPoller.GetFeatureFlags(), nil + return c.featureFlagsPoller.GetFeatureFlags() } func (c *client) GetAllFlags(flagConfig FeatureFlagPayloadNoKey) (map[string]interface{}, error) {