feat(flags): Add feature flag version info, evaluation reason, and id to $feature_flag_called event
#427
Conversation
|
Size Change: +6.31 kB (+3.92%) Total Size: 167 kB
ℹ️ View Unchanged
|
| silent: true, | ||
| verbose: false, |
There was a problem hiding this comment.
I turned these off because there's a lot of console output when running tests which adds a lot of noise to the test output and makes it hard to find what really went wrong.
There was a problem hiding this comment.
yeah, these can be configured at test time through CLI arguments anyway.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Here's my review of the pull request:
This PR adds support for version 4 of the /decide endpoint, enhancing feature flag tracking with version info, evaluation reasons, and flag IDs in the $feature_flag_called event.
Key changes:
- Added new
FeatureFlagDetail,FeatureFlagMetadata, andEvaluationReasontypes inposthog-core/src/types.tsto support additional flag metadata - Introduced
featureFlagUtils.tswith utility functions for processing feature flags and converting between v3/v4 formats - Updated
/decideendpoint calls to use v4 by default while maintaining backward compatibility - Enhanced
$feature_flag_calledevents with new properties:$feature_flag_version,$feature_flag_reason, and$feature_flag_id - Fixed duplicate code in
getFeatureFlagPayloadwhere default values were being set twice
The implementation looks solid with good test coverage, though I recommend:
- Adding tests to verify the new event properties in
posthog-web.spec.ts - Filling out the PR template's bump level and affected libraries checkboxes for version management
11 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile
haacked
force-pushed
the
haacked/decide-v4-support
branch
2 times, most recently
from
8da60bc to
99647da
Compare
|
this will affect react native as well so make sure to bump versions/chagelog, etc |
Thanks for the heads up! I moved the PR back to draft because I need to verify the changes with RN. |
It's a lot of noise when running tests.
We pass in the flag id on the query string. Might as well use it!
haacked
force-pushed
the
haacked/decide-v4-support
branch
from
99647da to
c53e6f1
Compare
If the bootstrap.featureFlagPayloads has a key that's not in bootstrap.featureFlags, we assume the caller wanted that to be a true flag.
Handle v4 Response from decide
This changes how we load and save flags from persistence to support the new v4 format. However, we have to be careful to honor any flags already in persistence in the legacy format.
Use getFeatureFlagDetails to reimplement it.
haacked
force-pushed
the
haacked/decide-v4-support
branch
from
c53e6f1 to
fc5f1d4
Compare
haacked
force-pushed
the
haacked/decide-v4-support
branch
from
fc5f1d4 to
fe9d19c
Compare
The $feature_flag_called event now has additional information such as `feature_flag_id`, `feature_flag_version`, `feature_flag_reason`, and `feature_flag_requestId` Fixes #427
|
I tested |
There was a problem hiding this comment.
PR Summary
This PR enhances feature flag evaluation by upgrading to the /decide?v=4 endpoint and inserting additional metadata into the $feature_flag_called event.
• Introduced new types and utility functions (posthog-core/src/types.ts, posthog-core/src/featureFlagUtils.ts) to support flag versioning, evaluation reason, and ID.
• Updated event processing in posthog-core/src/index.ts and posthog-node/src/posthog-node.ts to include the new properties.
• Modified tests (posthog-node/test/feature-flags.decide.spec.ts, posthog-core/test/posthog.featureflags.spec.ts) to validate decide v4 responses.
• Improved developer experience with jest.config.js and README.md updates.
21 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
haacked
force-pushed
the
haacked/decide-v4-support
branch
from
fe9d19c to
b6955c2
Compare
|
I'm going to change this to only deploy Done! |
| silent: true, | ||
| verbose: false, |
There was a problem hiding this comment.
yeah, these can be configured at test time through CLI arguments anyway.
| } | ||
|
|
||
| flagDetail = remoteResponse.response | ||
| response = getFeatureFlagValue(flagDetail) ?? false |
There was a problem hiding this comment.
this is the change where we should just return a FeatureFlagValue here instead of defaulting to false (i.e. let it be undefined if we couldn't compute it for whatever reason).
There was a problem hiding this comment.
Yeah, I did this because that's the current behavior.
posthog-node/src/posthog-node.ts
Outdated
| matchValue = await this.getFeatureFlag(key, distinctId, { | ||
| ...options, | ||
| onlyEvaluateLocally: true, | ||
| sendFeatureFlagEvents: false, // TODO: Discuss with @dylan. |
There was a problem hiding this comment.
per discussion, we should leave this false. cc @marandaneto does that make sense to you, too?
There was a problem hiding this comment.
The reason I think we should leave sendFeatureFlagEvents false here is this path only applies to local evaluation. It'd be weird for it send feature flag events in this case, but not in the /decide case. If we want both cases to send feature flag events, we can discuss separately and do that as a follow-up.
| // If we haven't loaded flags yet, or errored out, we respond with undefined | ||
| return { | ||
| response: undefined, | ||
| requestId: undefined, | ||
| } | ||
| } | ||
|
|
||
| let response = featureFlags[key] | ||
| // `/decide` v3 returns all flags | ||
| let response = getFeatureFlagValue(flagDetailResponse.response) |
There was a problem hiding this comment.
make sure this can return undefined if we did error out.
There was a problem hiding this comment.
Do you want me to make that change in this PR or is that something you plan to do after this?
There was a problem hiding this comment.
Ok, we'll do this as a follow-up.
It's now a TODID.
Updates
posthog-nodeto take advantage of/decide?v=4changes. See PostHog/posthog#29751 for more info.The end result is
$feature_flag_calledevents will have additional properties:$feature_flag_version$feature_flag_reason$feature_flag_idProblem
Changes
Release info Sub-libraries affected
Bump level
Libraries affected
Changelog notes