fix: Add weighted mean with the migration fixes #28458

Sriram-bk · 2025-02-07T20:17:49Z

Problem

This PR fixes the issues with the migration in this PR. The issues with the migration caused dashboards for certain customers to fail.

The original PR added the 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.

All the changes from the original PR are included in this one since it was reverted.

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 adds weighted mean calculation functionality for retention insights and fixes migration issues that caused dashboard failures. Here's a summary of the key changes:

Added RetentionMeanDropdown component with options for 'none', 'simple', and 'weighted' mean calculations in /frontend/src/scenes/insights/filters/RetentionMeanDropdown.tsx
Modified schema to change showMean from boolean to string enum ('none', 'simple', 'weighted') in /frontend/src/queries/schema.json
Added migration script in /posthog/migrations/0560_migrate_retention_show_mean.py to convert existing boolean values to string format
Fixed bug in RetentionTable.tsx to exclude cohorts with 0 users from mean calculations
Updated test files to reflect new string-based mean type instead of boolean

The changes appear well-structured and address both the weighted mean feature request and the zero-user cohort bug while ensuring backward compatibility.

_{💡 (5/5) You can turn off certain types of comments like style here!}

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

greptile-apps · 2025-02-07T20:20:29Z

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

+    const { retentionFilter } = useValues(insightVizDataLogic(insightProps))
+    const { updateInsightFilter } = useActions(insightVizDataLogic(insightProps))
+
+    const showMean = retentionFilter?.showMean || RETENTION_MEAN_NONE


style: Default fallback to RETENTION_MEAN_NONE is correct but could be more explicit with nullish coalescing (??) instead of logical OR (||) to handle edge cases where showMean is intentionally set to false

greptile-apps · 2025-02-07T20:20:30Z

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

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


style: Consider adding type safety by explicitly typing the onChange parameter as RetentionMeanType

Suggested change

onChange={(showMean) => {

updateInsightFilter({ showMean })

}}

onChange={(showMean: RetentionMeanType) => {

updateInsightFilter({ showMean })

}}

greptile-apps · 2025-02-07T20:21:12Z

frontend/src/scenes/retention/RetentionTable.tsx

+                                                const weights = validRows.map((row) =>
+                                                    parseInt(row[1]?.toString() || '0')
+                                                )


style: parseInt without radix parameter could lead to unexpected results with certain string formats. Consider using parseInt(x, 10) for explicit base-10 parsing.

Suggested change

const weights = validRows.map((row) =>

parseInt(row[1]?.toString() || '0')

)

const weights = validRows.map((row) =>

parseInt(row[1]?.toString() || '0', 10)

)

greptile-apps · 2025-02-07T20:21:13Z

frontend/src/scenes/retention/RetentionTable.tsx

+                                                    return !(
+                                                        (columnIndex >= row.length - 1 && isLatestPeriod) ||
+                                                        !row[columnIndex] ||
+                                                        row[columnIndex].count <= 0
+                                                    )


style: The row filtering logic is duplicated between weighted and simple mean calculations. Consider extracting this into a shared helper function.

greptile-apps · 2025-02-07T20:21:47Z

posthog/migrations/0560_migrate_retention_show_mean.py

+    )
+
+    for insight in retention_insights.iterator(chunk_size=100):
+        show_mean_value = insight.query["source"]["retentionFilter"]["showMean"]


logic: Accessing nested dictionary keys without .get() could raise KeyError if structure is malformed

posthog-bot · 2025-02-07T20:38:42Z

📸 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.

github-actions · 2025-02-07T20:51:27Z

Size Change: +441 B (0%)

Total Size: 9.73 MB

ℹ️ View Unchanged

Filename	Size	Change
`frontend/dist/toolbar.js`	9.73 MB	+441 B (0%)

_{compressed-size-action}

posthog-bot · 2025-02-08T00:20:56Z

📸 UI snapshots have been updated

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

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

posthog-bot · 2025-02-08T05:52:39Z

📸 UI snapshots have been updated

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

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

posthog-bot · 2025-02-08T06:15:17Z

📸 UI snapshots have been updated

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

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

posthog-bot · 2025-02-10T19:33:49Z

📸 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-10T19:55:32Z

📸 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.

