feat(flags): Add local props and flags to all calls (#149)

Co-authored-by: Ben White <[email protected]>
PostHog · Jan 9, 2024 · 20ea30d · 20ea30d
1 parent ae4d29b
commit 20ea30d
Show file tree

Hide file tree

Showing 9 changed files with 569 additions and 54 deletions.
diff --git a/posthog-core/src/index.ts b/posthog-core/src/index.ts
@@ -56,10 +56,10 @@ export abstract class PostHogCoreStateless {
   private captureMode: 'form' | 'json'
   private removeDebugCallback?: () => void
   private debugMode: boolean = false
-  private pendingPromises: Record<string, Promise<any>> = {}
   private disableGeoip: boolean = true
 
   private _optoutOverride: boolean | undefined
+  private pendingPromises: Record<string, Promise<any>> = {}
 
   // internal
   protected _events = new SimpleEventEmitter()
@@ -141,6 +141,14 @@ export abstract class PostHogCoreStateless {
     }
   }
 
+  protected addPendingPromise(promise: Promise<any>): void {
+    const promiseUUID = generateUUID()
+    this.pendingPromises[promiseUUID] = promise
+    promise.finally(() => {
+      delete this.pendingPromises[promiseUUID]
+    })
+  }
+
   /***
    *** TRACKING
    ***/
@@ -249,7 +257,7 @@ export abstract class PostHogCoreStateless {
     return this.fetchWithRetry(url, fetchOptions)
       .then((response) => response.json() as Promise<PostHogDecideResponse>)
       .catch((error) => {
-        console.error('Error fetching feature flags', error)
+        this._events.emit('error', error)
         return undefined
       })
   }
@@ -469,15 +477,11 @@ export abstract class PostHogCoreStateless {
       sent_at: currentISOTime(),
     }
 
-    const promiseUUID = generateUUID()
-
     const done = (err?: any): void => {
       if (err) {
         this._events.emit('error', err)
       }
       callback?.(err, messages)
-      // remove promise from pendingPromises
-      delete this.pendingPromises[promiseUUID]
       this._events.emit('flush', messages)
     }
 
@@ -513,13 +517,13 @@ export abstract class PostHogCoreStateless {
             body: payload,
           }
     const requestPromise = this.fetchWithRetry(url, fetchOptions)
-    this.pendingPromises[promiseUUID] = requestPromise
-
-    requestPromise
-      .then(() => done())
-      .catch((err) => {
-        done(err)
-      })
+    this.addPendingPromise(
+      requestPromise
+        .then(() => done())
+        .catch((err) => {
+          done(err)
+        })
+    )
   }
 
   private async fetchWithRetry(
@@ -565,6 +569,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-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<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 => {

diff --git 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
@@ -1,6 +1,6 @@
 {
   "name": "posthog-node",
-  "version": "3.4.0",
+  "version": "3.5.0",
   "description": "PostHog Node.js integration",
   "repository": {
     "type": "git",

diff --git 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}`))
         }
       }
     }
@@ -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
       }
 
@@ -383,7 +387,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
@@ -105,26 +105,54 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 {
       super.captureStateless(distinctId, event, props, { timestamp, disableGeoip })
     }
 
-    if (sendFeatureFlags) {
-      super.getFeatureFlagsStateless(distinctId, groups, undefined, undefined, disableGeoip).then((flags) => {
-        const featureVariantProperties: Record<string, string | boolean> = {}
+    // :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)
+        }
+
+        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,
+          })
+        }
+        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)) {
-            if (variant !== false) {
-              featureVariantProperties[`$feature/${feature}`] = variant
-            }
+            additionalProperties[`$feature/${feature}`] = variant
           }
         }
         const activeFlags = Object.keys(flags || {}).filter((flag) => flags?.[flag] !== false)
-        const flagProperties = {
-          $active_feature_flags: activeFlags || undefined,
-          ...featureVariantProperties,
+        if (activeFlags.length > 0) {
+          additionalProperties['$active_feature_flags'] = activeFlags
         }
-        _capture({ ...properties, $groups: groups, ...flagProperties })
+
+        return additionalProperties
       })
-    } else {
-      _capture({ ...properties, $groups: groups })
-    }
+      .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 {
@@ -156,8 +184,18 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 {
       disableGeoip?: boolean
     }
   ): Promise<string | boolean | undefined> {
-    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 +270,19 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 {
       disableGeoip?: boolean
     }
   ): Promise<JsonType | undefined> {
-    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 +373,18 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 {
       disableGeoip?: boolean
     }
   ): Promise<PosthogFlagsAndPayloadsResponse> {
-    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 +444,25 @@ export class PostHog extends PostHogCoreStateless implements PostHogNodeV1 {
     this.featureFlagsPoller?.stopPoller()
     return super.shutdownAsync()
   }
+
+  private addLocalPersonAndGroupProperties(
+    distinctId: string,
+    groups?: Record<string, string>,
+    personProperties?: Record<string, string>,
+    groupProperties?: Record<string, Record<string, string>>
+  ): { allPersonProperties: Record<string, string>; allGroupProperties: Record<string, Record<string, string>> } {
+    const allPersonProperties = { $current_distinct_id: distinctId, ...(personProperties || {}) }
+
+    const allGroupProperties: Record<string, Record<string, string>> = {}
+    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/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()
 

diff --git 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: '[email protected]' },
+          person_properties: {
+            $current_distinct_id: 'some-distinct-id_outside_rollout?',
+            region: 'USA',
+            email: '[email protected]',
+          },
           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,
         }),