Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

chore(flags): Add new debugging property $used_bootstrap_value, $feature_flag_bootstrapped_response and $feature_flag_bootstrapped_payload to $feature_flag_called event #1567

Merged
merged 15 commits into from
Dec 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 44 additions & 1 deletion cypress/e2e/capture.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,50 @@ 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', () => {
start({
options: {
bootstrap: {
featureFlags: {
'some-feature': 'some-value',
},
featureFlagPayloads: {
'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) => {
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')
expect(flagCallEvent.properties.$used_bootstrap_value).to.equal(true)
})
})

it('captures rage clicks', () => {
Expand Down
19 changes: 10 additions & 9 deletions src/__tests__/featureflags.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ describe('featureflags', () => {
get_property: (key) => instance.persistence.props[key],
capture: () => {},
decideEndpointWasHit: false,
receivedFlagValues: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this PR we are essentially find+replacing the existing flag decideEndpointWasHit for this new, more accurately named receivedFlagValues flag. The prior will now mean that the endpoint truly was hit with no errors, and the latter will mean that there are flag values available for the SDK to return.
decideEndpointWasHit is now really only used for passing back the $used_bootstrap_value flag in $feature_flag_called events.

_send_request: jest.fn().mockImplementation(({ callback }) =>
callback({
statusCode: 200,
Expand Down Expand Up @@ -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',
Expand All @@ -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')

Expand All @@ -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',
Expand Down Expand Up @@ -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: {
Expand All @@ -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' },
})
Expand All @@ -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()
Expand Down Expand Up @@ -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)
Expand All @@ -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) => {
Expand Down
4 changes: 3 additions & 1 deletion src/decide.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const logger = createLogger('[Decide]')
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()
}

private _loadRemoteConfigJs(cb: (config?: RemoteConfig) => void): void {
Expand Down Expand Up @@ -119,6 +119,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
Expand Down
2 changes: 2 additions & 0 deletions src/posthog-core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 = []
Expand Down
20 changes: 15 additions & 5 deletions src/posthog-featureflags.ts
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,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()
Expand All @@ -229,7 +230,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
}
Expand All @@ -246,7 +247,16 @@ 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,
havenbarnes marked this conversation as resolved.
Show resolved Hide resolved
$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,
})
}
}
return flagValue
Expand All @@ -268,7 +278,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
}
Expand All @@ -287,7 +297,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)
Expand Down Expand Up @@ -341,7 +351,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)
}
Expand Down
Loading