aspicer · 2025-02-11T21:05:03Z

I tried testing this locally, but if you have a saved insight that has 'showMean' set to true, I'm not sure that it ends up being properly set to 'simple' once you switch to this branch.

I don't want to give bad advice here - @thmsobrmlr is there a place where you think it would best to have that sort of logic at the current time?

Sriram-bk · 2025-02-12T04:49:08Z

I tried testing this locally, but if you have a saved insight that has 'showMean' set to true, I'm not sure that it ends up being properly set to 'simple' once you switch to this branch.

@aspicer That behavior was expected. Since it's just a config value, I thought this would be okay in the brief time we do the migration to port the values. The only impact it would have on our users would be to set the mean retention once again. After which, the setting would be correctly set on the showMeanRetention attribute if they were to toggle it.

I didn't want to allow the value of showMean to override the value of showMeanRetention because if a user were to change the showMeanRetention value using the updated UI they wouldn't see the correct data due to the override, which I think would be more confusing.

I do agree that this isn't ideal, but it felt the least confusing/annoying. If you or @thmsobrmlr think some other approach would be better, more than happy to make any changes.

posthog-bot · 2025-02-13T08:20:29Z

📸 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.

posthog-bot · 2025-02-18T21:02:08Z

📸 UI snapshots have been updated

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

chromium: 0 added, 4 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.

posthog-bot · 2025-02-18T21:22:30Z

📸 UI snapshots have been updated

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

