feat: Add weighted mean for retention insights #28302

Sriram-bk · 2025-02-04T21:06:03Z

Problem

This PR adds functionality to allow our customers to get a weighted mean for their retention insights. It also fixes a bug where cohort sizes of 0 were being included in the mean calculations.

Closes #25998
Closes #26217

Changes

No mean

Simple Mean

6-bea6-19c71efdaad4" />

Weighted Mean

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

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

How did you test this code?

Tested locally.

greptile-apps

PR Summary

This PR enhances retention insights by adding weighted mean calculations and fixing cohort size issues. Here's a summary of the key changes:

Added new RetentionMeanDropdown component in /frontend/src/scenes/insights/filters/RetentionMeanDropdown.tsx replacing the boolean checkbox with a tri-state selector (none/simple/weighted)
Created migration 0660_migrate_retention_show_mean.py to convert existing boolean show_mean values to strings ('simple' or null)
Fixed bug in /frontend/src/scenes/retention/RetentionTable.tsx to exclude cohorts with 0 users from mean calculations
Updated type definitions in /frontend/src/types.ts and /posthog/schema.py to support string-based mean types
Modified query handling in /frontend/src/scenes/insights/utils.tsx for backward compatibility with old boolean values

_{💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!}

_{10 file(s) reviewed, 7 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

frontend/src/scenes/insights/filters/RetentionMeanDropdown.tsx

greptile-apps · 2025-02-04T21:07:12Z

frontend/src/scenes/insights/filters/RetentionMeanDropdown.tsx

+            onChange={(showMean) => {
+                updateInsightFilter({ showMean })
+            }}


style: consider adding type safety for showMean parameter to ensure it matches RetentionMeanType

greptile-apps · 2025-02-04T21:07:59Z

frontend/src/scenes/insights/utils.tsx

+        if (retentionFilter && 'showMean' in retentionFilter && typeof retentionFilter.showMean === 'boolean') {
+            retentionFilter.showMean = retentionFilter.showMean ? 'simple' : null


style: This code is duplicated from parseDraftQueryFromLocalStorage. Consider extracting to a shared helper function

greptile-apps · 2025-02-04T21:08:32Z

posthog/migrations/0660_migrate_retention_show_mean.py

+    retention_insights = Insight.objects.filter(filters__insight="RETENTION", deleted=False).exclude(
+        filters__show_mean__isnull=True
+    )


logic: The filter excludes null show_mean values but doesn't handle cases where show_mean key doesn't exist in filters dict. Consider adding filters__has_key='show_mean' to the query.

greptile-apps · 2025-02-04T21:08:32Z

posthog/migrations/0660_migrate_retention_show_mean.py

+        if isinstance(insight.filters.get("show_mean"), bool):
+            # Convert boolean to string - if True, use 'simple'
+            insight.filters["show_mean"] = "simple" if insight.filters["show_mean"] else None
+            insight.save()


style: No error handling for corrupted data where filters['show_mean'] exists but is neither boolean nor string. Consider adding a try/except block.

Suggested change

if isinstance(insight.filters.get("show_mean"), bool):

# Convert boolean to string - if True, use 'simple'

insight.filters["show_mean"] = "simple" if insight.filters["show_mean"] else None

insight.save()

try:

if isinstance(insight.filters.get("show_mean"), bool):

# Convert boolean to string - if True, use 'simple'

insight.filters["show_mean"] = "simple" if insight.filters["show_mean"] else None

insight.save()

except Exception as e:

print(f"Error processing insight {insight.id}: {e}")

posthog-bot · 2025-02-04T21:24:59Z

📸 UI snapshots have been updated

14 snapshot changes in total. 0 added, 14 modified, 0 deleted:

chromium: 0 added, 8 modified, 0 deleted (diff for shard 2)
webkit: 0 added, 6 modified, 0 deleted (diff for shard 2)

Triggered by this commit.

👉 Review this PR's diff of snapshots.

github-actions · 2025-02-04T22:40:35Z

Size Change: 0 B

Total Size: 1.18 MB

ℹ️ View Unchanged

Filename	Size
`frontend/dist/toolbar.js`	1.18 MB

_{compressed-size-action}

aspicer · 2025-02-05T01:00:44Z

frontend/src/scenes/insights/utils.tsx

@@ -587,7 +595,15 @@ export function crushDraftQueryForLocalStorage(query: Node<Record<string, any>>,

 export function parseDraftQueryFromURL(query: string): Node<Record<string, any>> | null {
    try {
-        return JSON.parse(query)
+        const parsedQuery = JSON.parse(query)


Can we put this repeated code in a helper function called something like "migrateDraftQuery"?

There will be a lot more of these I think.

aspicer · 2025-02-05T01:14:05Z

frontend/src/scenes/insights/filters/RetentionMeanDropdown.tsx

+                    label: 'Simple mean',
+                },
+                {
+                    value: 'weighted',


Do you think we could add a tooltip here that just describes what this is in slightly more words?

Not sure - do you think it's more clear if these modes are called
"Average by week"
"Average by person"

Or do you think the current "Mean" and "weight mean" are more clear?

I think simple mean and weighted mean are more clear. Even if someone doesn't know what a weighted mean is, it's something they can look up. I do think it might be helpful to have a concise tooltip though. Not sure what the copy should be, I'll sleep on it and add this in the AM.

aspicer · 2025-02-05T01:24:41Z

posthog/migrations/0660_migrate_retention_show_mean.py

+    Insight = apps.get_model("posthog", "Insight")
+
+    # Get all retention insights
+    retention_insights = Insight.objects.filter(


You're going to want to chunk these to avoid potential memory issues (although I think it would be okay as is):
see 0459_convert_personsnode_insights_to_actorsquery.py for an example

aspicer

A couple things to address but this looks (and works) fantastic! Amazing 🎉

anirudhpillai · 2025-02-05T15:50:56Z

Great work again, much needed feature!
What do you think about defaulting to always show the simple mean?
Defaulting to showing the mean instead of hiding it makes more sense (especially later when we add breakdowns where means will become important)

posthog-bot · 2025-02-06T05:58:13Z

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

posthog-bot · 2025-02-06T06:18:02Z

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

… bool values for showMean

posthog-bot · 2025-02-06T18:55:45Z

📸 UI snapshots have been updated

12 snapshot changes in total. 0 added, 12 modified, 0 deleted:

chromium: 0 added, 8 modified, 0 deleted (diff for shard 2)
webkit: 0 added, 4 modified, 0 deleted (diff for shard 2)

Triggered by this commit.

👉 Review this PR's diff of snapshots.

isAdrisal · 2025-02-07T01:57:38Z

@Sriram-bk is it possible this PR broke existing retention insights? I've just submitted help ticket #24857 with more details.

We have two retention charts that are now not loading, and the dashboard they were on shows an error message. All was working fine with these yesterday before this PR.

Also, when trying to recreate the insights to fix our dashboard, I get this error when toggling on the "Show mean across cohorts" option. If I toggle it back off, the error remains and I can't recover from it without having to start all over again.

Sriram-bk requested a review from a team February 4, 2025 21:06

Sriram-bk changed the title ~~Add weighted mean for retention insights~~ feat: Add weighted mean for retention insights Feb 4, 2025

greptile-apps bot reviewed Feb 4, 2025

View reviewed changes

aspicer reviewed Feb 5, 2025

View reviewed changes

PostHog deleted a comment from greptile-apps bot Feb 5, 2025

aspicer reviewed Feb 5, 2025

View reviewed changes

aspicer approved these changes Feb 5, 2025

View reviewed changes

Sriram-bk force-pushed the sri/retention-add-weighted-mean branch 2 times, most recently from ee2d752 to 1b92bf3 Compare February 6, 2025 00:10

Sriram-bk force-pushed the sri/retention-add-weighted-mean branch from 6a0f9fe to fdd5b67 Compare February 6, 2025 18:06

Sriram Balakrishnan and others added 13 commits February 6, 2025 10:34

Added migration to convert retention filter show_mean from bool to str

34cc9f5

Add weighted mean calculations to retention insight

233f8f9

Update the parseDraftQueryFromURL to account for older urls that have…

e1030a5

… bool values for showMean

Updated styles

7155b6e

Update schemas

b100bfd

Updated migration

d96e974

Added reverse migration

551dbdf

Fixed tests

9e66471

Added helper function for parsing and migrating query in utils.tsx

b5b4ca0

Chunked migration

6eed407

Added tooltips

2ac2550

Made simple mean the default

e291ebc

Fixed test failures

19c51bb

Fixed another test

7c24c03

Sriram-bk force-pushed the sri/retention-add-weighted-mean branch from fdd5b67 to 7c24c03 Compare February 6, 2025 18:35

github-actions bot added 2 commits February 6, 2025 18:46

Update UI snapshots for webkit (2)

9317a95

Update UI snapshots for chromium (2)

d46831b

Sriram-bk merged commit c20e135 into master Feb 6, 2025
103 checks passed

Sriram-bk deleted the sri/retention-add-weighted-mean branch February 6, 2025 21:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add weighted mean for retention insights #28302

feat: Add weighted mean for retention insights #28302

Sriram-bk commented Feb 4, 2025 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot Feb 4, 2025

greptile-apps bot Feb 4, 2025

greptile-apps bot Feb 4, 2025

greptile-apps bot Feb 4, 2025

posthog-bot commented Feb 4, 2025

github-actions bot commented Feb 4, 2025 •

edited

Loading

aspicer Feb 5, 2025

aspicer Feb 5, 2025 •

edited

Loading

Sriram-bk Feb 5, 2025

aspicer Feb 5, 2025

aspicer left a comment

anirudhpillai commented Feb 5, 2025

posthog-bot commented Feb 6, 2025

posthog-bot commented Feb 6, 2025

posthog-bot commented Feb 6, 2025

isAdrisal commented Feb 7, 2025 •

edited

Loading

		if (retentionFilter && 'showMean' in retentionFilter && typeof retentionFilter.showMean === 'boolean') {
		retentionFilter.showMean = retentionFilter.showMean ? 'simple' : null

feat: Add weighted mean for retention insights #28302

feat: Add weighted mean for retention insights #28302

Conversation

Sriram-bk commented Feb 4, 2025 • edited Loading

Problem

Changes

No mean

Simple Mean

Weighted Mean

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

How did you test this code?

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Feb 4, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 4, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 4, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 4, 2025

Choose a reason for hiding this comment

posthog-bot commented Feb 4, 2025

📸 UI snapshots have been updated

github-actions bot commented Feb 4, 2025 • edited Loading

aspicer Feb 5, 2025

Choose a reason for hiding this comment

aspicer Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Sriram-bk Feb 5, 2025

Choose a reason for hiding this comment

aspicer Feb 5, 2025

Choose a reason for hiding this comment

aspicer left a comment

Choose a reason for hiding this comment

anirudhpillai commented Feb 5, 2025

posthog-bot commented Feb 6, 2025

📸 UI snapshots have been updated

posthog-bot commented Feb 6, 2025

📸 UI snapshots have been updated

posthog-bot commented Feb 6, 2025

📸 UI snapshots have been updated

isAdrisal commented Feb 7, 2025 • edited Loading

Sriram-bk commented Feb 4, 2025 •

edited

Loading

github-actions bot commented Feb 4, 2025 •

edited

Loading

aspicer Feb 5, 2025 •

edited

Loading

isAdrisal commented Feb 7, 2025 •

edited

Loading