add support for fetching feature flag payloads #64
Conversation
There was a problem hiding this comment.
Excellent work! I have a few minor notes and a suggestion but this is great work and I really appreciate the thorough testing. Thank you for helping us make our Go SDK better! Let me know what you think of my notes and then I'm happy to approve this and get it merged.
posthog_test.go
Outdated
| // should we be capturing an event here? | ||
| // lastEvent := client.GetLastCapturedEvent() | ||
| // if lastEvent == nil || lastEvent.Event != "$feature_flag_called" { | ||
| // t.Errorf("Expected a $feature_flag_called event, got: %v", lastEvent) | ||
| // } | ||
|
|
||
| // Check that the properties of the captured event match the response from /decide | ||
| // if lastEvent != nil { | ||
| // if lastEvent.Properties["$feature_flag"] != "beta-feature" { | ||
| // t.Errorf("Expected feature flag key 'beta-feature', got: %v", lastEvent.Properties["$feature_flag"]) | ||
| // } | ||
| // if lastEvent.Properties["$feature_flag_response"] != expectedPayload { | ||
| // t.Errorf("Expected feature flag response %v, got: %v", expectedPayload, lastEvent.Properties["$feature_flag_response"]) | ||
| // } | ||
| // } |
There was a problem hiding this comment.
| // should we be capturing an event here? | |
| // lastEvent := client.GetLastCapturedEvent() | |
| // if lastEvent == nil || lastEvent.Event != "$feature_flag_called" { | |
| // t.Errorf("Expected a $feature_flag_called event, got: %v", lastEvent) | |
| // } | |
| // Check that the properties of the captured event match the response from /decide | |
| // if lastEvent != nil { | |
| // if lastEvent.Properties["$feature_flag"] != "beta-feature" { | |
| // t.Errorf("Expected feature flag key 'beta-feature', got: %v", lastEvent.Properties["$feature_flag"]) | |
| // } | |
| // if lastEvent.Properties["$feature_flag_response"] != expectedPayload { | |
| // t.Errorf("Expected feature flag response %v, got: %v", expectedPayload, lastEvent.Properties["$feature_flag_response"]) | |
| // } | |
| // } |
yeah, this is a reasonable callout! I included this originally so that I could test events being captured correctly, but it doesn't make sense to include it in the SDK overall since it's one extra capture call that's not strictly necessary. I'm fine to remove this.
| if err := flagConfig.validate(); err != nil { | ||
| return "", err | ||
| } |
There was a problem hiding this comment.
I'd be inclined to return false instead of "" here, since we're returning the more permissive interface{} type. This is also the pattern we use elsewhere throughout this SDK, e.g. here
| func (c *client) getFeatureFlagPayloadFromDecide(key string, distinctId string, groups Groups, personProperties Properties, groupProperties map[string]Properties) (string, error) { | ||
| decideResponse, err := c.makeDecideRequest(distinctId, groups, personProperties, groupProperties) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| if value, ok := decideResponse.FeatureFlagPayloads[key]; ok { | ||
| return value, nil | ||
| } | ||
|
|
||
| return "", nil | ||
| } | ||
|
|
There was a problem hiding this comment.
I see what you mean when you're asking about when to return "", nil, since this is an example where you do that. Currently, the model we've kept for this SDK (that I've sure you've seen) is to have all of these methods return interface{}, error, instead of something with better types. The original decision was before my time, but I think the idea was probably to flexibly handle the fact that a feature flag could either be true, false, or a string.
However, now that we're venturing into handling payloads, I'm fine with this approach of more explicit typing, since the featureFlagPayload values should always be strings.
Since you're the one implementing this new feature (no one else is using payloads in the Go SDK yet), I think it's up to you – if you want to work with feature flag payload values as strings everywhere, then we can keep it like this. Perhaps it's worth calling out the difference between feature flag payload values vs feature flag values (the types do a good job of explaining what, but maybe a comment is worth the why). But overall I think this approach is reasonable.
There was a problem hiding this comment.
Got it! I'm inclined to change the return type from interface{} to string across the board, including in the user-facing method GetFeatureFlagPayload. If I have the opportunity to use explicit types, I'll take it.
The changes I'll be making are:
- Changing the return type from interface{} to string.
- Returning an empty string and a nil error if there was no actual error, but the feature flag doesn't have a payload defined. I don't think there is a need to return special value or error if the feature flag doesn't have a payload, but you are explicitly trying to fetch it.
- Return an empty string and an error if an actual error occurred.
Thoughts?
There was a problem hiding this comment.
yup, that sounds good to me!
Adds support for feature flag payloads. It works for both local evaluation and decide endpoint, also supports multivariate flags. Usage:
Questions
When should we return an empty string and nil as the payload?