feat(experiments): support time units in conversion windows #30871

andehen · 2025-04-07T12:50:07Z

Problem

We currently only support specifying hours. Cumbersome to enter days f.ex.

Changes

Add support for specifying a time unit
split out into separate component

How did you test this code?

modified tests to work with new format, tests passes
manually set and unset time windows and verified content in the db

greptile-apps

PR Summary

The changes introduce multi-unit conversion window support for experiment metrics by refactoring both frontend and backend logic to replace the old hour-only configuration.

Updated /frontend/src/queries/schema/schema-general.ts and /posthog/schema.py to replace time_window_hours with conversion_window and conversion_window_unit.
Refactored /frontend/src/scenes/experiments/ExperimentMetricForm.tsx by extracting conversion window handling into a new component at /frontend/src/scenes/experiments/ExperimentMetricConversionWindowFilter.tsx.
Modified /posthog/hogql_queries/experiments/funnel_query_utils.py and /posthog/hogql_queries/experiments/experiment_query_runner.py to incorporate the new conversion window logic.
Added utility conversion_window_to_seconds in /posthog/hogql_queries/experiments/base_query_utils.py with corresponding test updates.

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

greptile-apps · 2025-04-07T12:50:42Z

frontend/src/scenes/experiments/ExperimentMetricConversionWindowFilter.tsx

+                    onChange={(value) =>
+                        handleSetMetric({
+                            ...metric,
+                            conversion_window: value === 'experiment_duration' ? undefined : 14,
+                            conversion_window_unit:
+                                value === 'experiment_duration' ? undefined : FunnelConversionWindowTimeUnit.Day,
+                        })
+                    }


logic: Switching to time_window mode always resets conversion_window to 14 and unit to Day. Consider preserving the user's previous conversion_window_unit if available.

github-actions · 2025-04-07T13:15:15Z

Size Change: +400 B (0%)

Total Size: 13.2 MB

ℹ️ View Unchanged

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

_{compressed-size-action}

posthog-bot · 2025-04-07T13:20:29Z

📸 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-04-07T13:31:13Z

📸 UI snapshots have been updated

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

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

posthog-bot · 2025-04-07T13:40:29Z

📸 UI snapshots have been updated

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

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

posthog-bot · 2025-04-07T13:49:54Z

📸 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 7)
webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

rodrigoi · 2025-04-08T23:54:04Z

frontend/src/scenes/experiments/ExperimentMetricConversionWindowFilter.tsx

+    const options: LemonSelectOption<FunnelConversionWindowTimeUnit>[] = Object.keys(TIME_INTERVAL_BOUNDS).map(
+        (unit) => ({
+            label: capitalizeFirstLetter(pluralize(metric.conversion_window ?? 72, unit, `${unit}s`, false)),
+            value: unit as FunnelConversionWindowTimeUnit,
+        })
+    )
+    const intervalBounds = TIME_INTERVAL_BOUNDS[metric.conversion_window_unit ?? FunnelConversionWindowTimeUnit.Day]


⛏️ Complete and absolute nitpick: you could extract these as functions that take an ExperimentMetric argument. Then, they could also be moved out into FunnelConversionWindowFilter scene, removing the TIME_INTERVAL_BOUNDS import (hiding that away).

As the functions are only used here and are pretty straight forward, I think I prefer leaving them there for now.

rodrigoi · 2025-04-09T03:47:45Z

frontend/src/scenes/experiments/ExperimentMetricForm.tsx

-                    )}
-                </div>
-            </div>
+            <ExperimentMetricConversionWindowFilter metric={metric} handleSetMetric={handleSetMetric} />


rodrigoi · 2025-04-09T04:06:19Z

posthog/hogql_queries/experiments/base_query_utils.py

+def conversion_window_to_seconds(conversion_window: int, conversion_window_unit: FunnelConversionWindowTimeUnit) -> int:
+    match conversion_window_unit:
+        case FunnelConversionWindowTimeUnit.SECOND:
+            multiplier = 1
+        case FunnelConversionWindowTimeUnit.MINUTE:
+            multiplier = 60
+        case FunnelConversionWindowTimeUnit.HOUR:
+            multiplier = 60 * 60
+        case FunnelConversionWindowTimeUnit.DAY:
+            multiplier = 24 * 60 * 60
+        case FunnelConversionWindowTimeUnit.WEEK:
+            multiplier = 7 * 24 * 60 * 60
+        case FunnelConversionWindowTimeUnit.MONTH:
+            multiplier = 30 * 24 * 60 * 60
+        case _:
+            raise ValueError(f"Unsupported conversion window unit: {conversion_window_unit}")
+
+    return conversion_window * multiplier


When I see a switch/case, I think could it be better?. We could use if and make this hilariously recursive:

def conversion_window_to_seconds(conversion_window: int, conversion_window_unit: FunnelConversionWindowTimeUnit) -> int: # Base case if conversion_window_unit == FunnelConversionWindowTimeUnit.SECOND: return conversion_window elif conversion_window_unit == FunnelConversionWindowTimeUnit.MINUTE: return conversion_window_to_seconds(conversion_window * 60, FunnelConversionWindowTimeUnit.SECOND) elif conversion_window_unit == FunnelConversionWindowTimeUnit.HOUR: return conversion_window_to_seconds(conversion_window * 60, FunnelConversionWindowTimeUnit.MINUTE) elif conversion_window_unit == FunnelConversionWindowTimeUnit.DAY: return conversion_window_to_seconds(conversion_window * 24, FunnelConversionWindowTimeUnit.HOUR) elif conversion_window_unit == FunnelConversionWindowTimeUnit.WEEK: return conversion_window_to_seconds(conversion_window * 7, FunnelConversionWindowTimeUnit.DAY) elif conversion_window_unit == FunnelConversionWindowTimeUnit.MONTH: return conversion_window_to_seconds(conversion_window * 30, FunnelConversionWindowTimeUnit.DAY) else: raise ValueError(f"Unsupported conversion window unit: {conversion_window_unit}")

