Skip to content

Conversation

@haacked
Copy link
Collaborator

@haacked haacked commented Feb 10, 2025

The client.get_feature_flag method will by default call client.capture with the event named $feature_flag_called.

Some time ago (in e60d52c), the capture method was changed to enrich the event with all locally cached feature flags even if send_feature_flags is False.

What this leads to is client.get_feature_flag -> client.capture -> client.get_all_flags. But the call to client.get_all_flags isn't passed the original person_properties that were passed to client.get_feature_flag. This leads to inconsistency in the enriched data. This can also cause confusion because it's unclear which feature flag is the one being called because the event adds "all" the locally cached feature flags.

This PR takes the simplest path by not enriching the captured event with all feature flags for the $feature_flag_called event.

Note that this didn't show up in the tests because most of the tests of get_feature_flag apply @mock.patch.object(Client, "capture") to the test, which hides this unexpected behavior.

@haacked haacked requested a review from dmarticus February 10, 2025 19:32
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR fixes an issue where the PostHog Python client was incorrectly enriching feature flag events with all cached flags, leading to potential data inconsistencies and unclear flag evaluation tracking.

  • Modifies client.get_feature_flag to prevent enriching $feature_flag_called events with all cached feature flags
  • Adds test case test_capture_is_called_but_does_not_add_all_flags to verify only specific flag data is included
  • Fixes inconsistency where enriched data didn't use original person properties from get_feature_flag
  • Addresses hidden issue that wasn't caught by existing tests due to mocked capture calls

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@haacked
Copy link
Collaborator Author

haacked commented Feb 10, 2025

This change should probably be applied to all the other client libraries.

@haacked haacked requested a review from havenbarnes February 10, 2025 21:01
Copy link
Contributor

@dmarticus dmarticus left a comment

Choose a reason for hiding this comment

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

I think this is a reasonable change, and the implementation + test makes sense. Two follow-up questions from my perspective:

  1. does this behavior exist for all SDKs (at the very least, the most popular ones that aren't python (web, posthog-js-lite, and Android)?
  2. what's the documentation story behind this? My guess is we don't have this documented; if that's the case, I think it's worth adding docs around this. If we do have it documented, we'd need to update the docs. Overall, it feels like docs around how we emit posthog events seems like a smart thing to do

@haacked
Copy link
Collaborator Author

haacked commented Feb 10, 2025

  1. Here's what I've found:
    a. This getFeatureFlag method in https://github.com/PostHog/posthog-js-lite/ doesn't have the sendFeatureFlags argument.
    b. This getFeatureFlag method appears to have the same issue. I'd have to test it to be sure. I can start on that one.
    c. The capture method in posthog-android doesn't include feature flags.
  2. There doesn't seem to be any documentation about this that I could find. The Capturing events page doesn't mention the behavior. The SDK documentation for the posthog-js-lite says this:
  • @param sendFeatureFlags OPTIONAL | Used with experiments. Determines whether to send feature flag values with the event.

It may make sense to amend that to be more correct. Something like:

  • @param sendFeatureFlags OPTIONAL | Used with experiments. Determines whether to request and send up-to-date feature flag values with the event. If false, but locally evaluated feature flags are already cached, it will send the locally cached feature flags with the event.

I can certainly follow-up with the documentation and the other libraries. But that probably shouldn't hold up this change.

@dmarticus
Copy link
Contributor

Awesome, your investigation makes sense, and agreed that it's not blocking for this, just wanted to get an idea of the scope.

@haacked haacked merged commit a3cf4ad into master Feb 11, 2025
2 checks passed
@haacked haacked deleted the haacked/fix-extra-feature-capture branch February 11, 2025 01:39
haacked added a commit to PostHog/posthog-dotnet that referenced this pull request Feb 11, 2025
haacked added a commit to PostHog/posthog-js-lite that referenced this pull request Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants