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

fix(flags): Update feature flag insights when feature flag key changes #27340

Merged
merged 12 commits into from
Jan 9, 2025

Conversation

haacked
Copy link
Contributor

@haacked haacked commented Jan 7, 2025

Problem

This fixes #24938. The original issue describes a problem when changing a feature flag key. In that situation, the system created insights aren't updated accordingly. In particular, the description and the query filter need to be updated to reference the new key.

Changes

This changes the feature_flag api update method to update the system generated insights for a feature flag. It changes the description to include the new key. It also changes the filter query value for the $feature_flag term.

However, it has a limitation in that it only updates those insights if the user hasn't changed the filter or the insight name. If the user makes a change to the insights, we don't want to inadvertently stomp their changes, so we ignore it.

Does this work well for both Cloud and self-hosted?

Yes

How did you test this code?

  1. Wrote a couple unit tests.
  2. Tested that renaming a key updates the description and filter for the insights.
  3. Tested that renaming a key does not affect insights with a different name than the system generated ones.
  4. Tested that if you update an insight, then update it back so it matches what the system generated insight looks like, then it will get updated again. This isn't a requirement, but just a side-effect of the implementation and is fine.
  5. Tested that if an insight description matches system generated, but the filter does not, we do not update it. And vice versa.

@haacked haacked requested a review from a team January 7, 2025 23:53
@haacked haacked changed the title Fix: Update feature flag insights when feature flag key changes fix: Update feature flag insights when feature flag key changes Jan 8, 2025
@haacked haacked changed the title fix: Update feature flag insights when feature flag key changes fix(flags): Update feature flag insights when feature flag key changes Jan 8, 2025
This only updates the insights that the PostHog system creates when a feature flag creates.

If a user adds another insight to the feature flag dashboard, it's on them to keep it updated.

If the user changes the name of a system created insight, then we will not update it. We don't want to stomp on user changes.

Fixes #24938
If we change the name of these insights, we want to still be able to update them if the feature flag key changes.
@haacked haacked force-pushed the haacked/24938-update-usage-insight branch from 96875cf to 8671c60 Compare January 8, 2025 00:47
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.

this is generally excellent – thorough, clearly tested, and makes sense. I wanted to nitpick more but there isn't really a lot here that I think is worth nit-picking, to be honest. I left one note about my preference around raw f-strings vs helper functions but otherwise nothing blocking. I think I'll have more interesting feedback on a more complex PR ;)

Comment on lines +696 to +701
def _get_feature_flag_total_volume_insight_description(feature_flag_key: str) -> str:
return f"Shows the number of total calls made on feature flag with key: {feature_flag_key}"


def _get_feature_flag_unique_users_insight_description(feature_flag_key: str) -> str:
return f"Shows the number of unique user calls made on feature flag per variant with key: {feature_flag_key}"
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a "dylan preference" thing (as compared to an encoded PostHog convention), but I generally don't think this pattern of writing helper functions that just return f-strings is better than using an f-string in the _create_tile_for_insight method – I feel like f-strings are more readable than this type of helper function. IMO, the reasons I'd change an f-string into a helper like this would be:

  • we use this f-string a lot and copying the function saves a significant amount of characters
  • the formatting is complex or has logical constraints around the values that it is formatting (e.g., something like this
def format_currency(amount, currency="USD"):
    if not isinstance(amount, (int, float)):
        raise ValueError("Amount must be a number")
    return f"{currency} {amount:,.2f}"

price = format_currency(1234.56)  # "USD 1,234.56"

would make sense as a helper function, since it does more than just format the currency.

In this case, though, I don't think there's value in writing helpers just to f-string a single use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback!

The reason I made these helpers is because they're used in two places:

  1. _create_tile_for_insight
  2. _update_tile_with_new_key

I didn't want to hard-code the same strings in both places just in case we ever update the descriptions for these tiles. I was concerned that someone changing the description when we create a tile, might not notice its usage in _update_tile_with_new_key, which could throw off the logic.

@haacked haacked merged commit a529a8d into master Jan 9, 2025
92 checks passed
@haacked haacked deleted the haacked/24938-update-usage-insight branch January 9, 2025 00:56
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.

Changing feature flag key doesn't update the Usage insight correctly
2 participants