or try to make it shorter with a map:

def conversion_window_to_seconds(conversion_window: int, conversion_window_unit: FunnelConversionWindowTimeUnit) -> int: unit_multipliers = { FunnelConversionWindowTimeUnit.SECOND: 1, FunnelConversionWindowTimeUnit.MINUTE: 60, FunnelConversionWindowTimeUnit.HOUR: 3600, FunnelConversionWindowTimeUnit.DAY: 86400, FunnelConversionWindowTimeUnit.WEEK: 604800, FunnelConversionWindowTimeUnit.MONTH: 2592000, } if conversion_window_unit not in unit_multipliers: raise ValueError(f"Unsupported conversion window unit: {conversion_window_unit}") return conversion_window * unit_multipliers[conversion_window_unit]

and then I realize that sometimes a case is 🆗. But a recursive solution could be fun. Just imagine the face of someone finding that code?

Recursion would be fun. But it felt a little to verbose in this case. I went with the map solution, that felt best. 7656d52 (#30871)

I was hoping for some good ol' recursion :(

rodrigoi

posthog-bot · 2025-04-09T05:16:11Z

📸 UI snapshots have been updated

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

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

posthog-bot · 2025-04-09T05:25:23Z

📸 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 5)
webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

posthog-bot · 2025-04-09T06:59:25Z

📸 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 5)
webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

posthog-bot · 2025-04-09T07:08:40Z

📸 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 5)
webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>

andehen requested a review from a team April 7, 2025 12:50

greptile-apps bot reviewed Apr 7, 2025

View reviewed changes

andehen changed the title ~~feature(experiments): support time units in conversion windows~~ feat(experiments): support time units in conversion windows Apr 7, 2025

rodrigoi reviewed Apr 8, 2025

View reviewed changes

rodrigoi reviewed Apr 9, 2025

View reviewed changes

rodrigoi approved these changes Apr 9, 2025

View reviewed changes

andehen enabled auto-merge (squash) April 9, 2025 05:07

andehen and others added 8 commits April 9, 2025 08:48

WIP conversion window component

a730f70

tweaks

055bd82

upate schema

b87dfda

Update experiment backend with new config format

f2a3f1d

conversion window -> time window

ca67ca1

replace hours with time window

2ec3c6a

Update query snapshots

fd6e09c

use map

1167cc2

andehen force-pushed the experiments/fix-conversion-window-config branch from 109e6b7 to 1167cc2 Compare April 9, 2025 06:49

Update UI snapshots for chromium (5)

dc3f2ad

github-actions bot added 2 commits April 9, 2025 07:02

Update query snapshots

1550094

Update UI snapshots for chromium (5)

38dcc35

andehen merged commit cd1c0d8 into master Apr 9, 2025
109 checks passed

andehen deleted the experiments/fix-conversion-window-config branch April 9, 2025 07:29

hpouillot pushed a commit that referenced this pull request Apr 9, 2025

feat(experiments): support time units in conversion windows (#30871)

2847f0d

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(experiments): support time units in conversion windows #30871

feat(experiments): support time units in conversion windows #30871

andehen commented Apr 7, 2025

greptile-apps bot left a comment

greptile-apps bot Apr 7, 2025

github-actions bot commented Apr 7, 2025 •

edited

Loading

posthog-bot commented Apr 7, 2025

posthog-bot commented Apr 7, 2025

posthog-bot commented Apr 7, 2025

posthog-bot commented Apr 7, 2025

rodrigoi Apr 8, 2025

andehen Apr 9, 2025

rodrigoi Apr 9, 2025

rodrigoi Apr 9, 2025

andehen Apr 9, 2025 •

edited

Loading

rodrigoi Apr 9, 2025

rodrigoi left a comment

posthog-bot commented Apr 9, 2025

posthog-bot commented Apr 9, 2025

posthog-bot commented Apr 9, 2025

posthog-bot commented Apr 9, 2025

feat(experiments): support time units in conversion windows #30871

feat(experiments): support time units in conversion windows #30871

Conversation

andehen commented Apr 7, 2025

Problem

Changes

How did you test this code?

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Apr 7, 2025

Choose a reason for hiding this comment

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

posthog-bot commented Apr 7, 2025

📸 UI snapshots have been updated

posthog-bot commented Apr 7, 2025

📸 UI snapshots have been updated

posthog-bot commented Apr 7, 2025

📸 UI snapshots have been updated

posthog-bot commented Apr 7, 2025

📸 UI snapshots have been updated

rodrigoi Apr 8, 2025

Choose a reason for hiding this comment

andehen Apr 9, 2025

Choose a reason for hiding this comment

rodrigoi Apr 9, 2025

Choose a reason for hiding this comment

rodrigoi Apr 9, 2025

Choose a reason for hiding this comment

andehen Apr 9, 2025 • edited Loading

Choose a reason for hiding this comment

rodrigoi Apr 9, 2025

Choose a reason for hiding this comment

rodrigoi left a comment

Choose a reason for hiding this comment

posthog-bot commented Apr 9, 2025

📸 UI snapshots have been updated

posthog-bot commented Apr 9, 2025

📸 UI snapshots have been updated

posthog-bot commented Apr 9, 2025

📸 UI snapshots have been updated

posthog-bot commented Apr 9, 2025

📸 UI snapshots have been updated

github-actions bot commented Apr 7, 2025 •

edited

Loading

andehen Apr 9, 2025 •

edited

Loading