From ccf1faa65ee86a9607ad0997b98c51f58e89633f Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Tue, 26 Dec 2023 12:54:02 +0000 Subject: [PATCH 01/20] feat(flags): Add local props and flags to all calls --- posthog-node/src/posthog-node.ts | 79 ++++++++++- posthog-node/test/posthog-node.spec.ts | 177 ++++++++++++++++++++++++- 2 files changed, 249 insertions(+), 7 deletions(-) diff --git a/posthog-node/src/posthog-node.ts b/posthog-node/src/posthog-node.ts index a6a5520e..f3faee65 100644 --- a/posthog-node/src/posthog-node.ts +++ b/posthog-node/src/posthog-node.ts @@ -122,6 +122,26 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 { } _capture({ ...properties, $groups: groups, ...flagProperties }) }) + } else if (this.featureFlagsPoller?.featureFlags) { + const groupsWithStringValues: Record = {} + for (const [key, value] of Object.entries(groups || {})) { + groupsWithStringValues[key] = String(value) + } + this.getAllFlags(distinctId, { groups: groupsWithStringValues, disableGeoip, onlyEvaluateLocally: true }).then(flags => { + const featureVariantProperties: Record = {} + if (flags) { + for (const [feature, variant] of Object.entries(flags)) { + 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 }) + }) + } else { _capture({ ...properties, $groups: groups }) } @@ -156,8 +176,18 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 { disableGeoip?: boolean } ): Promise { - const { groups, personProperties, groupProperties, disableGeoip } = options || {} - let { onlyEvaluateLocally, sendFeatureFlagEvents } = options || {} + const { groups, disableGeoip } = options || {} + let { onlyEvaluateLocally, sendFeatureFlagEvents, personProperties, groupProperties } = options || {} + + const adjustedProperties = this.addLocalPersonAndGroupProperties( + distinctId, + groups, + personProperties, + groupProperties + ) + + personProperties = adjustedProperties.allPersonProperties + groupProperties = adjustedProperties.allGroupProperties // set defaults if (onlyEvaluateLocally == undefined) { @@ -232,8 +262,19 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 { disableGeoip?: boolean } ): Promise { - const { groups, personProperties, groupProperties, disableGeoip } = options || {} - let { onlyEvaluateLocally, sendFeatureFlagEvents } = options || {} + const { groups, disableGeoip } = options || {} + let { onlyEvaluateLocally, sendFeatureFlagEvents, personProperties, groupProperties } = options || {} + + const adjustedProperties = this.addLocalPersonAndGroupProperties( + distinctId, + groups, + personProperties, + groupProperties + ) + + personProperties = adjustedProperties.allPersonProperties + groupProperties = adjustedProperties.allGroupProperties + let response = undefined // Try to get match value locally if not provided @@ -324,8 +365,18 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 { disableGeoip?: boolean } ): Promise { - const { groups, personProperties, groupProperties, disableGeoip } = options || {} - let { onlyEvaluateLocally } = options || {} + const { groups, disableGeoip } = options || {} + let { onlyEvaluateLocally, personProperties, groupProperties } = options || {} + + const adjustedProperties = this.addLocalPersonAndGroupProperties( + distinctId, + groups, + personProperties, + groupProperties + ) + + personProperties = adjustedProperties.allPersonProperties + groupProperties = adjustedProperties.allGroupProperties // set defaults if (onlyEvaluateLocally == undefined) { @@ -385,4 +436,20 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 { this.featureFlagsPoller?.stopPoller() return super.shutdownAsync() } + + private addLocalPersonAndGroupProperties(distinctId: string, groups?: Record, personProperties?: Record, groupProperties?: Record>) { + const allPersonProperties = { $current_distinct_id: distinctId, ...(personProperties || {}) } + + const allGroupProperties: Record> = {} + if (groups) { + for (const groupName of Object.keys(groups)) { + allGroupProperties[groupName] = { + $group_key: groups[groupName], + ...(groupProperties?.[groupName] || {}), + } + } + } + + return { allPersonProperties, allGroupProperties } + } } diff --git a/posthog-node/test/posthog-node.spec.ts b/posthog-node/test/posthog-node.spec.ts index c65e0ce7..c4f7db98 100644 --- a/posthog-node/test/posthog-node.spec.ts +++ b/posthog-node/test/posthog-node.spec.ts @@ -384,8 +384,77 @@ describe('PostHog Node.js', () => { 'feature-variant': 2, } + const multivariateFlag = { + "id": 1, + "name": "Beta Feature", + "key": "beta-feature-local", + "is_simple_flag": false, + "active": true, + "rollout_percentage": 100, + "filters": { + "groups": [ + { + "properties": [ + {"key": "email", "type": "person", "value": "test@posthog.com", "operator": "exact"} + ], + "rollout_percentage": 100, + }, + { + "rollout_percentage": 50, + }, + ], + "multivariate": { + "variants": [ + {"key": "first-variant", "name": "First Variant", "rollout_percentage": 50}, + {"key": "second-variant", "name": "Second Variant", "rollout_percentage": 25}, + {"key": "third-variant", "name": "Third Variant", "rollout_percentage": 25}, + ] + }, + "payloads": {"first-variant": "some-payload", "third-variant": {"a": "json"}}, + }, + } + const basicFlag = { + "id": 1, + "name": "Beta Feature", + "key": "person-flag", + "is_simple_flag": true, + "active": true, + "filters": { + "groups": [ + { + "properties": [ + { + "key": "region", + "operator": "exact", + "value": ["USA"], + "type": "person", + } + ], + "rollout_percentage": 100, + } + ], + "payloads": {"true": 300}, + }, + } + const falseFlag = { + "id": 1, + "name": "Beta Feature", + "key": "false-flag", + "is_simple_flag": true, + "active": true, + "filters": { + "groups": [ + { + "properties": [], + "rollout_percentage": 0, + } + ], + "payloads": {"true": 300}, + }, + } + mockedFetch.mockImplementation( - apiImplementation({ decideFlags: mockFeatureFlags, decideFlagPayloads: mockFeatureFlagPayloads }) + apiImplementation({ decideFlags: mockFeatureFlags, decideFlagPayloads: mockFeatureFlagPayloads, localFlags: {flags: [multivariateFlag, basicFlag, falseFlag]} }) ) posthog = new PostHog('TEST_API_KEY', { @@ -464,6 +533,111 @@ describe('PostHog Node.js', () => { ) }) + it('captures feature flags with locally evaluated flags', async () => { + mockedFetch.mockClear() + mockedFetch.mockClear() + expect(mockedFetch).toHaveBeenCalledTimes(0) + + posthog = new PostHog('TEST_API_KEY', { + host: 'http://example.com', + flushAt: 1, + fetchRetryCount: 0, + personalApiKey: 'TEST_PERSONAL_API_KEY', + }) + + jest.runOnlyPendingTimers() + + await waitForPromises() + + posthog.capture({ + distinctId: 'distinct_id', + event: 'node test event', + }) + + expect(mockedFetch).toHaveBeenCalledWith(...anyLocalEvalCall) + // no decide call + expect(mockedFetch).not.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', + event: 'node test event', + properties: expect.objectContaining({ + $active_feature_flags: ['beta-feature-local'], + '$feature/beta-feature-local': 'third-variant', + '$feature/false-flag': false, + $lib: 'posthog-node', + $lib_version: '1.2.3', + $geoip_disable: true, + }), + }) + ) + expect(getLastBatchEvents()?.[0].properties.hasOwnProperty('$feature/beta-feature-local')).toBe(true) + expect(getLastBatchEvents()?.[0].properties.hasOwnProperty('$feature/beta-feature')).toBe(false) + + await posthog.shutdownAsync() + + }) + + it('doesnt add flag properties when locally evaluated flags are empty', async () => { + mockedFetch.mockClear() + expect(mockedFetch).toHaveBeenCalledTimes(0) + mockedFetch.mockImplementation( + apiImplementation({ decideFlags: {'a': false, 'b': 'true'}, decideFlagPayloads: {}, localFlags: {flags: []} }) + ) + + + posthog = new PostHog('TEST_API_KEY', { + host: 'http://example.com', + flushAt: 1, + fetchRetryCount: 0, + personalApiKey: 'TEST_PERSONAL_API_KEY', + }) + + jest.runOnlyPendingTimers() + + await waitForPromises() + + posthog.capture({ + distinctId: 'distinct_id', + event: 'node test event', + }) + + expect(mockedFetch).toHaveBeenCalledWith(...anyLocalEvalCall) + // no decide call + expect(mockedFetch).not.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', + event: 'node test event', + properties: expect.objectContaining({ + $lib: 'posthog-node', + $lib_version: '1.2.3', + $geoip_disable: true, + }), + }) + ) + expect(getLastBatchEvents()?.[0].properties.hasOwnProperty('$feature/beta-feature-local')).toBe(false) + expect(getLastBatchEvents()?.[0].properties.hasOwnProperty('$feature/beta-feature')).toBe(false) + + await posthog.shutdownAsync() + }) + it('captures feature flags with same geoip setting as capture', async () => { mockedFetch.mockClear() mockedFetch.mockClear() @@ -566,6 +740,7 @@ describe('PostHog Node.js', () => { }) it('$feature_flag_called is called appropriately when querying flags', async () => { + mockedFetch.mockClear() const flags = { flags: [ { From 262a30c5150aa32c84a0ae140a460ac039606550 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Tue, 26 Dec 2023 12:54:16 +0000 Subject: [PATCH 02/20] prettier --- posthog-node/src/posthog-node.ts | 34 ++++--- posthog-node/test/posthog-node.spec.ts | 130 ++++++++++++------------- 2 files changed, 85 insertions(+), 79 deletions(-) diff --git a/posthog-node/src/posthog-node.ts b/posthog-node/src/posthog-node.ts index f3faee65..c20fd8f4 100644 --- a/posthog-node/src/posthog-node.ts +++ b/posthog-node/src/posthog-node.ts @@ -127,21 +127,22 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 { for (const [key, value] of Object.entries(groups || {})) { groupsWithStringValues[key] = String(value) } - this.getAllFlags(distinctId, { groups: groupsWithStringValues, disableGeoip, onlyEvaluateLocally: true }).then(flags => { - const featureVariantProperties: Record = {} - if (flags) { - for (const [feature, variant] of Object.entries(flags)) { - featureVariantProperties[`$feature/${feature}`] = variant + this.getAllFlags(distinctId, { groups: groupsWithStringValues, disableGeoip, onlyEvaluateLocally: true }).then( + (flags) => { + const featureVariantProperties: Record = {} + if (flags) { + for (const [feature, variant] of Object.entries(flags)) { + 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 }) } - const activeFlags = Object.keys(flags || {}).filter((flag) => flags?.[flag] !== false) - const flagProperties = { - $active_feature_flags: activeFlags || undefined, - ...featureVariantProperties, - } - _capture({ ...properties, $groups: groups, ...flagProperties }) - }) - + ) } else { _capture({ ...properties, $groups: groups }) } @@ -437,7 +438,12 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 { return super.shutdownAsync() } - private addLocalPersonAndGroupProperties(distinctId: string, groups?: Record, personProperties?: Record, groupProperties?: Record>) { + private addLocalPersonAndGroupProperties( + distinctId: string, + groups?: Record, + personProperties?: Record, + groupProperties?: Record> + ) { const allPersonProperties = { $current_distinct_id: distinctId, ...(personProperties || {}) } const allGroupProperties: Record> = {} diff --git a/posthog-node/test/posthog-node.spec.ts b/posthog-node/test/posthog-node.spec.ts index c4f7db98..2d8db1eb 100644 --- a/posthog-node/test/posthog-node.spec.ts +++ b/posthog-node/test/posthog-node.spec.ts @@ -385,76 +385,78 @@ describe('PostHog Node.js', () => { } const multivariateFlag = { - "id": 1, - "name": "Beta Feature", - "key": "beta-feature-local", - "is_simple_flag": false, - "active": true, - "rollout_percentage": 100, - "filters": { - "groups": [ - { - "properties": [ - {"key": "email", "type": "person", "value": "test@posthog.com", "operator": "exact"} - ], - "rollout_percentage": 100, - }, - { - "rollout_percentage": 50, - }, - ], - "multivariate": { - "variants": [ - {"key": "first-variant", "name": "First Variant", "rollout_percentage": 50}, - {"key": "second-variant", "name": "Second Variant", "rollout_percentage": 25}, - {"key": "third-variant", "name": "Third Variant", "rollout_percentage": 25}, - ] + id: 1, + name: 'Beta Feature', + key: 'beta-feature-local', + is_simple_flag: false, + active: true, + rollout_percentage: 100, + filters: { + groups: [ + { + properties: [{ key: 'email', type: 'person', value: 'test@posthog.com', operator: 'exact' }], + rollout_percentage: 100, + }, + { + rollout_percentage: 50, }, - "payloads": {"first-variant": "some-payload", "third-variant": {"a": "json"}}, + ], + multivariate: { + variants: [ + { key: 'first-variant', name: 'First Variant', rollout_percentage: 50 }, + { key: 'second-variant', name: 'Second Variant', rollout_percentage: 25 }, + { key: 'third-variant', name: 'Third Variant', rollout_percentage: 25 }, + ], + }, + payloads: { 'first-variant': 'some-payload', 'third-variant': { a: 'json' } }, }, - } + } const basicFlag = { - "id": 1, - "name": "Beta Feature", - "key": "person-flag", - "is_simple_flag": true, - "active": true, - "filters": { - "groups": [ + id: 1, + name: 'Beta Feature', + key: 'person-flag', + is_simple_flag: true, + active: true, + filters: { + groups: [ + { + properties: [ { - "properties": [ - { - "key": "region", - "operator": "exact", - "value": ["USA"], - "type": "person", - } - ], - "rollout_percentage": 100, - } - ], - "payloads": {"true": 300}, + key: 'region', + operator: 'exact', + value: ['USA'], + type: 'person', + }, + ], + rollout_percentage: 100, + }, + ], + payloads: { true: 300 }, }, - } + } const falseFlag = { - "id": 1, - "name": "Beta Feature", - "key": "false-flag", - "is_simple_flag": true, - "active": true, - "filters": { - "groups": [ - { - "properties": [], - "rollout_percentage": 0, - } - ], - "payloads": {"true": 300}, + id: 1, + name: 'Beta Feature', + key: 'false-flag', + is_simple_flag: true, + active: true, + filters: { + groups: [ + { + properties: [], + rollout_percentage: 0, + }, + ], + payloads: { true: 300 }, }, - } - + } + mockedFetch.mockImplementation( - apiImplementation({ decideFlags: mockFeatureFlags, decideFlagPayloads: mockFeatureFlagPayloads, localFlags: {flags: [multivariateFlag, basicFlag, falseFlag]} }) + apiImplementation({ + decideFlags: mockFeatureFlags, + decideFlagPayloads: mockFeatureFlagPayloads, + localFlags: { flags: [multivariateFlag, basicFlag, falseFlag] }, + }) ) posthog = new PostHog('TEST_API_KEY', { @@ -583,17 +585,15 @@ describe('PostHog Node.js', () => { expect(getLastBatchEvents()?.[0].properties.hasOwnProperty('$feature/beta-feature')).toBe(false) await posthog.shutdownAsync() - }) it('doesnt add flag properties when locally evaluated flags are empty', async () => { mockedFetch.mockClear() expect(mockedFetch).toHaveBeenCalledTimes(0) mockedFetch.mockImplementation( - apiImplementation({ decideFlags: {'a': false, 'b': 'true'}, decideFlagPayloads: {}, localFlags: {flags: []} }) + apiImplementation({ decideFlags: { a: false, b: 'true' }, decideFlagPayloads: {}, localFlags: { flags: [] } }) ) - posthog = new PostHog('TEST_API_KEY', { host: 'http://example.com', flushAt: 1, From 2a2e2c0e3a8efc9cd21df8800c6d8c37b0f27a1b Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Tue, 26 Dec 2023 14:23:49 +0000 Subject: [PATCH 03/20] fix some other broken tests uncovered --- posthog-core/src/index.ts | 2 +- posthog-node/src/feature-flags.ts | 4 +-- posthog-node/src/posthog-node.ts | 11 +++++-- posthog-node/test/feature-flags.spec.ts | 19 ++++++++++-- posthog-node/test/posthog-node.spec.ts | 39 ++++++++++++++++++------- 5 files changed, 57 insertions(+), 18 deletions(-) diff --git a/posthog-core/src/index.ts b/posthog-core/src/index.ts index b1a80683..a8fe1b63 100644 --- a/posthog-core/src/index.ts +++ b/posthog-core/src/index.ts @@ -249,7 +249,7 @@ export abstract class PostHogCoreStateless { return this.fetchWithRetry(url, fetchOptions) .then((response) => response.json() as Promise) .catch((error) => { - console.error('Error fetching feature flags', error) + this._events.emit('error', error) return undefined }) } diff --git a/posthog-node/src/feature-flags.ts b/posthog-node/src/feature-flags.ts index b8ef8dc4..2e3e092b 100644 --- a/posthog-node/src/feature-flags.ts +++ b/posthog-node/src/feature-flags.ts @@ -121,7 +121,7 @@ class FeatureFlagsPoller { console.debug(`InconclusiveMatchError when computing flag locally: ${key}: ${e}`) } } else if (e instanceof Error) { - console.error(`Error computing flag locally: ${key}: ${e}`) + this.onError?.(new Error(`Error computing flag locally: ${key}: ${e}`)) } } } @@ -383,7 +383,7 @@ class FeatureFlagsPoller { const responseJson = await res.json() if (!('flags' in responseJson)) { - console.error(`Invalid response when getting feature flags: ${JSON.stringify(responseJson)}`) + this.onError?.(new Error(`Invalid response when getting feature flags: ${JSON.stringify(responseJson)}`)) } this.featureFlags = responseJson.flags || [] diff --git a/posthog-node/src/posthog-node.ts b/posthog-node/src/posthog-node.ts index c20fd8f4..901da95f 100644 --- a/posthog-node/src/posthog-node.ts +++ b/posthog-node/src/posthog-node.ts @@ -136,11 +136,16 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 { } } const activeFlags = Object.keys(flags || {}).filter((flag) => flags?.[flag] !== false) - const flagProperties = { - $active_feature_flags: activeFlags || undefined, + let flagProperties: Record = { ...featureVariantProperties, } - _capture({ ...properties, $groups: groups, ...flagProperties }) + if (activeFlags.length > 0) { + flagProperties = { + ...flagProperties, + $active_feature_flags: activeFlags, + } + } + _capture({ ...flagProperties, ...properties, $groups: groups }) } ) } else { diff --git a/posthog-node/test/feature-flags.spec.ts b/posthog-node/test/feature-flags.spec.ts index cb3eb132..5a5a619e 100644 --- a/posthog-node/test/feature-flags.spec.ts +++ b/posthog-node/test/feature-flags.spec.ts @@ -50,6 +50,17 @@ export const apiImplementation = ({ }) as any } + if ((url as any).includes('batch/')) { + return Promise.resolve({ + status: 200, + text: () => Promise.resolve('ok'), + json: () => + Promise.resolve({ + status: 'ok', + }), + }) as any + } + return Promise.resolve({ status: 400, text: () => Promise.resolve('ok'), @@ -342,7 +353,11 @@ describe('local evaluation', () => { token: 'TEST_API_KEY', distinct_id: 'some-distinct-id_outside_rollout?', groups: {}, - person_properties: { region: 'USA', email: 'a@b.com' }, + person_properties: { + $current_distinct_id: 'some-distinct-id_outside_rollout?', + region: 'USA', + email: 'a@b.com', + }, group_properties: {}, geoip_disable: true, }), @@ -361,7 +376,7 @@ describe('local evaluation', () => { token: 'TEST_API_KEY', distinct_id: 'some-distinct-id', groups: {}, - person_properties: { doesnt_matter: '1' }, + person_properties: { $current_distinct_id: 'some-distinct-id', doesnt_matter: '1' }, group_properties: {}, geoip_disable: true, }), diff --git a/posthog-node/test/posthog-node.spec.ts b/posthog-node/test/posthog-node.spec.ts index 2d8db1eb..40ca60c1 100644 --- a/posthog-node/test/posthog-node.spec.ts +++ b/posthog-node/test/posthog-node.spec.ts @@ -43,7 +43,7 @@ describe('PostHog Node.js', () => { afterEach(async () => { // ensure clean shutdown & no test interdependencies - await posthog.shutdownAsync() + // await posthog.shutdownAsync() }) describe('core methods', () => { @@ -455,7 +455,7 @@ describe('PostHog Node.js', () => { apiImplementation({ decideFlags: mockFeatureFlags, decideFlagPayloads: mockFeatureFlagPayloads, - localFlags: { flags: [multivariateFlag, basicFlag, falseFlag] }, + // localFlags: { flags: [multivariateFlag, basicFlag, falseFlag] }, }) ) @@ -484,7 +484,7 @@ describe('PostHog Node.js', () => { expect(mockedFetch).toHaveBeenCalledTimes(2) }) - it('captures feature flags when no personal API key is present', async () => { + it.skip('captures feature flags when no personal API key is present', async () => { mockedFetch.mockClear() mockedFetch.mockClear() expect(mockedFetch).toHaveBeenCalledTimes(0) @@ -535,12 +535,12 @@ describe('PostHog Node.js', () => { ) }) - it('captures feature flags with locally evaluated flags', async () => { + it.skip('captures feature flags with locally evaluated flags', async () => { mockedFetch.mockClear() mockedFetch.mockClear() expect(mockedFetch).toHaveBeenCalledTimes(0) - posthog = new PostHog('TEST_API_KEY', { + posthog = new PostHog('TEST_API_KEY_LOCAL_FLAGS', { host: 'http://example.com', flushAt: 1, fetchRetryCount: 0, @@ -587,14 +587,14 @@ describe('PostHog Node.js', () => { await posthog.shutdownAsync() }) - it('doesnt add flag properties when locally evaluated flags are empty', async () => { + it.skip('doesnt add flag properties when locally evaluated flags are empty', async () => { mockedFetch.mockClear() expect(mockedFetch).toHaveBeenCalledTimes(0) mockedFetch.mockImplementation( apiImplementation({ decideFlags: { a: false, b: 'true' }, decideFlagPayloads: {}, localFlags: { flags: [] } }) ) - posthog = new PostHog('TEST_API_KEY', { + posthog = new PostHog('TEST_API_KEY_EMPTY_LOCAL_FLAGS', { host: 'http://example.com', flushAt: 1, fetchRetryCount: 0, @@ -679,7 +679,7 @@ describe('PostHog Node.js', () => { expect(mockedFetch).not.toHaveBeenCalledWith(...anyLocalEvalCall) }) - it('manages memory well when sending feature flags', async () => { + it.skip('manages memory well when sending feature flags', async () => { const flags = { flags: [ { @@ -703,7 +703,7 @@ describe('PostHog Node.js', () => { apiImplementation({ localFlags: flags, decideFlags: { 'beta-feature': 'decide-fallback-value' } }) ) - posthog = new PostHog('TEST_API_KEY', { + posthog = new PostHog('TEST_API_KEY_MEMORY', { host: 'http://example.com', personalApiKey: 'TEST_PERSONAL_API_KEY', maxCacheSize: 10, @@ -741,6 +741,7 @@ describe('PostHog Node.js', () => { it('$feature_flag_called is called appropriately when querying flags', async () => { mockedFetch.mockClear() + const flags = { flags: [ { @@ -766,17 +767,25 @@ describe('PostHog Node.js', () => { posthog = new PostHog('TEST_API_KEY', { host: 'http://example.com', - personalApiKey: 'TEST_PERSONAL_API_KEY', + personalApiKey: 'TEST_PERSONAL_API_KEY_FFC', maxCacheSize: 10, fetchRetryCount: 0, + featureFlagsPollingInterval: 20000, }) + jest.runOnlyPendingTimers() + expect( await posthog.getFeatureFlag('beta-feature', 'some-distinct-id', { personProperties: { region: 'USA', name: 'Aloha' }, }) ).toEqual(true) + + // 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.flushAsync() + expect(mockedFetch).toHaveBeenCalledWith('http://example.com/batch/', expect.any(Object)) expect(getLastBatchEvents()?.[0]).toEqual( @@ -803,6 +812,8 @@ describe('PostHog Node.js', () => { }) ).toEqual(true) jest.runOnlyPendingTimers() + await waitForPromises() + await posthog.flushAsync() expect(mockedFetch).not.toHaveBeenCalledWith('http://example.com/batch/', expect.any(Object)) @@ -815,6 +826,8 @@ describe('PostHog Node.js', () => { }) ).toEqual(true) jest.runOnlyPendingTimers() + await waitForPromises() + await posthog.flushAsync() expect(mockedFetch).toHaveBeenCalledWith('http://example.com/batch/', expect.any(Object)) expect(getLastBatchEvents()?.[0]).toEqual( @@ -842,6 +855,8 @@ describe('PostHog Node.js', () => { }) ).toEqual(true) jest.runOnlyPendingTimers() + await waitForPromises() + await posthog.flushAsync() expect(mockedFetch).not.toHaveBeenCalledWith('http://example.com/batch/', expect.any(Object)) // # called for different flag, falls back to decide, should call capture again @@ -852,6 +867,8 @@ describe('PostHog Node.js', () => { }) ).toEqual('decide-value') jest.runOnlyPendingTimers() + await waitForPromises() + await posthog.flushAsync() // one to decide, one to batch expect(mockedFetch).toHaveBeenCalledWith(...anyDecideCall) expect(mockedFetch).toHaveBeenCalledWith('http://example.com/batch/', expect.any(Object)) @@ -880,6 +897,8 @@ describe('PostHog Node.js', () => { }) ).toEqual(true) jest.runOnlyPendingTimers() + await waitForPromises() + await posthog.flushAsync() // call decide, but not batch expect(mockedFetch).toHaveBeenCalledWith(...anyDecideCall) expect(mockedFetch).not.toHaveBeenCalledWith('http://example.com/batch/', expect.any(Object)) From 7fb1382708cbcb748f1b2e77cec66e2368a3d4d6 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Tue, 26 Dec 2023 14:27:19 +0000 Subject: [PATCH 04/20] lint --- posthog-node/src/posthog-node.ts | 2 +- posthog-node/test/posthog-node.spec.ts | 16 ++++++++++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/posthog-node/src/posthog-node.ts b/posthog-node/src/posthog-node.ts index 901da95f..6ef08574 100644 --- a/posthog-node/src/posthog-node.ts +++ b/posthog-node/src/posthog-node.ts @@ -448,7 +448,7 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 { groups?: Record, personProperties?: Record, groupProperties?: Record> - ) { + ): { allPersonProperties: Record; allGroupProperties: Record> } { const allPersonProperties = { $current_distinct_id: distinctId, ...(personProperties || {}) } const allGroupProperties: Record> = {} diff --git a/posthog-node/test/posthog-node.spec.ts b/posthog-node/test/posthog-node.spec.ts index 40ca60c1..a831840c 100644 --- a/posthog-node/test/posthog-node.spec.ts +++ b/posthog-node/test/posthog-node.spec.ts @@ -581,8 +581,12 @@ describe('PostHog Node.js', () => { }), }) ) - expect(getLastBatchEvents()?.[0].properties.hasOwnProperty('$feature/beta-feature-local')).toBe(true) - expect(getLastBatchEvents()?.[0].properties.hasOwnProperty('$feature/beta-feature')).toBe(false) + expect( + Object.prototype.hasOwnProperty.call(getLastBatchEvents()?.[0].properties, '$feature/beta-feature-local') + ).toBe(true) + expect(Object.prototype.hasOwnProperty.call(getLastBatchEvents()?.[0].properties, '$feature/beta-feature')).toBe( + false + ) await posthog.shutdownAsync() }) @@ -632,8 +636,12 @@ describe('PostHog Node.js', () => { }), }) ) - expect(getLastBatchEvents()?.[0].properties.hasOwnProperty('$feature/beta-feature-local')).toBe(false) - expect(getLastBatchEvents()?.[0].properties.hasOwnProperty('$feature/beta-feature')).toBe(false) + expect( + Object.prototype.hasOwnProperty.call(getLastBatchEvents()?.[0].properties, '$feature/beta-feature-local') + ).toBe(false) + expect(Object.prototype.hasOwnProperty.call(getLastBatchEvents()?.[0].properties, '$feature/beta-feature')).toBe( + false + ) await posthog.shutdownAsync() }) From 9b2acd502242076e662093b582989c10977ec2cd Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Tue, 26 Dec 2023 14:29:08 +0000 Subject: [PATCH 05/20] lint --- posthog-node/test/posthog-node.spec.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/posthog-node/test/posthog-node.spec.ts b/posthog-node/test/posthog-node.spec.ts index a831840c..4a14cbf4 100644 --- a/posthog-node/test/posthog-node.spec.ts +++ b/posthog-node/test/posthog-node.spec.ts @@ -43,7 +43,7 @@ describe('PostHog Node.js', () => { afterEach(async () => { // ensure clean shutdown & no test interdependencies - // await posthog.shutdownAsync() + await posthog.shutdownAsync() }) describe('core methods', () => { @@ -642,8 +642,6 @@ describe('PostHog Node.js', () => { expect(Object.prototype.hasOwnProperty.call(getLastBatchEvents()?.[0].properties, '$feature/beta-feature')).toBe( false ) - - await posthog.shutdownAsync() }) it('captures feature flags with same geoip setting as capture', async () => { From e5e271d7a234a30c5e30cce942574bb4089f78b5 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Tue, 26 Dec 2023 14:30:07 +0000 Subject: [PATCH 06/20] lint --- posthog-node/test/posthog-node.spec.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/posthog-node/test/posthog-node.spec.ts b/posthog-node/test/posthog-node.spec.ts index 4a14cbf4..029e8d45 100644 --- a/posthog-node/test/posthog-node.spec.ts +++ b/posthog-node/test/posthog-node.spec.ts @@ -455,7 +455,7 @@ describe('PostHog Node.js', () => { apiImplementation({ decideFlags: mockFeatureFlags, decideFlagPayloads: mockFeatureFlagPayloads, - // localFlags: { flags: [multivariateFlag, basicFlag, falseFlag] }, + localFlags: { flags: [multivariateFlag, basicFlag, falseFlag] }, }) ) @@ -484,7 +484,7 @@ describe('PostHog Node.js', () => { expect(mockedFetch).toHaveBeenCalledTimes(2) }) - it.skip('captures feature flags when no personal API key is present', async () => { + it('captures feature flags when no personal API key is present', async () => { mockedFetch.mockClear() mockedFetch.mockClear() expect(mockedFetch).toHaveBeenCalledTimes(0) @@ -535,7 +535,7 @@ describe('PostHog Node.js', () => { ) }) - it.skip('captures feature flags with locally evaluated flags', async () => { + it('captures feature flags with locally evaluated flags', async () => { mockedFetch.mockClear() mockedFetch.mockClear() expect(mockedFetch).toHaveBeenCalledTimes(0) @@ -591,7 +591,7 @@ describe('PostHog Node.js', () => { await posthog.shutdownAsync() }) - it.skip('doesnt add flag properties when locally evaluated flags are empty', async () => { + it('doesnt add flag properties when locally evaluated flags are empty', async () => { mockedFetch.mockClear() expect(mockedFetch).toHaveBeenCalledTimes(0) mockedFetch.mockImplementation( @@ -685,7 +685,7 @@ describe('PostHog Node.js', () => { expect(mockedFetch).not.toHaveBeenCalledWith(...anyLocalEvalCall) }) - it.skip('manages memory well when sending feature flags', async () => { + it('manages memory well when sending feature flags', async () => { const flags = { flags: [ { From 34506bc862239b1041da98c7a4a36db74e82f523 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Tue, 26 Dec 2023 14:38:16 +0000 Subject: [PATCH 07/20] fix --- posthog-node/test/posthog-node.spec.ts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/posthog-node/test/posthog-node.spec.ts b/posthog-node/test/posthog-node.spec.ts index 029e8d45..3f9184bd 100644 --- a/posthog-node/test/posthog-node.spec.ts +++ b/posthog-node/test/posthog-node.spec.ts @@ -540,7 +540,7 @@ describe('PostHog Node.js', () => { mockedFetch.mockClear() expect(mockedFetch).toHaveBeenCalledTimes(0) - posthog = new PostHog('TEST_API_KEY_LOCAL_FLAGS', { + posthog = new PostHog('TEST_API_KEY', { host: 'http://example.com', flushAt: 1, fetchRetryCount: 0, @@ -598,7 +598,7 @@ describe('PostHog Node.js', () => { apiImplementation({ decideFlags: { a: false, b: 'true' }, decideFlagPayloads: {}, localFlags: { flags: [] } }) ) - posthog = new PostHog('TEST_API_KEY_EMPTY_LOCAL_FLAGS', { + posthog = new PostHog('TEST_API_KEY', { host: 'http://example.com', flushAt: 1, fetchRetryCount: 0, @@ -709,20 +709,24 @@ describe('PostHog Node.js', () => { apiImplementation({ localFlags: flags, decideFlags: { 'beta-feature': 'decide-fallback-value' } }) ) - posthog = new PostHog('TEST_API_KEY_MEMORY', { + posthog = new PostHog('TEST_API_KEY', { host: 'http://example.com', personalApiKey: 'TEST_PERSONAL_API_KEY', maxCacheSize: 10, fetchRetryCount: 0, + flushAt: 1, }) expect(Object.keys(posthog.distinctIdHasSentFlagCalls).length).toEqual(0) - for (let i = 0; i < 1000; i++) { + // 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++) { const distinctId = `some-distinct-id${i}` await posthog.getFeatureFlag('beta-feature', distinctId) jest.runOnlyPendingTimers() + await waitForPromises() const batchEvents = getLastBatchEvents() expect(batchEvents).toMatchObject([ From 39e868afaecf9163635241e0c8b9ca2eee2d690f Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Thu, 4 Jan 2024 12:47:41 +0000 Subject: [PATCH 08/20] add test --- posthog-node/test/posthog-node.spec.ts | 152 ++++++++++++++++++++++++- 1 file changed, 150 insertions(+), 2 deletions(-) diff --git a/posthog-node/test/posthog-node.spec.ts b/posthog-node/test/posthog-node.spec.ts index 3f9184bd..f7492395 100644 --- a/posthog-node/test/posthog-node.spec.ts +++ b/posthog-node/test/posthog-node.spec.ts @@ -4,6 +4,7 @@ jest.mock('../src/fetch') import fetch from '../src/fetch' import { anyDecideCall, anyLocalEvalCall, apiImplementation } from './feature-flags.spec' import { waitForPromises, wait } from '../../posthog-core/test/test-utils/test-utils' +import exp from 'constants' jest.mock('../package.json', () => ({ version: '1.2.3' })) @@ -777,10 +778,9 @@ describe('PostHog Node.js', () => { posthog = new PostHog('TEST_API_KEY', { host: 'http://example.com', - personalApiKey: 'TEST_PERSONAL_API_KEY_FFC', + personalApiKey: 'TEST_PERSONAL_API_KEY', maxCacheSize: 10, fetchRetryCount: 0, - featureFlagsPollingInterval: 20000, }) jest.runOnlyPendingTimers() @@ -954,5 +954,153 @@ describe('PostHog Node.js', () => { expect.objectContaining({ method: 'POST', body: expect.not.stringContaining('geoip_disable') }) ) }) + + it('should add default person & group properties for feature flags', async () => { + await posthog.getFeatureFlag('random_key', 'some_id', { + groups: { company: 'id:5', instance: 'app.posthog.com' }, + personProperties: { x1: 'y1' }, + groupProperties: { company: { x: 'y' } }, + }) + jest.runOnlyPendingTimers() + + expect(mockedFetch).toHaveBeenCalledWith( + 'http://example.com/decide/?v=3', + expect.objectContaining({ + body: JSON.stringify({ + token: 'TEST_API_KEY', + distinct_id: 'some_id', + groups: { company: 'id:5', instance: 'app.posthog.com' }, + person_properties: { + $current_distinct_id: 'some_id', + x1: 'y1', + }, + group_properties: { + company: { $group_key: 'id:5', x: 'y' }, + instance: { $group_key: 'app.posthog.com' }, + }, + geoip_disable: true, + }), + }) + ) + + mockedFetch.mockClear() + + await posthog.getFeatureFlag('random_key', 'some_id', { + groups: {company: 'id:5', instance: 'app.posthog.com'}, + personProperties: { $current_distinct_id: 'override' }, + groupProperties: { company: { $group_key: 'group_override' } }, + }) + jest.runOnlyPendingTimers() + + expect(mockedFetch).toHaveBeenCalledWith( + 'http://example.com/decide/?v=3', + expect.objectContaining({ + body: JSON.stringify({ + token: 'TEST_API_KEY', + distinct_id: 'some_id', + groups: { company: 'id:5', instance: 'app.posthog.com' }, + person_properties: { + $current_distinct_id: 'override', + }, + group_properties: { + company: { $group_key: 'group_override' }, + instance: { $group_key: 'app.posthog.com' }, + }, + geoip_disable: true, + }) + })) + + mockedFetch.mockClear() + + // test nones + await posthog.getAllFlagsAndPayloads('some_id', { + groups: undefined, + personProperties: undefined, + groupProperties: undefined, + }) + + jest.runOnlyPendingTimers() + + expect(mockedFetch).toHaveBeenCalledWith( + 'http://example.com/decide/?v=3', + expect.objectContaining({ + body: JSON.stringify({ + token: 'TEST_API_KEY', + distinct_id: 'some_id', + groups: {}, + person_properties: { + $current_distinct_id: 'some_id', + }, + group_properties: {}, + geoip_disable: true, + }), + }) + ) + + mockedFetch.mockClear() + await posthog.getAllFlags('some_id', { + groups: { company: 'id:5'}, + personProperties: undefined, + groupProperties: undefined, + }) + jest.runOnlyPendingTimers() + + expect(mockedFetch).toHaveBeenCalledWith( + 'http://example.com/decide/?v=3', + expect.objectContaining({ + body: JSON.stringify({ + token: 'TEST_API_KEY', + distinct_id: 'some_id', + groups: { company: 'id:5'}, + person_properties: { + $current_distinct_id: 'some_id', + }, + group_properties: { company: { $group_key: 'id:5' }}, + geoip_disable: true + }), + }) + ) + + mockedFetch.mockClear() + await posthog.getFeatureFlagPayload('random_key', 'some_id', undefined) + jest.runOnlyPendingTimers() + + expect(mockedFetch).toHaveBeenCalledWith( + 'http://example.com/decide/?v=3', + expect.objectContaining({ + body: JSON.stringify({ + token: 'TEST_API_KEY', + distinct_id: 'some_id', + groups: {}, + person_properties: { + $current_distinct_id: 'some_id', + }, + group_properties: {}, + geoip_disable: true, + }), + }) + ) + + mockedFetch.mockClear() + + await posthog.isFeatureEnabled('random_key', 'some_id') + jest.runOnlyPendingTimers() + + expect(mockedFetch).toHaveBeenCalledWith( + 'http://example.com/decide/?v=3', + expect.objectContaining({ + body: JSON.stringify({ + token: 'TEST_API_KEY', + distinct_id: 'some_id', + groups: {}, + person_properties: { + $current_distinct_id: 'some_id', + }, + group_properties: {}, + geoip_disable: true, + }), + }) + ) + }) }) }) From bcce85b2862e89d34a7c3fb9abdc5a229b375686 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Thu, 4 Jan 2024 12:52:50 +0000 Subject: [PATCH 09/20] lint --- posthog-node/test/posthog-node.spec.ts | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/posthog-node/test/posthog-node.spec.ts b/posthog-node/test/posthog-node.spec.ts index f7492395..df9bda38 100644 --- a/posthog-node/test/posthog-node.spec.ts +++ b/posthog-node/test/posthog-node.spec.ts @@ -960,7 +960,7 @@ describe('PostHog Node.js', () => { groups: { company: 'id:5', instance: 'app.posthog.com' }, personProperties: { x1: 'y1' }, groupProperties: { company: { x: 'y' } }, - }) + }) jest.runOnlyPendingTimers() expect(mockedFetch).toHaveBeenCalledWith( @@ -986,7 +986,7 @@ describe('PostHog Node.js', () => { mockedFetch.mockClear() await posthog.getFeatureFlag('random_key', 'some_id', { - groups: {company: 'id:5', instance: 'app.posthog.com'}, + groups: { company: 'id:5', instance: 'app.posthog.com' }, personProperties: { $current_distinct_id: 'override' }, groupProperties: { company: { $group_key: 'group_override' } }, }) @@ -1007,8 +1007,9 @@ describe('PostHog Node.js', () => { instance: { $group_key: 'app.posthog.com' }, }, geoip_disable: true, + }), }) - })) + ) mockedFetch.mockClear() @@ -1039,7 +1040,7 @@ describe('PostHog Node.js', () => { mockedFetch.mockClear() await posthog.getAllFlags('some_id', { - groups: { company: 'id:5'}, + groups: { company: 'id:5' }, personProperties: undefined, groupProperties: undefined, }) @@ -1051,12 +1052,12 @@ describe('PostHog Node.js', () => { body: JSON.stringify({ token: 'TEST_API_KEY', distinct_id: 'some_id', - groups: { company: 'id:5'}, + groups: { company: 'id:5' }, person_properties: { $current_distinct_id: 'some_id', }, - group_properties: { company: { $group_key: 'id:5' }}, - geoip_disable: true + group_properties: { company: { $group_key: 'id:5' } }, + geoip_disable: true, }), }) ) From e91f2a9ed2aec3dc1dc03bec1ff8024000877ce3 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Thu, 4 Jan 2024 13:03:37 +0000 Subject: [PATCH 10/20] lint --- posthog-node/test/posthog-node.spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/posthog-node/test/posthog-node.spec.ts b/posthog-node/test/posthog-node.spec.ts index df9bda38..bb98806f 100644 --- a/posthog-node/test/posthog-node.spec.ts +++ b/posthog-node/test/posthog-node.spec.ts @@ -4,7 +4,6 @@ jest.mock('../src/fetch') import fetch from '../src/fetch' import { anyDecideCall, anyLocalEvalCall, apiImplementation } from './feature-flags.spec' import { waitForPromises, wait } from '../../posthog-core/test/test-utils/test-utils' -import exp from 'constants' jest.mock('../package.json', () => ({ version: '1.2.3' })) From 06415fa6f3b221745a1e18275613cb019cc69bc3 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Thu, 4 Jan 2024 14:42:38 +0000 Subject: [PATCH 11/20] make sure events are not lost with sendFeatureFlags --- posthog-core/src/index.ts | 6 +++- posthog-node/src/posthog-node.ts | 23 +++++++++++--- posthog-node/test/posthog-node.spec.ts | 43 ++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 5 deletions(-) diff --git a/posthog-core/src/index.ts b/posthog-core/src/index.ts index a8fe1b63..503de7c0 100644 --- a/posthog-core/src/index.ts +++ b/posthog-core/src/index.ts @@ -56,7 +56,6 @@ export abstract class PostHogCoreStateless { private captureMode: 'form' | 'json' private removeDebugCallback?: () => void private debugMode: boolean = false - private pendingPromises: Record> = {} private disableGeoip: boolean = true private _optoutOverride: boolean | undefined @@ -65,6 +64,7 @@ export abstract class PostHogCoreStateless { protected _events = new SimpleEventEmitter() protected _flushTimer?: any protected _retryOptions: RetriableOptions + protected pendingPromises: Record> = {} // Abstract methods to be overridden by implementations abstract fetch(url: string, options: PostHogFetchOptions): Promise @@ -565,6 +565,10 @@ export abstract class PostHogCoreStateless { }) ) ) + // flush again to make sure we send all events, some of which might've been added + // while we were waiting for the pending promises to resolve + // For example, see sendFeatureFlags in posthog-node/src/posthog-node.ts::capture + await this.flushAsync() } catch (e) { if (!isPostHogFetchError(e)) { throw e diff --git a/posthog-node/src/posthog-node.ts b/posthog-node/src/posthog-node.ts index 6ef08574..042e2ded 100644 --- a/posthog-node/src/posthog-node.ts +++ b/posthog-node/src/posthog-node.ts @@ -13,6 +13,7 @@ import { PostHogMemoryStorage } from '../../posthog-core/src/storage-memory' import { EventMessageV1, GroupIdentifyMessage, IdentifyMessageV1, PostHogNodeV1 } from './types' import { FeatureFlagsPoller } from './feature-flags' import fetch from './fetch' +import { generateUUID } from 'posthog-core/src/utils' export type PostHogOptions = PosthogCoreOptions & { persistence?: 'memory' @@ -106,7 +107,12 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 { } if (sendFeatureFlags) { - super.getFeatureFlagsStateless(distinctId, groups, undefined, undefined, disableGeoip).then((flags) => { + const promiseUUID = generateUUID() + // :TRICKY: If we flush, or need to shut down, to not lose events we want this promise to resolve before we flush + const promise = super.getFeatureFlagsStateless(distinctId, groups, undefined, undefined, disableGeoip) + this.pendingPromises[promiseUUID] = promise + + promise.then((flags) => { const featureVariantProperties: Record = {} if (flags) { for (const [feature, variant] of Object.entries(flags)) { @@ -121,13 +127,20 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 { ...featureVariantProperties, } _capture({ ...properties, $groups: groups, ...flagProperties }) + }).catch(() => { + _capture({ ...properties, $groups: groups }) }) - } else if (this.featureFlagsPoller?.featureFlags) { + } else if ((this.featureFlagsPoller?.featureFlags?.length || 0) > 0) { const groupsWithStringValues: Record = {} for (const [key, value] of Object.entries(groups || {})) { groupsWithStringValues[key] = String(value) } - this.getAllFlags(distinctId, { groups: groupsWithStringValues, disableGeoip, onlyEvaluateLocally: true }).then( + + const promiseUUID = generateUUID() + // :TRICKY: If we flush, or need to shut down, to not lose events we want this promise to resolve before we flush + const promise = this.getAllFlags(distinctId, { groups: groupsWithStringValues, disableGeoip, onlyEvaluateLocally: true }) + this.pendingPromises[promiseUUID] = promise + promise.then( (flags) => { const featureVariantProperties: Record = {} if (flags) { @@ -147,7 +160,9 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 { } _capture({ ...flagProperties, ...properties, $groups: groups }) } - ) + ).catch(() => { + _capture({ ...properties, $groups: groups }) + }) } else { _capture({ ...properties, $groups: groups }) } diff --git a/posthog-node/test/posthog-node.spec.ts b/posthog-node/test/posthog-node.spec.ts index bb98806f..a636b40f 100644 --- a/posthog-node/test/posthog-node.spec.ts +++ b/posthog-node/test/posthog-node.spec.ts @@ -286,6 +286,7 @@ describe('PostHog Node.js', () => { afterEach(() => { posthog.debug(false) + jest.useFakeTimers() }) it('should shutdown cleanly', async () => { @@ -321,6 +322,48 @@ describe('PostHog Node.js', () => { jest.useFakeTimers() logSpy.mockRestore() }) + + it('should shutdown cleanly with pending capture flag promises', async () => { + posthog = new PostHog('TEST_API_KEY', { + host: 'http://example.com', + fetchRetryCount: 0, + flushAt: 4, + }) + + const logSpy = jest.spyOn(console, 'log').mockImplementation(() => {}) + jest.useRealTimers() + posthog.debug(true) + + for (let i = 0; i < 10; i++) { + posthog.capture({ event: 'test-event', distinctId: `${i}`, sendFeatureFlags: true }) + } + + await posthog.shutdownAsync() + // all capture calls happen during shutdown + const batchEvents = getLastBatchEvents() + expect(batchEvents?.length).toEqual(2) + expect(batchEvents?.[batchEvents?.length - 1]).toMatchObject( + { + // last event in batch + distinct_id: '9', + event: 'test-event', + library: 'posthog-node', + library_version: '1.2.3', + properties: { + $lib: 'posthog-node', + $lib_version: '1.2.3', + $geoip_disable: true, + $active_feature_flags: [], + }, + timestamp: expect.any(String), + type: 'capture', + }, + ) + expect(10).toEqual(logSpy.mock.calls.filter((call) => call[1].includes('capture')).length) + expect(3).toEqual(logSpy.mock.calls.filter((call) => call[1].includes('flush')).length) + jest.useFakeTimers() + logSpy.mockRestore() + }) }) describe('groupIdentify', () => { From 55ef845b9ae0a5a1ed18b70e063e4b99eb8051de Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Thu, 4 Jan 2024 14:47:19 +0000 Subject: [PATCH 12/20] lint --- posthog-node/src/posthog-node.ts | 52 ++++++++++++++------------ posthog-node/test/posthog-node.spec.ts | 30 +++++++-------- 2 files changed, 43 insertions(+), 39 deletions(-) diff --git a/posthog-node/src/posthog-node.ts b/posthog-node/src/posthog-node.ts index 042e2ded..220a2e31 100644 --- a/posthog-node/src/posthog-node.ts +++ b/posthog-node/src/posthog-node.ts @@ -112,24 +112,26 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 { const promise = super.getFeatureFlagsStateless(distinctId, groups, undefined, undefined, disableGeoip) this.pendingPromises[promiseUUID] = promise - promise.then((flags) => { - const featureVariantProperties: Record = {} - if (flags) { - for (const [feature, variant] of Object.entries(flags)) { - if (variant !== false) { - featureVariantProperties[`$feature/${feature}`] = variant + promise + .then((flags) => { + const featureVariantProperties: Record = {} + 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 }) - }) + 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 = {} for (const [key, value] of Object.entries(groups || {})) { @@ -138,10 +140,14 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 { const promiseUUID = generateUUID() // :TRICKY: If we flush, or need to shut down, to not lose events we want this promise to resolve before we flush - const promise = this.getAllFlags(distinctId, { groups: groupsWithStringValues, disableGeoip, onlyEvaluateLocally: true }) + const promise = this.getAllFlags(distinctId, { + groups: groupsWithStringValues, + disableGeoip, + onlyEvaluateLocally: true, + }) this.pendingPromises[promiseUUID] = promise - promise.then( - (flags) => { + promise + .then((flags) => { const featureVariantProperties: Record = {} if (flags) { for (const [feature, variant] of Object.entries(flags)) { @@ -159,10 +165,10 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 { } } _capture({ ...flagProperties, ...properties, $groups: groups }) - } - ).catch(() => { - _capture({ ...properties, $groups: groups }) - }) + }) + .catch(() => { + _capture({ ...properties, $groups: groups }) + }) } else { _capture({ ...properties, $groups: groups }) } diff --git a/posthog-node/test/posthog-node.spec.ts b/posthog-node/test/posthog-node.spec.ts index a636b40f..0a0392a0 100644 --- a/posthog-node/test/posthog-node.spec.ts +++ b/posthog-node/test/posthog-node.spec.ts @@ -342,23 +342,21 @@ describe('PostHog Node.js', () => { // all capture calls happen during shutdown const batchEvents = getLastBatchEvents() expect(batchEvents?.length).toEqual(2) - expect(batchEvents?.[batchEvents?.length - 1]).toMatchObject( - { - // last event in batch - distinct_id: '9', - event: 'test-event', - library: 'posthog-node', - library_version: '1.2.3', - properties: { - $lib: 'posthog-node', - $lib_version: '1.2.3', - $geoip_disable: true, - $active_feature_flags: [], - }, - timestamp: expect.any(String), - type: 'capture', + expect(batchEvents?.[batchEvents?.length - 1]).toMatchObject({ + // last event in batch + distinct_id: '9', + event: 'test-event', + library: 'posthog-node', + library_version: '1.2.3', + properties: { + $lib: 'posthog-node', + $lib_version: '1.2.3', + $geoip_disable: true, + $active_feature_flags: [], }, - ) + timestamp: expect.any(String), + type: 'capture', + }) expect(10).toEqual(logSpy.mock.calls.filter((call) => call[1].includes('capture')).length) expect(3).toEqual(logSpy.mock.calls.filter((call) => call[1].includes('flush')).length) jest.useFakeTimers() From 8e8c666a0683ed361982395dfc9beb05e22cba27 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Thu, 4 Jan 2024 15:10:44 +0000 Subject: [PATCH 13/20] clean up promise --- posthog-node/src/posthog-node.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/posthog-node/src/posthog-node.ts b/posthog-node/src/posthog-node.ts index 220a2e31..7794b36f 100644 --- a/posthog-node/src/posthog-node.ts +++ b/posthog-node/src/posthog-node.ts @@ -132,6 +132,9 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 { .catch(() => { _capture({ ...properties, $groups: groups }) }) + .finally(() => { + delete this.pendingPromises[promiseUUID] + }) } else if ((this.featureFlagsPoller?.featureFlags?.length || 0) > 0) { const groupsWithStringValues: Record = {} for (const [key, value] of Object.entries(groups || {})) { @@ -169,6 +172,9 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 { .catch(() => { _capture({ ...properties, $groups: groups }) }) + .finally(() => { + delete this.pendingPromises[promiseUUID] + }) } else { _capture({ ...properties, $groups: groups }) } From e8e57c0ff221de2754f8759104005041a3f18ce7 Mon Sep 17 00:00:00 2001 From: Ben White Date: Fri, 5 Jan 2024 09:16:43 +0100 Subject: [PATCH 14/20] Simplify pending promise stuff --- posthog-core/src/index.ts | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/posthog-core/src/index.ts b/posthog-core/src/index.ts index 503de7c0..e9e027ec 100644 --- a/posthog-core/src/index.ts +++ b/posthog-core/src/index.ts @@ -59,12 +59,12 @@ export abstract class PostHogCoreStateless { private disableGeoip: boolean = true private _optoutOverride: boolean | undefined + private pendingPromises: Record> = {} // internal protected _events = new SimpleEventEmitter() protected _flushTimer?: any protected _retryOptions: RetriableOptions - protected pendingPromises: Record> = {} // Abstract methods to be overridden by implementations abstract fetch(url: string, options: PostHogFetchOptions): Promise @@ -141,6 +141,14 @@ export abstract class PostHogCoreStateless { } } + protected addPendingPromise(promise: Promise): void { + const promiseUUID = generateUUID() + this.pendingPromises[promiseUUID] = promise + promise.finally(() => { + delete this.pendingPromises[promiseUUID] + }) + } + /*** *** TRACKING ***/ @@ -476,8 +484,6 @@ export abstract class PostHogCoreStateless { this._events.emit('error', err) } callback?.(err, messages) - // remove promise from pendingPromises - delete this.pendingPromises[promiseUUID] this._events.emit('flush', messages) } @@ -513,7 +519,7 @@ export abstract class PostHogCoreStateless { body: payload, } const requestPromise = this.fetchWithRetry(url, fetchOptions) - this.pendingPromises[promiseUUID] = requestPromise + this.addPendingPromise(requestPromise) requestPromise .then(() => done()) From 63b38807c41e0be052cb59b13709032d24932b63 Mon Sep 17 00:00:00 2001 From: Ben White Date: Fri, 5 Jan 2024 09:17:38 +0100 Subject: [PATCH 15/20] Fix up --- posthog-node/src/posthog-node.ts | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/posthog-node/src/posthog-node.ts b/posthog-node/src/posthog-node.ts index 7794b36f..2d3cbcb1 100644 --- a/posthog-node/src/posthog-node.ts +++ b/posthog-node/src/posthog-node.ts @@ -13,7 +13,6 @@ import { PostHogMemoryStorage } from '../../posthog-core/src/storage-memory' import { EventMessageV1, GroupIdentifyMessage, IdentifyMessageV1, PostHogNodeV1 } from './types' import { FeatureFlagsPoller } from './feature-flags' import fetch from './fetch' -import { generateUUID } from 'posthog-core/src/utils' export type PostHogOptions = PosthogCoreOptions & { persistence?: 'memory' @@ -107,10 +106,9 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 { } if (sendFeatureFlags) { - const promiseUUID = generateUUID() // :TRICKY: If we flush, or need to shut down, to not lose events we want this promise to resolve before we flush const promise = super.getFeatureFlagsStateless(distinctId, groups, undefined, undefined, disableGeoip) - this.pendingPromises[promiseUUID] = promise + this.addPendingPromise(promise) promise .then((flags) => { @@ -132,23 +130,19 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 { .catch(() => { _capture({ ...properties, $groups: groups }) }) - .finally(() => { - delete this.pendingPromises[promiseUUID] - }) } else if ((this.featureFlagsPoller?.featureFlags?.length || 0) > 0) { const groupsWithStringValues: Record = {} for (const [key, value] of Object.entries(groups || {})) { groupsWithStringValues[key] = String(value) } - const promiseUUID = generateUUID() // :TRICKY: If we flush, or need to shut down, to not lose events we want this promise to resolve before we flush const promise = this.getAllFlags(distinctId, { groups: groupsWithStringValues, disableGeoip, onlyEvaluateLocally: true, }) - this.pendingPromises[promiseUUID] = promise + this.addPendingPromise(promise) promise .then((flags) => { const featureVariantProperties: Record = {} @@ -172,9 +166,6 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 { .catch(() => { _capture({ ...properties, $groups: groups }) }) - .finally(() => { - delete this.pendingPromises[promiseUUID] - }) } else { _capture({ ...properties, $groups: groups }) } From b2cb7bfcee11da4b679d54104132d0e18d11987c Mon Sep 17 00:00:00 2001 From: Ben White Date: Fri, 5 Jan 2024 09:39:22 +0100 Subject: [PATCH 16/20] Fix promises --- posthog-core/src/index.ts | 14 ++--- posthog-node/src/posthog-node.ts | 96 ++++++++++++++++---------------- 2 files changed, 55 insertions(+), 55 deletions(-) diff --git a/posthog-core/src/index.ts b/posthog-core/src/index.ts index e9e027ec..51e937bb 100644 --- a/posthog-core/src/index.ts +++ b/posthog-core/src/index.ts @@ -519,13 +519,13 @@ export abstract class PostHogCoreStateless { body: payload, } const requestPromise = this.fetchWithRetry(url, fetchOptions) - this.addPendingPromise(requestPromise) - - requestPromise - .then(() => done()) - .catch((err) => { - done(err) - }) + this.addPendingPromise( + requestPromise + .then(() => done()) + .catch((err) => { + done(err) + }) + ) } private async fetchWithRetry( diff --git a/posthog-node/src/posthog-node.ts b/posthog-node/src/posthog-node.ts index 2d3cbcb1..3a4738f7 100644 --- a/posthog-node/src/posthog-node.ts +++ b/posthog-node/src/posthog-node.ts @@ -107,29 +107,29 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 { if (sendFeatureFlags) { // :TRICKY: If we flush, or need to shut down, to not lose events we want this promise to resolve before we flush - const promise = super.getFeatureFlagsStateless(distinctId, groups, undefined, undefined, disableGeoip) - this.addPendingPromise(promise) - - promise - .then((flags) => { - const featureVariantProperties: Record = {} - if (flags) { - for (const [feature, variant] of Object.entries(flags)) { - if (variant !== false) { - featureVariantProperties[`$feature/${feature}`] = variant + this.addPendingPromise( + super + .getFeatureFlagsStateless(distinctId, groups, undefined, undefined, disableGeoip) + .then((flags) => { + const featureVariantProperties: Record = {} + 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 }) - }) + 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 = {} for (const [key, value] of Object.entries(groups || {})) { @@ -137,35 +137,35 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 { } // :TRICKY: If we flush, or need to shut down, to not lose events we want this promise to resolve before we flush - const promise = this.getAllFlags(distinctId, { - groups: groupsWithStringValues, - disableGeoip, - onlyEvaluateLocally: true, - }) - this.addPendingPromise(promise) - promise - .then((flags) => { - const featureVariantProperties: Record = {} - if (flags) { - for (const [feature, variant] of Object.entries(flags)) { - featureVariantProperties[`$feature/${feature}`] = variant + this.addPendingPromise( + this.getAllFlags(distinctId, { + groups: groupsWithStringValues, + disableGeoip, + onlyEvaluateLocally: true, + }) + .then((flags) => { + const featureVariantProperties: Record = {} + 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 = { - ...featureVariantProperties, - } - if (activeFlags.length > 0) { - flagProperties = { - ...flagProperties, - $active_feature_flags: activeFlags, + const activeFlags = Object.keys(flags || {}).filter((flag) => flags?.[flag] !== false) + let flagProperties: Record = { + ...featureVariantProperties, } - } - _capture({ ...flagProperties, ...properties, $groups: groups }) - }) - .catch(() => { - _capture({ ...properties, $groups: groups }) - }) + if (activeFlags.length > 0) { + flagProperties = { + ...flagProperties, + $active_feature_flags: activeFlags, + } + } + _capture({ ...flagProperties, ...properties, $groups: groups }) + }) + .catch(() => { + _capture({ ...properties, $groups: groups }) + }) + ) } else { _capture({ ...properties, $groups: groups }) } From e33d7865704710c6e133ba8752c050f85bcc1feb Mon Sep 17 00:00:00 2001 From: Ben White Date: Fri, 5 Jan 2024 09:44:02 +0100 Subject: [PATCH 17/20] Fix --- posthog-core/src/index.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/posthog-core/src/index.ts b/posthog-core/src/index.ts index 51e937bb..8aee9bee 100644 --- a/posthog-core/src/index.ts +++ b/posthog-core/src/index.ts @@ -477,8 +477,6 @@ export abstract class PostHogCoreStateless { sent_at: currentISOTime(), } - const promiseUUID = generateUUID() - const done = (err?: any): void => { if (err) { this._events.emit('error', err) From e79189504be63ecd2618d3f2a2c16228c03a74f2 Mon Sep 17 00:00:00 2001 From: Ben White Date: Tue, 9 Jan 2024 12:04:52 +0000 Subject: [PATCH 18/20] fix: local props refactor (#150) --- posthog-core/test/test-utils/test-utils.ts | 10 +- posthog-node/src/feature-flags.ts | 12 ++- posthog-node/src/posthog-node.ts | 108 +++++++++------------ posthog-node/test/posthog-node.spec.ts | 40 ++++---- 4 files changed, 83 insertions(+), 87 deletions(-) diff --git a/posthog-core/test/test-utils/test-utils.ts b/posthog-core/test/test-utils/test-utils.ts index a326f3f2..8324fe00 100644 --- a/posthog-core/test/test-utils/test-utils.ts +++ b/posthog-core/test/test-utils/test-utils.ts @@ -5,9 +5,13 @@ export const wait = async (t: number): Promise => { } export const waitForPromises = async (): Promise => { - 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 => { diff --git a/posthog-node/src/feature-flags.ts b/posthog-node/src/feature-flags.ts index 2e3e092b..45fdb929 100644 --- a/posthog-node/src/feature-flags.ts +++ b/posthog-node/src/feature-flags.ts @@ -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 } diff --git a/posthog-node/src/posthog-node.ts b/posthog-node/src/posthog-node.ts index 3a4738f7..83c61521 100644 --- a/posthog-node/src/posthog-node.ts +++ b/posthog-node/src/posthog-node.ts @@ -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 = {} - 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 = {} - 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 = {} - 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 = { - ...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 = {} + 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 = {} + 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 { diff --git a/posthog-node/test/posthog-node.spec.ts b/posthog-node/test/posthog-node.spec.ts index 0a0392a0..8f6efbca 100644 --- a/posthog-node/test/posthog-node.spec.ts +++ b/posthog-node/test/posthog-node.spec.ts @@ -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([ { @@ -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({ @@ -98,6 +101,7 @@ describe('PostHog Node.js', () => { groups: { other_group: 'x' }, }) + await waitForPromises() jest.runOnlyPendingTimers() expect(getLastBatchEvents()?.[0]).toEqual( expect.objectContaining({ @@ -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([ @@ -194,6 +199,7 @@ describe('PostHog Node.js', () => { disableGeoip: false, }) + await waitForPromises() jest.runOnlyPendingTimers() const batchEvents = getLastBatchEvents() expect(batchEvents?.[0].properties).toEqual({ @@ -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 }, @@ -229,6 +237,7 @@ describe('PostHog Node.js', () => { disableGeoip: true, }) + await waitForPromises() jest.runOnlyPendingTimers() batchEvents = getLastBatchEvents() console.warn(batchEvents) @@ -248,6 +257,7 @@ describe('PostHog Node.js', () => { disableGeoip: false, }) + await waitForPromises() jest.runOnlyPendingTimers() batchEvents = getLastBatchEvents() expect(batchEvents?.[0].properties).toEqual({ @@ -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', @@ -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', @@ -589,7 +597,6 @@ describe('PostHog Node.js', () => { }) jest.runOnlyPendingTimers() - await waitForPromises() posthog.capture({ @@ -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( @@ -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', @@ -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([ From 25c7157cb62a81eb8a4d5e0abe2edee944772638 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Tue, 9 Jan 2024 12:43:20 +0000 Subject: [PATCH 19/20] fix test --- posthog-node/test/extensions/sentry-integration.spec.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/posthog-node/test/extensions/sentry-integration.spec.ts b/posthog-node/test/extensions/sentry-integration.spec.ts index 3c4f86a2..817e7dc3 100644 --- a/posthog-node/test/extensions/sentry-integration.spec.ts +++ b/posthog-node/test/extensions/sentry-integration.spec.ts @@ -3,6 +3,7 @@ import { PostHog as PostHog } from '../../src/posthog-node' import { PostHogSentryIntegration } from '../../src/extensions/sentry-integration' jest.mock('../../src/fetch') import fetch from '../../src/fetch' +import { waitForPromises } from 'posthog-core/test/test-utils/test-utils' jest.mock('../../package.json', () => ({ version: '1.2.3' })) @@ -108,6 +109,7 @@ describe('PostHogSentryIntegration', () => { processorFunction(createMockSentryException()) + await waitForPromises() jest.runOnlyPendingTimers() const batchEvents = getLastBatchEvents() From 04a995fe6d5a52da192f4ea4ff6bcff7b9028d61 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Tue, 9 Jan 2024 13:06:00 +0000 Subject: [PATCH 20/20] prepare for release --- posthog-node/CHANGELOG.md | 5 +++++ posthog-node/package.json | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/posthog-node/CHANGELOG.md b/posthog-node/CHANGELOG.md index 1e58f225..d60a5f23 100644 --- a/posthog-node/CHANGELOG.md +++ b/posthog-node/CHANGELOG.md @@ -1,3 +1,8 @@ +# 3.5.0 - 2024-01-09 + +1. When local evaluation is enabled, we automatically add flag information to all events sent to PostHog, whenever possible. This makes it easier to use these events in experiments. +2. Fixes a bug where in some rare cases we may drop events when send_feature_flags is enabled on capture. + # 3.4.0 - 2024-01-09 1. Numeric property handling for feature flags now does the expected: When passed in a number, we do a numeric comparison. When passed in a string, we do a string comparison. Previously, we always did a string comparison. diff --git a/posthog-node/package.json b/posthog-node/package.json index fa5a8788..28bdcee8 100644 --- a/posthog-node/package.json +++ b/posthog-node/package.json @@ -1,6 +1,6 @@ { "name": "posthog-node", - "version": "3.4.0", + "version": "3.5.0", "description": "PostHog Node.js integration", "repository": { "type": "git",