Skip to content

Commit

Permalink
fix: local props refactor (#150)
Browse files Browse the repository at this point in the history
  • Loading branch information
@benjackwhite
benjackwhite authored Jan 9, 2024
1 parent e33d786 commit e791895
Show file tree
Hide file tree
Showing 4 changed files with 83 additions and 87 deletions.
10 changes: 7 additions & 3 deletions posthog-core/test/test-utils/test-utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,13 @@ export const wait = async (t: number): Promise<void> => {
}

export const waitForPromises = async (): Promise<void> => {
jest.useRealTimers()
await new Promise((resolve) => setTimeout(resolve, 100) as any)
jest.useFakeTimers()
await new Promise((resolve) => {
// IMPORTANT: Only enable real timers for this promise - allows us to pass a short amount of ticks
// whilst keeping any timers made during other promises as fake timers
jest.useRealTimers()
setTimeout(resolve, 10)
jest.useFakeTimers()
})
}

export const parseBody = (mockCall: any): any => {
Expand Down
12 changes: 8 additions & 4 deletions posthog-node/src/feature-flags.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,14 +211,18 @@ class FeatureFlagsPoller {
const groupName = this.groupTypeMapping[String(aggregation_group_type_index)]

if (!groupName) {
console.warn(
`[FEATURE FLAGS] Unknown group type index ${aggregation_group_type_index} for feature flag ${flag.key}`
)
if (this.debugMode) {
console.warn(
`[FEATURE FLAGS] Unknown group type index ${aggregation_group_type_index} for feature flag ${flag.key}`
)
}
throw new InconclusiveMatchError('Flag has unknown group type index')
}

if (!(groupName in groups)) {
console.warn(`[FEATURE FLAGS] Can't compute group feature flag: ${flag.key} without group names passed in`)
if (this.debugMode) {
console.warn(`[FEATURE FLAGS] Can't compute group feature flag: ${flag.key} without group names passed in`)
}
return false
}

Expand Down
108 changes: 46 additions & 62 deletions posthog-node/src/posthog-node.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,70 +105,54 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 {
super.captureStateless(distinctId, event, props, { timestamp, disableGeoip })
}

if (sendFeatureFlags) {
// :TRICKY: If we flush, or need to shut down, to not lose events we want this promise to resolve before we flush
this.addPendingPromise(
super
.getFeatureFlagsStateless(distinctId, groups, undefined, undefined, disableGeoip)
.then((flags) => {
const featureVariantProperties: Record<string, string | boolean> = {}
if (flags) {
for (const [feature, variant] of Object.entries(flags)) {
if (variant !== false) {
featureVariantProperties[`$feature/${feature}`] = variant
}
}
}
const activeFlags = Object.keys(flags || {}).filter((flag) => flags?.[flag] !== false)
const flagProperties = {
$active_feature_flags: activeFlags || undefined,
...featureVariantProperties,
}
_capture({ ...properties, $groups: groups, ...flagProperties })
})
.catch(() => {
_capture({ ...properties, $groups: groups })
})
)
} else if ((this.featureFlagsPoller?.featureFlags?.length || 0) > 0) {
const groupsWithStringValues: Record<string, string> = {}
for (const [key, value] of Object.entries(groups || {})) {
groupsWithStringValues[key] = String(value)
}
// :TRICKY: If we flush, or need to shut down, to not lose events we want this promise to resolve before we flush
const capturePromise = Promise.resolve()
.then(async () => {
if (sendFeatureFlags) {
// If we are sending feature flags, we need to make sure we have the latest flags
return await super.getFeatureFlagsStateless(distinctId, groups, undefined, undefined, disableGeoip)
}

// :TRICKY: If we flush, or need to shut down, to not lose events we want this promise to resolve before we flush
this.addPendingPromise(
this.getAllFlags(distinctId, {
groups: groupsWithStringValues,
disableGeoip,
onlyEvaluateLocally: true,
})
.then((flags) => {
const featureVariantProperties: Record<string, string | boolean> = {}
if (flags) {
for (const [feature, variant] of Object.entries(flags)) {
featureVariantProperties[`$feature/${feature}`] = variant
}
}
const activeFlags = Object.keys(flags || {}).filter((flag) => flags?.[flag] !== false)
let flagProperties: Record<string, any> = {
...featureVariantProperties,
}
if (activeFlags.length > 0) {
flagProperties = {
...flagProperties,
$active_feature_flags: activeFlags,
}
}
_capture({ ...flagProperties, ...properties, $groups: groups })
if ((this.featureFlagsPoller?.featureFlags?.length || 0) > 0) {
// Otherwise we may as well check for the flags locally and include them if there
const groupsWithStringValues: Record<string, string> = {}
for (const [key, value] of Object.entries(groups || {})) {
groupsWithStringValues[key] = String(value)
}

return await this.getAllFlags(distinctId, {
groups: groupsWithStringValues,
disableGeoip,
onlyEvaluateLocally: true,
})
.catch(() => {
_capture({ ...properties, $groups: groups })
})
)
} else {
_capture({ ...properties, $groups: groups })
}
}
return {}
})
.then((flags) => {
// Derive the relevant flag properties to add
const additionalProperties: Record<string, any> = {}
if (flags) {
for (const [feature, variant] of Object.entries(flags)) {
additionalProperties[`$feature/${feature}`] = variant
}
}
const activeFlags = Object.keys(flags || {}).filter((flag) => flags?.[flag] !== false)
if (activeFlags.length > 0) {
additionalProperties['$active_feature_flags'] = activeFlags
}

return additionalProperties
})
.catch(() => {
// Something went wrong getting the flag info - we should capture the event anyways
return {}
})
.then((additionalProperties) => {
// No matter what - capture the event
_capture({ ...additionalProperties, ...properties, $groups: groups })
})

this.addPendingPromise(capturePromise)
}

identify({ distinctId, properties, disableGeoip }: IdentifyMessageV1): void {
Expand Down
40 changes: 22 additions & 18 deletions posthog-node/test/posthog-node.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ describe('PostHog Node.js', () => {
expect(mockedFetch).toHaveBeenCalledTimes(0)
posthog.capture({ distinctId: '123', event: 'test-event', properties: { foo: 'bar' }, groups: { org: 123 } })

await waitForPromises()
jest.runOnlyPendingTimers()

const batchEvents = getLastBatchEvents()
expect(batchEvents).toEqual([
{
Expand All @@ -76,6 +78,7 @@ describe('PostHog Node.js', () => {
expect(mockedFetch).toHaveBeenCalledTimes(0)
posthog.capture({ distinctId: '123', event: 'test-event', properties: { foo: 'bar' }, groups: { org: 123 } })

await waitForPromises()
jest.runOnlyPendingTimers()
expect(getLastBatchEvents()?.[0]).toEqual(
expect.objectContaining({
Expand All @@ -98,6 +101,7 @@ describe('PostHog Node.js', () => {
groups: { other_group: 'x' },
})

await waitForPromises()
jest.runOnlyPendingTimers()
expect(getLastBatchEvents()?.[0]).toEqual(
expect.objectContaining({
Expand Down Expand Up @@ -173,6 +177,7 @@ describe('PostHog Node.js', () => {
it('should allow overriding timestamp', async () => {
expect(mockedFetch).toHaveBeenCalledTimes(0)
posthog.capture({ event: 'custom-time', distinctId: '123', timestamp: new Date('2021-02-03') })
await waitForPromises()
jest.runOnlyPendingTimers()
const batchEvents = getLastBatchEvents()
expect(batchEvents).toMatchObject([
Expand All @@ -194,6 +199,7 @@ describe('PostHog Node.js', () => {
disableGeoip: false,
})

await waitForPromises()
jest.runOnlyPendingTimers()
const batchEvents = getLastBatchEvents()
expect(batchEvents?.[0].properties).toEqual({
Expand All @@ -212,7 +218,9 @@ describe('PostHog Node.js', () => {
})
client.capture({ distinctId: '123', event: 'test-event', properties: { foo: 'bar' }, groups: { org: 123 } })

await waitForPromises()
jest.runOnlyPendingTimers()

let batchEvents = getLastBatchEvents()
expect(batchEvents?.[0].properties).toEqual({
$groups: { org: 123 },
Expand All @@ -229,6 +237,7 @@ describe('PostHog Node.js', () => {
disableGeoip: true,
})

await waitForPromises()
jest.runOnlyPendingTimers()
batchEvents = getLastBatchEvents()
console.warn(batchEvents)
Expand All @@ -248,6 +257,7 @@ describe('PostHog Node.js', () => {
disableGeoip: false,
})

await waitForPromises()
jest.runOnlyPendingTimers()
batchEvents = getLastBatchEvents()
expect(batchEvents?.[0].properties).toEqual({
Expand Down Expand Up @@ -352,7 +362,6 @@ describe('PostHog Node.js', () => {
$lib: 'posthog-node',
$lib_version: '1.2.3',
$geoip_disable: true,
$active_feature_flags: [],
},
timestamp: expect.any(String),
type: 'capture',
Expand Down Expand Up @@ -542,15 +551,14 @@ describe('PostHog Node.js', () => {
sendFeatureFlags: true,
})

jest.runOnlyPendingTimers()
await waitForPromises()

expect(mockedFetch).toHaveBeenCalledWith(
'http://example.com/decide/?v=3',
expect.objectContaining({ method: 'POST' })
)

jest.runOnlyPendingTimers()

await waitForPromises()

expect(getLastBatchEvents()?.[0]).toEqual(
expect.objectContaining({
distinct_id: 'distinct_id',
Expand Down Expand Up @@ -589,7 +597,6 @@ describe('PostHog Node.js', () => {
})

jest.runOnlyPendingTimers()

await waitForPromises()

posthog.capture({
Expand Down Expand Up @@ -646,15 +653,14 @@ describe('PostHog Node.js', () => {
personalApiKey: 'TEST_PERSONAL_API_KEY',
})

jest.runOnlyPendingTimers()

await waitForPromises()

posthog.capture({
distinctId: 'distinct_id',
event: 'node test event',
})

jest.runOnlyPendingTimers()
await waitForPromises()

expect(mockedFetch).toHaveBeenCalledWith(...anyLocalEvalCall)
// no decide call
expect(mockedFetch).not.toHaveBeenCalledWith(
Expand Down Expand Up @@ -703,19 +709,19 @@ describe('PostHog Node.js', () => {
disableGeoip: false,
})

await waitForPromises()
jest.runOnlyPendingTimers()

expect(mockedFetch).toHaveBeenCalledWith(
'http://example.com/decide/?v=3',
expect.objectContaining({ method: 'POST', body: expect.not.stringContaining('geoip_disable') })
)

jest.runOnlyPendingTimers()

await waitForPromises()

expect(getLastBatchEvents()?.[0].properties).toEqual({
$active_feature_flags: ['feature-1', 'feature-2', 'feature-variant'],
'$feature/feature-1': true,
'$feature/feature-2': true,
'$feature/disabled-flag': false,
'$feature/feature-variant': 'variant',
$lib: 'posthog-node',
$lib_version: '1.2.3',
Expand Down Expand Up @@ -760,14 +766,12 @@ describe('PostHog Node.js', () => {

expect(Object.keys(posthog.distinctIdHasSentFlagCalls).length).toEqual(0)

// TODO: Check if this has a real world perf impact on sending events, i.e. adding local flag info
// via the promise slows down sending events
for (let i = 0; i < 20; i++) {
for (let i = 0; i < 100; i++) {
const distinctId = `some-distinct-id${i}`
await posthog.getFeatureFlag('beta-feature', distinctId)

jest.runOnlyPendingTimers()
await waitForPromises()
jest.runOnlyPendingTimers()

const batchEvents = getLastBatchEvents()
expect(batchEvents).toMatchObject([
Expand Down

0 comments on commit e791895

Please sign in to comment.