From 89e8deaa3c7fa6e6d889d88d0a63ceda3282673f Mon Sep 17 00:00:00 2001 From: Haven Barnes Date: Wed, 27 Nov 2024 18:00:57 -0800 Subject: [PATCH 1/9] chore(flags): Add new debugging property `$feature_flag_bootstrapped_response` and `$feature_flag_bootstrapped_payload` to `$feature_flag_called` event --- src/posthog-featureflags.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/posthog-featureflags.ts b/src/posthog-featureflags.ts index 89c0f7405..53d6d5142 100644 --- a/src/posthog-featureflags.ts +++ b/src/posthog-featureflags.ts @@ -244,7 +244,14 @@ export class PostHogFeatureFlags { } this.instance.persistence?.register({ [FLAG_CALL_REPORTED]: flagCallReported }) - this.instance.capture('$feature_flag_called', { $feature_flag: key, $feature_flag_response: flagValue }) + this.instance.capture('$feature_flag_called', { + $feature_flag: key, + $feature_flag_response: flagValue, + $feature_flag_payload: this.getFeatureFlagPayload(key) || null, + $feature_flag_bootstrapped_response: this.instance.config.bootstrap?.featureFlags?.[key] || null, + $feature_flag_bootstrapped_payload: + this.instance.config.bootstrap?.featureFlagPayloads?.[key] || null, + }) } } return flagValue From 70425b8041c64d1bdd09f12bd1485df9205902ce Mon Sep 17 00:00:00 2001 From: Haven Barnes Date: Wed, 27 Nov 2024 18:18:14 -0800 Subject: [PATCH 2/9] add test --- cypress/e2e/capture.cy.ts | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/cypress/e2e/capture.cy.ts b/cypress/e2e/capture.cy.ts index 3516e35e7..133370f52 100644 --- a/cypress/e2e/capture.cy.ts +++ b/cypress/e2e/capture.cy.ts @@ -160,6 +160,31 @@ describe('Event capture', () => { cy.phCaptures().should('include', '$feature_flag_called') }) + it('captures $feature_flag_called with bootstrapped value properties', () => { + start({ + options: { + bootstrap: { + featureFlags: { + 'some-feature': 'some-value', + }, + featureFlagPayloads: { + 'some-feature': 'some-payload', + }, + }, + }, + }) + + cy.get('[data-cy-feature-flag-button]').click() + + cy.phCaptures({ full: true }).then((captures) => { + const flagCallEvents = captures.filter((capture) => capture.event === '$feature_flag_called') + expect(flagCallEvents.length).to.eq(1) + const flagCallEvent = flagCallEvents[0] + expect(flagCallEvent.properties.$feature_flag_bootstrapped_response).to.equal('some-value') + expect(flagCallEvent.properties.$feature_flag_bootstrapped_payload).to.equal('some-payload') + }) + }) + it('captures rage clicks', () => { start({ options: { rageclick: true } }) From c4b0c0efb9150d5f62d23f4b309b04187b1bfe7d Mon Sep 17 00:00:00 2001 From: Haven Barnes Date: Wed, 4 Dec 2024 13:19:30 -0800 Subject: [PATCH 3/9] Add $used_bootstrap_value evt property, update tests --- cypress/e2e/capture.cy.ts | 20 +++++++++++++++++++- src/__tests__/featureflags.ts | 19 ++++++++++--------- src/decide.ts | 4 +++- src/posthog-core.ts | 2 ++ src/posthog-featureflags.ts | 11 +++++++---- 5 files changed, 41 insertions(+), 15 deletions(-) diff --git a/cypress/e2e/capture.cy.ts b/cypress/e2e/capture.cy.ts index 133370f52..bc11604d1 100644 --- a/cypress/e2e/capture.cy.ts +++ b/cypress/e2e/capture.cy.ts @@ -157,7 +157,14 @@ describe('Event capture', () => { cy.get('[data-cy-feature-flag-button]').click() - cy.phCaptures().should('include', '$feature_flag_called') + cy.phCaptures({ full: true }).then((captures) => { + const flagCallEvents = captures.filter((capture) => capture.event === '$feature_flag_called') + expect(flagCallEvents.length).to.eq(1) + const flagCallEvent = flagCallEvents[0] + expect(flagCallEvent.properties.$feature_flag_bootstrapped_response).to.not.exist + expect(flagCallEvent.properties.$feature_flag_bootstrapped_payload).to.not.exist + expect(flagCallEvent.properties.$used_bootstrap_value).to.equal(false) + }) }) it('captures $feature_flag_called with bootstrapped value properties', () => { @@ -171,9 +178,19 @@ describe('Event capture', () => { 'some-feature': 'some-payload', }, }, + advanced_disable_feature_flags: true, }, + waitForDecide: false, }) + cy.intercept( + '/decide/*', + { times: 1 }, + { + forceNetworkError: true, + } + ) + cy.get('[data-cy-feature-flag-button]').click() cy.phCaptures({ full: true }).then((captures) => { @@ -182,6 +199,7 @@ describe('Event capture', () => { const flagCallEvent = flagCallEvents[0] expect(flagCallEvent.properties.$feature_flag_bootstrapped_response).to.equal('some-value') expect(flagCallEvent.properties.$feature_flag_bootstrapped_payload).to.equal('some-payload') + expect(flagCallEvent.properties.$used_bootstrap_value).to.equal(true) }) }) diff --git a/src/__tests__/featureflags.ts b/src/__tests__/featureflags.ts index 7dd295c6c..0de1ee386 100644 --- a/src/__tests__/featureflags.ts +++ b/src/__tests__/featureflags.ts @@ -32,6 +32,7 @@ describe('featureflags', () => { get_property: (key) => instance.persistence.props[key], capture: () => {}, decideEndpointWasHit: false, + receivedFlagValues: false, _send_request: jest.fn().mockImplementation(({ callback }) => callback({ statusCode: 200, @@ -67,7 +68,7 @@ describe('featureflags', () => { }) it('should return flags from persistence even if decide endpoint was not hit', () => { - featureFlags.instance.decideEndpointWasHit = false + featureFlags.instance.receivedFlagValues = false expect(featureFlags.getFlags()).toEqual([ 'beta-feature', @@ -80,7 +81,7 @@ describe('featureflags', () => { it('should warn if decide endpoint was not hit and no flags exist', () => { ;(window as any).POSTHOG_DEBUG = true - featureFlags.instance.decideEndpointWasHit = false + featureFlags.instance.receivedFlagValues = false instance.persistence.unregister('$enabled_feature_flags') instance.persistence.unregister('$active_feature_flags') @@ -101,7 +102,7 @@ describe('featureflags', () => { }) it('should return the right feature flag and call capture', () => { - featureFlags.instance.decideEndpointWasHit = false + featureFlags.instance.receivedFlagValues = false expect(featureFlags.getFlags()).toEqual([ 'beta-feature', @@ -132,7 +133,7 @@ describe('featureflags', () => { }) it('should call capture for every different flag response', () => { - featureFlags.instance.decideEndpointWasHit = true + featureFlags.instance.receivedFlagValues = true instance.persistence.register({ $enabled_feature_flags: { @@ -156,13 +157,13 @@ describe('featureflags', () => { instance.persistence.register({ $enabled_feature_flags: {}, }) - featureFlags.instance.decideEndpointWasHit = false + featureFlags.instance.receivedFlagValues = false expect(featureFlags.getFlagVariants()).toEqual({}) expect(featureFlags.isFeatureEnabled('beta-feature')).toEqual(undefined) // no extra capture call because flags haven't loaded yet. expect(instance.capture).toHaveBeenCalledTimes(1) - featureFlags.instance.decideEndpointWasHit = true + featureFlags.instance.receivedFlagValues = true instance.persistence.register({ $enabled_feature_flags: { x: 'y' }, }) @@ -185,7 +186,7 @@ describe('featureflags', () => { }) it('should return the right feature flag and not call capture', () => { - featureFlags.instance.decideEndpointWasHit = true + featureFlags.instance.receivedFlagValues = true expect(featureFlags.isFeatureEnabled('beta-feature', { send_event: false })).toEqual(true) expect(instance.capture).not.toHaveBeenCalled() @@ -315,7 +316,7 @@ describe('featureflags', () => { }) it('onFeatureFlags callback should be called immediately if feature flags were loaded', () => { - featureFlags.instance.decideEndpointWasHit = true + featureFlags.instance.receivedFlagValues = true let called = false featureFlags.onFeatureFlags(() => (called = true)) expect(called).toEqual(true) @@ -324,7 +325,7 @@ describe('featureflags', () => { }) it('onFeatureFlags should not return flags that are off', () => { - featureFlags.instance.decideEndpointWasHit = true + featureFlags.instance.receivedFlagValues = true let _flags = [] let _variants = {} featureFlags.onFeatureFlags((flags, variants) => { diff --git a/src/decide.ts b/src/decide.ts index 88c7ab92d..ced733962 100644 --- a/src/decide.ts +++ b/src/decide.ts @@ -8,7 +8,7 @@ import { document } from './utils/globals' export class Decide { constructor(private readonly instance: PostHog) { // don't need to wait for `decide` to return if flags were provided on initialisation - this.instance.decideEndpointWasHit = this.instance._hasBootstrappedFeatureFlags() + this.instance.receivedFlagValues = this.instance._hasBootstrappedFeatureFlags() } call(): void { @@ -51,6 +51,8 @@ export class Decide { this.instance.featureFlags.receivedFeatureFlags(response ?? {}, errorsLoading) } + this.instance.decideEndpointWasHit = !errorsLoading + if (errorsLoading) { logger.error('Failed to fetch feature flags from PostHog.') return diff --git a/src/posthog-core.ts b/src/posthog-core.ts index 49e21a25f..5e1f9afac 100644 --- a/src/posthog-core.ts +++ b/src/posthog-core.ts @@ -276,6 +276,7 @@ export class PostHog { compression?: Compression __request_queue: QueuedRequestOptions[] decideEndpointWasHit: boolean + receivedFlagValues: boolean analyticsDefaultEndpoint: string version = Config.LIB_VERSION _initialPersonProfilesConfig: 'always' | 'never' | 'identified_only' | null @@ -295,6 +296,7 @@ export class PostHog { this.config = defaultConfig() this.decideEndpointWasHit = false + this.receivedFlagValues = false this.SentryIntegration = SentryIntegration this.sentryIntegration = (options?: SentryIntegrationOptions) => sentryIntegration(this, options) this.__request_queue = [] diff --git a/src/posthog-featureflags.ts b/src/posthog-featureflags.ts index 53d6d5142..04f62c4c8 100644 --- a/src/posthog-featureflags.ts +++ b/src/posthog-featureflags.ts @@ -209,6 +209,7 @@ export class PostHogFeatureFlags { // This is because we don't want to block clients waiting for flags to load. // It's possible they're waiting for the callback to render the UI, but it never occurs. this.receivedFeatureFlags(response.json ?? {}, errorsLoading) + this.instance.decideEndpointWasHit = !errorsLoading // :TRICKY: Reload - start another request if queued! this._startReloadTimer() @@ -227,7 +228,7 @@ export class PostHogFeatureFlags { * @param {Object|String} options (optional) If {send_event: false}, we won't send an $feature_flag_call event to PostHog. */ getFeatureFlag(key: string, options: { send_event?: boolean } = {}): boolean | string | undefined { - if (!this.instance.decideEndpointWasHit && !(this.getFlags() && this.getFlags().length > 0)) { + if (!this.instance.receivedFlagValues && !(this.getFlags() && this.getFlags().length > 0)) { logger.warn('getFeatureFlag for key "' + key + '" failed. Feature flags didn\'t load in time.') return undefined } @@ -251,6 +252,8 @@ export class PostHogFeatureFlags { $feature_flag_bootstrapped_response: this.instance.config.bootstrap?.featureFlags?.[key] || null, $feature_flag_bootstrapped_payload: this.instance.config.bootstrap?.featureFlagPayloads?.[key] || null, + // If we haven't yet received a response from the /decide endpoint, we must have used the bootstrapped value + $used_bootstrap_value: !this.instance.decideEndpointWasHit, }) } } @@ -273,7 +276,7 @@ export class PostHogFeatureFlags { * @param {Object|String} options (optional) If {send_event: false}, we won't send an $feature_flag_call event to PostHog. */ isFeatureEnabled(key: string, options: { send_event?: boolean } = {}): boolean | undefined { - if (!this.instance.decideEndpointWasHit && !(this.getFlags() && this.getFlags().length > 0)) { + if (!this.instance.receivedFlagValues && !(this.getFlags() && this.getFlags().length > 0)) { logger.warn('isFeatureEnabled for key "' + key + '" failed. Feature flags didn\'t load in time.') return undefined } @@ -292,7 +295,7 @@ export class PostHogFeatureFlags { if (!this.instance.persistence) { return } - this.instance.decideEndpointWasHit = true + this.instance.receivedFlagValues = true const currentFlags = this.getFlagVariants() const currentFlagPayloads = this.getFlagPayloads() parseFeatureFlagDecideResponse(response, this.instance.persistence, currentFlags, currentFlagPayloads) @@ -346,7 +349,7 @@ export class PostHogFeatureFlags { */ onFeatureFlags(callback: FeatureFlagsCallback): () => void { this.addFeatureFlagsHandler(callback) - if (this.instance.decideEndpointWasHit) { + if (this.instance.receivedFlagValues) { const { flags, flagVariants } = this._prepareFeatureFlagsForCallbacks() callback(flags, flagVariants) } From 29f8fbf8dfb2be19239b558f500bd23357ad799f Mon Sep 17 00:00:00 2001 From: Haven Barnes Date: Fri, 6 Dec 2024 11:23:05 -0800 Subject: [PATCH 4/9] bump testcafe result check timeout to 20m from 10m --- .github/workflows/testcafe.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/testcafe.yml b/.github/workflows/testcafe.yml index 7fffb7e3f..18b704ef8 100644 --- a/.github/workflows/testcafe.yml +++ b/.github/workflows/testcafe.yml @@ -62,5 +62,5 @@ jobs: run: pnpm testcafe ${{ matrix.browser }} --stop-on-first-fail - name: Check ${{ matrix.name }} events - timeout-minutes: 10 + timeout-minutes: 20 run: pnpm check-testcafe-results From 3895c2e64824788a080a6428f1d345714c3a6bab Mon Sep 17 00:00:00 2001 From: Haven Barnes Date: Fri, 6 Dec 2024 12:45:13 -0800 Subject: [PATCH 5/9] bump testcafe result check timeout to 60m just to get a passing run --- .github/workflows/testcafe.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/testcafe.yml b/.github/workflows/testcafe.yml index 18b704ef8..1d9d78886 100644 --- a/.github/workflows/testcafe.yml +++ b/.github/workflows/testcafe.yml @@ -62,5 +62,5 @@ jobs: run: pnpm testcafe ${{ matrix.browser }} --stop-on-first-fail - name: Check ${{ matrix.name }} events - timeout-minutes: 20 + timeout-minutes: 60 run: pnpm check-testcafe-results From 79bbfc11bbf07854e399800f28cd10307d34e987 Mon Sep 17 00:00:00 2001 From: Haven Barnes Date: Mon, 9 Dec 2024 10:16:26 -0800 Subject: [PATCH 6/9] bump --- .github/workflows/testcafe.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/testcafe.yml b/.github/workflows/testcafe.yml index 1d9d78886..a5dcfb95f 100644 --- a/.github/workflows/testcafe.yml +++ b/.github/workflows/testcafe.yml @@ -62,5 +62,5 @@ jobs: run: pnpm testcafe ${{ matrix.browser }} --stop-on-first-fail - name: Check ${{ matrix.name }} events - timeout-minutes: 60 + timeout-minutes: 120 run: pnpm check-testcafe-results From e87da17a52e0ce168db39b42f06cd9a23527d1c2 Mon Sep 17 00:00:00 2001 From: Haven Barnes Date: Mon, 9 Dec 2024 11:51:54 -0800 Subject: [PATCH 7/9] temp bump 120m timeout for checking results --- testcafe/check-testcafe-results.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testcafe/check-testcafe-results.js b/testcafe/check-testcafe-results.js index e65c214f1..4e0281aa9 100644 --- a/testcafe/check-testcafe-results.js +++ b/testcafe/check-testcafe-results.js @@ -47,7 +47,7 @@ If they seem to be failing unexpectedly, check grafana for ingestion lag at http log(JSON.stringify(files, null, 2)) // the deadline is the same for each assert, as the ingestion lag will be happening in parallel - const deadline = Date.now() + 1000 * 60 * 30 // 30 minutes + const deadline = Date.now() + 1000 * 60 * 120 // 30 minutes for (const file of files) { const testSessionId = file.testSessionId From 335412ca1623c0c5868acf7ea7bfbf9c2949adce Mon Sep 17 00:00:00 2001 From: Haven Barnes Date: Wed, 11 Dec 2024 18:16:01 -0800 Subject: [PATCH 8/9] bump again to get unblocked --- .github/workflows/testcafe.yml | 2 +- testcafe/check-testcafe-results.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/testcafe.yml b/.github/workflows/testcafe.yml index a5dcfb95f..9a00ad23b 100644 --- a/.github/workflows/testcafe.yml +++ b/.github/workflows/testcafe.yml @@ -62,5 +62,5 @@ jobs: run: pnpm testcafe ${{ matrix.browser }} --stop-on-first-fail - name: Check ${{ matrix.name }} events - timeout-minutes: 120 + timeout-minutes: 480 run: pnpm check-testcafe-results diff --git a/testcafe/check-testcafe-results.js b/testcafe/check-testcafe-results.js index 4e0281aa9..d421bdb4e 100644 --- a/testcafe/check-testcafe-results.js +++ b/testcafe/check-testcafe-results.js @@ -47,7 +47,7 @@ If they seem to be failing unexpectedly, check grafana for ingestion lag at http log(JSON.stringify(files, null, 2)) // the deadline is the same for each assert, as the ingestion lag will be happening in parallel - const deadline = Date.now() + 1000 * 60 * 120 // 30 minutes + const deadline = Date.now() + 1000 * 60 * 60 * 8 // 8 hours, temp change to get PR unblocked for (const file of files) { const testSessionId = file.testSessionId From 9c782e1fee878d4db00e14eb87e659355954df52 Mon Sep 17 00:00:00 2001 From: Haven Barnes Date: Thu, 12 Dec 2024 11:03:06 -0800 Subject: [PATCH 9/9] tweak --- .github/workflows/testcafe.yml | 2 +- testcafe/check-testcafe-results.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/testcafe.yml b/.github/workflows/testcafe.yml index 9a00ad23b..7fffb7e3f 100644 --- a/.github/workflows/testcafe.yml +++ b/.github/workflows/testcafe.yml @@ -62,5 +62,5 @@ jobs: run: pnpm testcafe ${{ matrix.browser }} --stop-on-first-fail - name: Check ${{ matrix.name }} events - timeout-minutes: 480 + timeout-minutes: 10 run: pnpm check-testcafe-results diff --git a/testcafe/check-testcafe-results.js b/testcafe/check-testcafe-results.js index d421bdb4e..e65c214f1 100644 --- a/testcafe/check-testcafe-results.js +++ b/testcafe/check-testcafe-results.js @@ -47,7 +47,7 @@ If they seem to be failing unexpectedly, check grafana for ingestion lag at http log(JSON.stringify(files, null, 2)) // the deadline is the same for each assert, as the ingestion lag will be happening in parallel - const deadline = Date.now() + 1000 * 60 * 60 * 8 // 8 hours, temp change to get PR unblocked + const deadline = Date.now() + 1000 * 60 * 30 // 30 minutes for (const file of files) { const testSessionId = file.testSessionId