feat(flags): fall back to `/decide` endpoint for `GetFeatureFlag` and `GetAllFlags` so that users can use this library without needing a personal API Key #46

dmarticus · 2024-08-06T21:21:35Z

Does what it says on the tin. I made the following changes:

Fallback logic in GetFeatureFlag and GetAllFlags that falls back to using the /decide endpoint for fetching feature flags if there's no PersonalApiKey provided
Tests for this new behavior

Potential TODOs:

Log the fallback?
improve error messages with missing token to tell the users they can fall back?

Other things:

Doc update associated with these changes: Added info about the personal api key for the Go SDK posthog.com#9121

dmarticus · 2024-08-06T21:23:51Z

feature_flags_test.go

@@ -325,7 +325,7 @@ func TestFlagGroup(t *testing.T) {
 				t.Errorf("Expected personProperties to be map[region:Canada], got %s", reqBody.PersonProperties)
 			}

-			groupPropertiesEquality := reflect.DeepEqual(reqBody.GroupProperties, map[string]Properties{"company": Properties{"name": "Project Name 1"}})
+			groupPropertiesEquality := reflect.DeepEqual(reqBody.GroupProperties, map[string]Properties{"company": {"name": "Project Name 1"}})


compiler told me that Properties field was unused.

dmarticus · 2024-08-06T21:24:00Z

featureflags.go

