Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 10 commits into from
Nov 26, 2024
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1093c2d
tests for new behavior
dmarticus Nov 21, 2024
4a8a30c
refactored code slightly
dmarticus Nov 21, 2024
84e89d6
made the method a bit easier to read
dmarticus Nov 21, 2024
16e3988
linting
dmarticus Nov 22, 2024
58bec8a
fix
dmarticus Nov 22, 2024
9d805fd
big more
dmarticus Nov 22, 2024
83419cb
Merge branch 'main' into fix/feature-flag-payload-events
marandaneto Nov 22, 2024
9e67950
bump version add changelog
dmarticus Nov 26, 2024
7b78ceb
merge conflicts
dmarticus Nov 26, 2024
745e622
more merge conflicts
dmarticus Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 53 additions & 30 deletions posthog-node/src/posthog-node.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,59 +287,82 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 {
disableGeoip?: boolean
}
): Promise<JsonType | undefined> {
const { groups, disableGeoip } = options || {}
let { onlyEvaluateLocally, sendFeatureFlagEvents, personProperties, groupProperties } = options || {}
const { groups, disableGeoip, onlyEvaluateLocally = false, personProperties, groupProperties } = options || {}

const adjustedProperties = this.addLocalPersonAndGroupProperties(
const { allPersonProperties, allGroupProperties } = this.addLocalPersonAndGroupProperties(
distinctId,
groups,
personProperties,
groupProperties
)

personProperties = adjustedProperties.allPersonProperties
groupProperties = adjustedProperties.allGroupProperties

let response = undefined

// Try to get match value locally if not provided
if (!matchValue) {
if (matchValue === undefined) {
matchValue = await this.getFeatureFlag(key, distinctId, {
...options,
onlyEvaluateLocally: true,
sendFeatureFlagEvents: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

})
}

if (matchValue) {
response = await this.featureFlagsPoller?.computeFeatureFlagPayloadLocally(key, matchValue)
}
let response: string | boolean | undefined
let payload: JsonType | undefined

// set defaults
if (onlyEvaluateLocally == undefined) {
onlyEvaluateLocally = false
}
if (sendFeatureFlagEvents == undefined) {
sendFeatureFlagEvents = true
if (matchValue) {
response = matchValue
payload = await this.featureFlagsPoller?.computeFeatureFlagPayloadLocally(key, matchValue)
} else {
response = undefined
payload = undefined
}

// set defaults
if (onlyEvaluateLocally == undefined) {
onlyEvaluateLocally = false
}
// Determine if the payload was evaluated locally
const payloadWasLocallyEvaluated = payload !== undefined

const payloadWasLocallyEvaluated = response !== undefined
// Fetch final flags and payloads either locally or from the remote server
let fetchedOrLocalFlags: Record<string, string | boolean> | undefined
let fetchedOrLocalPayloads: Record<string, JsonType | undefined> | undefined

if (!payloadWasLocallyEvaluated && !onlyEvaluateLocally) {
response = await super.getFeatureFlagPayloadStateless(
key,
if (payloadWasLocallyEvaluated || onlyEvaluateLocally) {
if (response !== undefined) {
fetchedOrLocalFlags = { [key]: response }
fetchedOrLocalPayloads = { [key]: payload }
} else {
fetchedOrLocalFlags = {}
fetchedOrLocalPayloads = {}
}
} else {
const fetchedData = await super.getFeatureFlagsAndPayloadsStateless(
distinctId,
groups,
personProperties,
groupProperties,
allPersonProperties,
allGroupProperties,
disableGeoip
)
fetchedOrLocalFlags = fetchedData.flags || {}
fetchedOrLocalPayloads = fetchedData.payloads || {}
}
return response

// Extract the final response and payload for the specific key
const finalResponse = fetchedOrLocalFlags[key]
const finalPayload = fetchedOrLocalPayloads[key]
const finalLocallyEvaluated = payloadWasLocallyEvaluated

this.capture({
distinctId,
event: '$feature_flag_called',
properties: {
$feature_flag: key,
$feature_flag_response: finalResponse,
$feature_flag_payload: finalPayload,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@marandaneto marandaneto Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

locally_evaluated: finalLocallyEvaluated,
[`$feature/${key}`]: finalResponse,
},
groups,
disableGeoip,
})

// Return the final payload
return finalPayload
}

async isFeatureEnabled(
Expand Down
37 changes: 35 additions & 2 deletions posthog-node/test/posthog-node.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -871,13 +871,18 @@ describe('PostHog Node.js', () => {
rollout_percentage: 100,
},
],
payloads: { true: { variant: 'A' } },
},
},
],
}

mockedFetch.mockImplementation(
apiImplementation({ localFlags: flags, decideFlags: { 'decide-flag': 'decide-value' } })
apiImplementation({
localFlags: flags,
decideFlags: { 'decide-flag': 'decide-value' },
decideFlagPayloads: { 'beta-feature': { variant: 'A' } },
})
)

posthog = new PostHog('TEST_API_KEY', {
Expand Down Expand Up @@ -919,6 +924,34 @@ describe('PostHog Node.js', () => {
)
mockedFetch.mockClear()

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_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()

// # called again for same user, shouldn't call capture again
expect(
await posthog.getFeatureFlag('beta-feature', 'some-distinct-id', {
Expand Down Expand Up @@ -1055,7 +1088,7 @@ describe('PostHog Node.js', () => {
expect(mockedFetch).toHaveBeenCalledTimes(0)

await expect(posthog.getFeatureFlagPayload('false-flag', '123', true)).resolves.toEqual(300)
expect(mockedFetch).toHaveBeenCalledTimes(0)
expect(mockedFetch).toHaveBeenCalledTimes(1) // this now calls the server, because in this case the flag is not locally evaluated but we have a payload that we need to calculate
})

it('should not double parse json with getFeatureFlagPayloads and server eval', async () => {
Expand Down
Loading