fix(flags): send feature_flag_called events with correct payload data in the event that we need to fetch the payloads from the server
#315
Conversation
|
Size Change: +500 B (+0.46%) Total Size: 108 kB
ℹ️ View Unchanged
|
| matchValue = await this.getFeatureFlag(key, distinctId, { | ||
| ...options, | ||
| onlyEvaluateLocally: true, | ||
| sendFeatureFlagEvents: false, |
There was a problem hiding this comment.
explicitly don't send events when we evaluate the feature flag (since these events won't have response data for the payload), we'll send them manually from this method insstead.
| expect( | ||
| await posthog.getFeatureFlagPayload('beta-feature', 'some-distinct-id', undefined, { | ||
| personProperties: { region: 'USA', name: 'Aloha' }, | ||
| }) | ||
| ).toEqual({ variant: 'A' }) | ||
|
|
||
| // TRICKY: There's now an extra step before events are queued, so need to wait for that to resolve | ||
| jest.runOnlyPendingTimers() | ||
| await waitForPromises() | ||
| await posthog.flush() | ||
|
|
||
| expect(mockedFetch).toHaveBeenCalledWith('http://example.com/batch/', expect.any(Object)) | ||
|
|
||
| expect(getLastBatchEvents()?.[0]).toEqual( | ||
| expect.objectContaining({ | ||
| distinct_id: 'some-distinct-id', | ||
| event: '$feature_flag_payload_called', | ||
| properties: expect.objectContaining({ | ||
| $feature_flag: 'beta-feature', | ||
| $feature_flag_response: true, | ||
| $feature_flag_payload: { variant: 'A' }, | ||
| locally_evaluated: true, | ||
| [`$feature/${'beta-feature'}`]: true, | ||
| }), | ||
| }) | ||
| ) | ||
| mockedFetch.mockClear() |
There was a problem hiding this comment.
added a test case that the getFeatureFlagPayload method also sends a feature_flag_called event.
| properties: { | ||
| $feature_flag: key, | ||
| $feature_flag_response: finalResponse, | ||
| $feature_flag_payload: finalPayload, |
There was a problem hiding this comment.
is this flag only for local evaluation? I don't think we send this one for any other Mobile SDK, but we do support feature flag payloads
There was a problem hiding this comment.
is this flag only for local evaluation?
Not sure what you mean by that; what I did here is add a new property to the feature flag called event that includes the feature_flag_payload anytime a user calls getFeatureFlagPayload.
There was a problem hiding this comment.
What I meant is that I did not see this new property $feature_flag_payload anywhere else yet, is that new? if so, we'd need to add to other SDKs as well?
JS for example has $feature_flag_payloads (plural with S) but its not used when capturing a $feature_flag_called event.
There was a problem hiding this comment.
So my question about local evaluation is if this is only for backend SDKs (only backend SDKs have local evaluation afaik), because I have not found any reference on the JS SDK
There was a problem hiding this comment.
AH I see! Okay this is a good point to raise: yes; right now this fallback on happens for local evaluation on backend SDKs, and we don't include any payload information in these events. In fact, I don't even think we send feature_flag_called events whenever a client-side SDK is used to get payloads, the code just goes like this
getFeatureFlagPayload(key: string): JsonType {
const payloads = this.getFlagPayloads()
return payloads[key]
}
where getFlagPayloads just loads the payloads from persistence.
getFlagPayloads(): Record<string, JsonType> {
const flagPayloads = this.instance.get_property(PERSISTENCE_FEATURE_FLAG_PAYLOADS)
return flagPayloads || {}
}
So this is already feature gap between client-side and server side SDKs in more ways than just including the payload – our server-side SDKs send feature_flag_called events on both getFeatureFlag and getFeatureFlagPayload calls. I feel like I should make this documentation clear somewhere if it isn't already, since users could get confused.
There was a problem hiding this comment.
I don't even think we send feature_flag_called events whenever a client-side SDK is used to get payloads
I think the reason is that before you read payloads, you read the flag using getFeatureFlag or isFeatureEnabled.
I don't think it's common to consume getFeatureFlagPayload directly before either of them
There was a problem hiding this comment.
yeah, I think that's a common pattern for the client-side SDKs that the server-side ones may not follow as much (e.g., the reason I started working on this change at all was because we have a user that's using our python SDK only for the get_feature_flag_payload method).
Okay, so here's what I'm going to do going forward:
- ship this change to the node SDK
- keep posthog-js as is
Thanks for chatting about it with me!
|
@dmarticus for releasing. |
Follow on to this discussion and this slack thread, it looks like the bug with the Python SDK (where we send empty responses in our
feature_flag_calledevents when we callget_feature_flag_payload) is also present in the Node SDK.This change makes it so that we ensure that we have a relevant response + payload to send on the event body whenever we fall back to remotely evaluating the feature flag payload.