@@ -311,7 +311,7 @@ func (poller *FeatureFlagsPoller) computeFlagLocally(
 		groupName, exists := poller.groups[fmt.Sprintf("%d", *flag.Filters.AggregationGroupTypeIndex)]

 		if !exists {
-			errMessage := "Flag has unknown group type index"
+			errMessage := "flag has unknown group type index"


compiler told me to lowercase error messages

dmarticus · 2024-08-06T21:24:06Z

featureflags.go

@@ -467,7 +467,7 @@ func matchCohort(property FlagProperty, properties Properties, cohorts map[strin
 	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 false, fmt.Errorf("can't match cohort: cohort %s not found", cohortId)


compiler told me to lowercase error messages

dmarticus · 2024-08-06T21:24:12Z

featureflags.go

@@ -578,7 +578,7 @@ func matchProperty(property FlagProperty, properties Properties) (bool, error) {
 		return false, &InconclusiveMatchError{"Can't match properties with operator is_not_set"}
 	}

-	override_value, _ := properties[key]
+	override_value := properties[key]


compiler told me this was unused.

dmarticus · 2024-08-06T21:24:15Z

featureflags.go

@@ -637,7 +637,7 @@ func matchProperty(property FlagProperty, properties Properties) (bool, error) {
 			valueString = strconv.Itoa(valueInt)
 			r, err = regexp.Compile(valueString)
 		} else {
-			errMessage := "Regex expression not allowed"
+			errMessage := "regex expression not allowed"


compiler told me to lowercase error messages

dmarticus · 2024-08-06T21:26:28Z

posthog_test.go

@@ -705,7 +706,7 @@ func TestClientMaxConcurrentRequests(t *testing.T) {
 			func(m APIMessage, e error) { errchan <- e },
 		},
 		Transport: testTransportDelayed,
-		// Only one concurreny request can be submitted, because the transport
+		// Only one concurrency request can be submitted, because the transport


drive-by spellcheck

dmarticus · 2024-08-06T21:27:05Z

posthog_test.go

+	receivedErrors := [2]error{}
 	receivedErrors[0] = client.ReloadFeatureFlags()
-	_, receivedErrors[1] = client.IsFeatureEnabled(
-		FeatureFlagPayload{
-			Key:        "some key",
-			DistinctId: "some id",
-		},
-	)
-	_, receivedErrors[2] = client.GetFeatureFlag(
-		FeatureFlagPayload{
-			Key:        "some key",
-			DistinctId: "some id",
-		},
-	)
-	_, receivedErrors[3] = client.GetFeatureFlags()
+	_, receivedErrors[1] = client.GetFeatureFlags()


we only have 2 methods that error on a missing PersonalApiKey, and neither of them are hot paths (i.e. users don't need them to use our feature flags).

dmarticus · 2024-08-06T21:28:29Z

posthog_test.go

@@ -766,6 +755,442 @@ func TestFeatureFlagsWithNoPersonalApiKey(t *testing.T) {

 }

+func TestIsFeatureEnabled(t *testing.T) {


this method didn't have tests, so now it does. Boy scout principle!

dmarticus · 2024-08-06T21:28:43Z

posthog_test.go

+	}
+}
+
+func TestGetFeatureFlagWithNoPersonalApiKey(t *testing.T) {


test the fallbacks

dmarticus · 2024-08-06T21:28:57Z

posthog_test.go

-
-	// flagValue, valueError := client.GetFeatureFlag("simpleFlag", "hey", false, Groups{}, NewProperties(), map[string]Properties{})
-	// if valueError != nil || flagValue != true {
-	// 	t.Errorf("simple flag with null rollout percentage should have value 'true'")
-	// }


this was dead code

dmarticus · 2024-08-06T21:29:23Z

posthog.go

+	if c.featureFlagsPoller != nil {
+		// get feature flags from the poller, which uses the personal api key
+		flagValue, err = c.featureFlagsPoller.GetFeatureFlag(flagConfig)
+	} else {


should I log that we're falling back here? Does it matter?

maybe, but only at a trace/info/debug level. Might be good for investigations.

fuziontech

A few comments, but otherwise this is sick
🚢 it

PS nice tests!

fuziontech · 2024-08-06T22:32:33Z

posthog.go

+func (c *client) setLastCapturedEvent(event Capture) {
+	c.lastEventMutex.Lock()
+	defer c.lastEventMutex.Unlock()
+	c.lastCapturedEvent = &event
+}
+
+func (c *client) GetLastCapturedEvent() *Capture {
+	c.lastEventMutex.RLock()
+	defer c.lastEventMutex.RUnlock()
+	if c.lastCapturedEvent == nil {
+		return nil
+	}
+	// Return a copy to avoid data races
+	eventCopy := *c.lastCapturedEvent
+	return &eventCopy
+}


eh, should be fine. Doesn't seem too unsafe to expose

fuziontech · 2024-08-06T22:38:57Z

posthog.go

+	if c.featureFlagsPoller != nil {
+		// get feature flags from the poller, which uses the personal api key
+		flagValue, err = c.featureFlagsPoller.GetFeatureFlag(flagConfig)
+	} else {


maybe, but only at a trace/info/debug level. Might be good for investigations.

Phanatic

Ship it!

initial commit

ef035ae

dmarticus requested review from Phanatic and jurajmajerik August 6, 2024 21:22

dmarticus commented Aug 6, 2024

View reviewed changes

info about the personal api key

0db81cf

dmarticus mentioned this pull request Aug 6, 2024

Added info about the personal api key for the Go SDK PostHog/posthog.com#9121

Merged

fuziontech approved these changes Aug 6, 2024

View reviewed changes

Phanatic approved these changes Aug 7, 2024

View reviewed changes

dmarticus added 3 commits August 7, 2024 15:54

adding docs

ae4cd24

logs and comments

34e3360

adding changelog

3dba1c0

dmarticus merged commit 76dfd13 into master Aug 7, 2024
1 check passed

dmarticus deleted the feat/fallback-to-decide-with-no-api-token branch August 7, 2024 23:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(flags): fall back to `/decide` endpoint for `GetFeatureFlag` and `GetAllFlags` so that users can use this library without needing a personal API Key #46

feat(flags): fall back to `/decide` endpoint for `GetFeatureFlag` and `GetAllFlags` so that users can use this library without needing a personal API Key #46

dmarticus commented Aug 6, 2024 •

edited

Loading

dmarticus Aug 6, 2024

dmarticus Aug 6, 2024

dmarticus Aug 6, 2024

dmarticus Aug 6, 2024

dmarticus Aug 6, 2024

dmarticus Aug 6, 2024

dmarticus Aug 6, 2024

dmarticus Aug 6, 2024

dmarticus Aug 6, 2024

dmarticus Aug 6, 2024

dmarticus Aug 6, 2024

fuziontech Aug 6, 2024

fuziontech left a comment •

edited

Loading

fuziontech Aug 6, 2024

fuziontech Aug 6, 2024

Phanatic left a comment

		@@ -766,6 +755,442 @@ func TestFeatureFlagsWithNoPersonalApiKey(t *testing.T) {

		}

		func TestIsFeatureEnabled(t *testing.T) {

feat(flags): fall back to /decide endpoint for GetFeatureFlag and GetAllFlags so that users can use this library without needing a personal API Key #46

feat(flags): fall back to /decide endpoint for GetFeatureFlag and GetAllFlags so that users can use this library without needing a personal API Key #46

Conversation

dmarticus commented Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuziontech left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Phanatic left a comment

Choose a reason for hiding this comment

feat(flags): fall back to `/decide` endpoint for `GetFeatureFlag` and `GetAllFlags` so that users can use this library without needing a personal API Key #46

feat(flags): fall back to `/decide` endpoint for `GetFeatureFlag` and `GetAllFlags` so that users can use this library without needing a personal API Key #46

dmarticus commented Aug 6, 2024 •

edited

Loading

fuziontech left a comment •

edited

Loading