chromium: 0 added, 22 modified, 0 deleted (wasn't pushed!)
webkit: 0 added, 8 modified, 0 deleted (wasn't pushed!)

Triggered by this commit.

👉 Review this PR's diff of snapshots.

posthog-bot · 2025-02-18T21:42:12Z

📸 UI snapshots have been updated

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

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

anirudhpillai · 2025-02-20T11:35:05Z

Just cross posting, I think it's easier + makes sense to just default to showing the mean.
https://posthog.slack.com/archives/C0368RPHLQH/p1739870843981749

It makes sense to always show the mean and the user can hide it if they don't want it.

thmsobrmlr

Thanks for the great work @Sriram-bk ! I left a couple of comments inline, but the PR should be safe to merge in the sense that the insights won't break. In the edge cases users have to configure the show mean setting again, which is acceptable I think.

thmsobrmlr · 2025-02-12T08:54:56Z

frontend/src/queries/nodes/InsightQuery/utils/filtersToQueryNode.ts

Just for clarification: This function is used for migrating filter-based insights to query-based ones. We've already converted all existing insights a couple of weeks ago. Only people who use our API directly can still generate these insights. Today we have about 130 filter-based insights and only two of these are retention ones.

Since the retention mean feature was never available with filter based insights, I imagine we don't need to add any handling for this legacy use case. If we were to do so however, it would only work if we also added a mixin here https://github.com/PostHog/posthog/blob/master/posthog/models/filters/filter.py, as otherwise the filter will get lost when serializing. We'd probably also want to handle both showMean and showMeanRetention then.

thmsobrmlr · 2025-02-24T21:15:56Z

frontend/src/queries/nodes/InsightQuery/utils/filtersToQueryNode.test.ts

@@ -669,7 +669,7 @@ describe('filtersToQueryNode', () => {
                returning_entity: { id: '1' },
                target_entity: { id: '1' },
                period: RetentionPeriod.Day,
-                show_mean: true,
+                mean_retention_calculation: 'simple',


nit: would be better to test that a show_mean filter gets converted to a meanRetentionCalculation field on the query. legacy queries exist with legacy properties.

thmsobrmlr · 2025-02-24T21:26:51Z

posthog/hogql_queries/legacy_compatibility/filter_to_query.py

@@ -539,6 +539,7 @@ def _insight_filter(filter: dict, allow_variables: bool = False):
                ),
                period=filter.get("period"),
                showMean=filter.get("show_mean"),
+                meanRetentionCalculation=filter.get("mean_retention_calculation"),


nit: the conversion in this method should do the same thing that we do frontend side i.e. in frontend/src/queries/nodes/InsightQuery/utils/filtersToQueryNode.ts. here you had this code:

meanRetentionCalculation: filters.mean_retention_calculation || (typeof filters.show_mean === 'boolean' ? (filters.show_mean ? 'simple' : 'none') : 'simple'),

it doesn't really matter in this case, as there shouldn't be legacy insights with a show_mean filter. just commenting this for completeness.

thmsobrmlr · 2025-02-24T21:33:10Z

frontend/src/scenes/insights/utils.tsx

-export function parseDraftQueryFromLocalStorage(
-    query: string
-): { query: Node<Record<string, any>>; timestamp: number } | null {
+function parseAndMigrateQuery<T>(query: string): T | null {


This should be added to the notebooks migration https://github.com/PostHog/posthog/blob/master/frontend/src/scenes/notebooks/Notebook/migrations/migrate.ts#L44-L56 as well. Ideally there should also be a unit test.

thmsobrmlr · 2025-02-24T21:40:17Z

posthog/management/commands/migrate_retention_show_mean.py

+    Migrate the showMean boolean field to meanRetentionCalculation string field in retention insights.
+    """
+    retention_insights = Insight.objects.filter(
+        deleted=False,


You don't need to add deleted=False, as that's the default for the InsightsManager. I'd argue that we want to migrate delete insights as well though, and thus we should use objects_including_soft_deleted.

thmsobrmlr · 2025-02-24T21:42:43Z

posthog/management/commands/migrate_retention_show_mean.py

+            if live_run:
+                with transaction.atomic():
+                    # Convert boolean to string - if True, use 'simple' else 'none'
+                    insight.query["source"]["retentionFilter"]["meanRetentionCalculation"] = (


Since we default to show the mean retention now, the field should only be None if showMean is explicitly False.

thmsobrmlr

Thanks for the great work @Sriram-bk ! I left a couple of comments inline, but the PR should be safe to merge in the sense that the insights won't break. In the edge cases users have to configure the show mean setting again, which is acceptable I think.

Fixed another test

Update UI snapshots for `chromium` (1)

…books

greptile-apps bot reviewed Feb 7, 2025

View reviewed changes

Sriram-bk force-pushed the sri/retention-add-weighted-mean-migration-fixed branch from a594a3b to 8e50320 Compare February 8, 2025 05:33

Sriram-bk requested review from a team and aspicer February 8, 2025 05:38

Sriram-bk force-pushed the sri/retention-add-weighted-mean-migration-fixed branch from 599b8e0 to 537f6e6 Compare February 10, 2025 18:25

Sriram-bk force-pushed the sri/retention-add-weighted-mean-migration-fixed branch from 654be72 to d2c92b1 Compare February 13, 2025 07:59

Sriram-bk force-pushed the sri/retention-add-weighted-mean-migration-fixed branch from 58fd242 to e1df09c Compare February 18, 2025 20:41

Sriram-bk requested a review from thmsobrmlr February 18, 2025 21:04

Sriram-bk force-pushed the sri/retention-add-weighted-mean-migration-fixed branch from 313c7f8 to 2dc83f7 Compare February 18, 2025 21:06

Sriram-bk force-pushed the sri/retention-add-weighted-mean-migration-fixed branch from e29f25e to a9a27b2 Compare February 18, 2025 23:14

Sriram-bk force-pushed the sri/retention-add-weighted-mean-migration-fixed branch from f724c9b to 424b98f Compare February 20, 2025 18:49

Sriram-bk added the team/product-analytics label Feb 24, 2025

thmsobrmlr approved these changes Feb 24, 2025

View reviewed changes

Sriram-bk force-pushed the sri/retention-add-weighted-mean-migration-fixed branch 2 times, most recently from 2605cf4 to b87d496 Compare February 25, 2025 18:13

Sriram-bk and others added 29 commits February 28, 2025 14:37

Update schemas

beeddc2

Updated migration

18f179b

Added reverse migration

183b025

Fixed tests

84d1dc0

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

2dc26e9

Chunked migration

dc72d8b

Added tooltips

b1394e5

Made simple mean the default

5229bb8

Fixed test failures

a822e64

Fixed another test

Renamed migration file and updated migration

eb9b266

Update showMean schema def to be Union bool | str

6f2ac8a

Added showMeanRetention to replace showMean

a1b739a

Fixed test

069556c

Removed migration file and added migration mgmt command instead

93cb616

Fixed up tests

f215caa

Update UI snapshots for chromium (1)

b51acbc

Update UI snapshots for `chromium` (1)

Updated filterToQueryNode to account for deprecated showMean for note…

55b12a4

…books

Refactored showMeanRetention to meanRetentionCalculation

8925673

Remove cumulative from rebase

e2b4b7a

Test fixes

2dcdaa5

Update UI snapshots for chromium (1)

fdba237

CR fixes

9e74486

Remove retentionLineGraphLogic.ts

1e14bf4

Update UI snapshots for chromium (1)

6e2547b

Minor fixes after rebase

86a7e44

Update UI snapshots for webkit (2)

9ccf71e

Update UI snapshots for chromium (2)

b8a819f

Update UI snapshots for chromium (1)

3ce7da5

Update UI snapshots for chromium (2)

8617865

Sriram-bk force-pushed the sri/retention-add-weighted-mean-migration-fixed branch from 5ef7fe3 to 8617865 Compare February 28, 2025 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Add weighted mean with the migration fixes #28458

fix: Add weighted mean with the migration fixes #28458

Sriram-bk commented Feb 7, 2025

greptile-apps bot left a comment

greptile-apps bot Feb 7, 2025

greptile-apps bot Feb 7, 2025

greptile-apps bot Feb 7, 2025

greptile-apps bot Feb 7, 2025

greptile-apps bot Feb 7, 2025

posthog-bot commented Feb 7, 2025

github-actions bot commented Feb 7, 2025 •

edited

Loading

posthog-bot commented Feb 8, 2025

posthog-bot commented Feb 8, 2025

posthog-bot commented Feb 8, 2025

posthog-bot commented Feb 10, 2025

posthog-bot commented Feb 10, 2025

aspicer commented Feb 11, 2025

Sriram-bk commented Feb 12, 2025

posthog-bot commented Feb 13, 2025

posthog-bot commented Feb 18, 2025

posthog-bot commented Feb 18, 2025

posthog-bot commented Feb 18, 2025

anirudhpillai commented Feb 20, 2025

thmsobrmlr left a comment

thmsobrmlr Feb 12, 2025

thmsobrmlr Feb 24, 2025

thmsobrmlr Feb 24, 2025

thmsobrmlr Feb 24, 2025

thmsobrmlr Feb 24, 2025

thmsobrmlr Feb 24, 2025

thmsobrmlr left a comment

fix: Add weighted mean with the migration fixes #28458

Are you sure you want to change the base?

fix: Add weighted mean with the migration fixes #28458

Conversation

Sriram-bk commented Feb 7, 2025

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 7, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 7, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 7, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 7, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 7, 2025

Choose a reason for hiding this comment

posthog-bot commented Feb 7, 2025

📸 UI snapshots have been updated

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

posthog-bot commented Feb 8, 2025

📸 UI snapshots have been updated

posthog-bot commented Feb 8, 2025

📸 UI snapshots have been updated

posthog-bot commented Feb 8, 2025

📸 UI snapshots have been updated

posthog-bot commented Feb 10, 2025

📸 UI snapshots have been updated

posthog-bot commented Feb 10, 2025

📸 UI snapshots have been updated

aspicer commented Feb 11, 2025

Sriram-bk commented Feb 12, 2025

posthog-bot commented Feb 13, 2025

📸 UI snapshots have been updated

posthog-bot commented Feb 18, 2025

📸 UI snapshots have been updated

posthog-bot commented Feb 18, 2025

📸 UI snapshots have been updated

posthog-bot commented Feb 18, 2025

📸 UI snapshots have been updated

anirudhpillai commented Feb 20, 2025

thmsobrmlr left a comment

Choose a reason for hiding this comment

thmsobrmlr Feb 12, 2025

Choose a reason for hiding this comment

thmsobrmlr Feb 24, 2025

Choose a reason for hiding this comment

thmsobrmlr Feb 24, 2025

Choose a reason for hiding this comment

thmsobrmlr Feb 24, 2025

Choose a reason for hiding this comment

thmsobrmlr Feb 24, 2025

Choose a reason for hiding this comment

thmsobrmlr Feb 24, 2025

Choose a reason for hiding this comment

thmsobrmlr left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 7, 2025 •

edited

Loading