feat(web-analytics): Allow customers to compare against a different period #26820

rafaeelaudibert · 2024-12-11T04:44:25Z

Changes

Right now, we'd always compare our data against the last period, i.e. if we're looking at the data from the last 14 days, then you'd always compare it against the previous 14 days.

This is not always ideal, as someone might want to compare against numbers from the same time last year.

We've added the comparator component in a previous commit (hidden behind a FF) and we're now actually using that data in the backend.

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

Yep

How did you test this code?

Manual UI tests
Backend tests

frontend/src/scenes/web-analytics/webAnalyticsLogic.tsx

github-actions · 2024-12-11T04:55:40Z

Size Change: 0 B

Total Size: 1.11 MB

ℹ️ View Unchanged

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

_{compressed-size-action}

posthog-bot · 2024-12-12T19:46:15Z

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

frontend/__snapshots__/scenes-other-onboarding--onboarding-billing--light.png

posthog-bot · 2024-12-12T20:05:37Z

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

…eriod Right now, we'd always compare our data against the last period, i.e. if we're looking at the data from the last 14 days, then you'd always compare it against the previous 14 days. This is not always ideal, as someone might want to compare against numbers from the same time last year. We've added the comparator component in a previous commit (hidden behind a FF) and we're now actually using that data in the backend.

Use same methods as we're using on `stats_table` to calculate previous metrics. We've extracted this up to `web_analytics_query_runner`

This is breaking some of our code and it's also a bad default

robbie-c

I got this error on the paths insight when I went from "no comparison" to "compare to previous period"

robbie-c · 2024-12-13T19:26:59Z

mypy-baseline.txt

@@ -1,67 +1,4 @@
 posthog/warehouse/models/ssh_tunnel.py:0: error: Incompatible types in assignment (expression has type "NoEncryption", variable has type "BestAvailableEncryption")  [assignment]
-posthog/temporal/data_imports/pipelines/sql_database_v2/schema_types.py:0: error: Statement is unreachable  [unreachable]


this is a huge diff! I couldn't spot a difference, but I guess there probably was one?

There was a single difference. I really don't know why everything was reordered 🤔

Yeah fair enough - don't worry too much about it. In general we should try to reduce this list but IMO we don't need to be too strict

robbie-c · 2024-12-13T19:41:07Z

posthog/hogql_queries/utils/query_previous_period_date_range.py

@@ -15,13 +15,13 @@ class QueryPreviousPeriodDateRange(QueryDateRange):
    """Translation of the raw `date_from` and `date_to` filter values to datetimes."""

    _team: Team
-    _date_range: Optional[InsightDateRange]
+    _date_range: Optional[InsightDateRange | DateRange]


Q: Why are these different?

A: They look the same except for defaults

class InsightDateRange(BaseModel): model_config = ConfigDict( extra="forbid", ) date_from: Optional[str] = "-7d" date_to: Optional[str] = None explicitDate: Optional[bool] = Field( default=False, description=( "Whether the date_from and date_to should be used verbatim. Disables rounding to the start and end of" " period." ), ) class DateRange(BaseModel): model_config = ConfigDict( extra="forbid", ) date_from: Optional[str] = None date_to: Optional[str] = None explicitDate: Optional[bool] = Field( default=False, description=( "Whether the date_from and date_to should be used verbatim. Disables rounding to the start and end of" " period." ), )

Is there any reason not to use InsightDateRange?

Basically because of the default. Insights want to set 7 days as default, we need it to be empty by default - there are a lot of assumptions behind that. I'll change it to have no default to improve the code readability, we can always revert it

posthog/hogql_queries/web_analytics/external_clicks.py

rafaeelaudibert · 2024-12-16T17:29:48Z

I got this error on the paths insight when I went from "no comparison" to "compare to previous period"

@robbie-c I think this is happening because you have some cached data, I can't reproduce it 🤔.

I've added a failsafe to guarantee that doesn't happen, though.

These are basically the same, the only difference is the default. Let's use `DateRange` without the default everywhere. If this proves to be a problem (too many broken tests) we can either:? 1. revert this 2. try to use it with the default on the places where we were not using a default before fix: Remove last references to `InsightDateRange`

robbie-c

rafaeelaudibert requested a review from robbie-c December 11, 2024 04:44

rafaeelaudibert commented Dec 11, 2024

View reviewed changes

frontend/src/scenes/web-analytics/webAnalyticsLogic.tsx Outdated Show resolved Hide resolved

Base automatically changed from display-stats-from-conversion-goals-only-2 to master December 11, 2024 16:08

rafaeelaudibert force-pushed the implement-comparison-to-complex-past-period branch from 7164e68 to fd09fea Compare December 12, 2024 19:29

rafaeelaudibert commented Dec 12, 2024

View reviewed changes

frontend/__snapshots__/scenes-other-onboarding--onboarding-billing--light.png Outdated Show resolved Hide resolved

rafaeelaudibert force-pushed the implement-comparison-to-complex-past-period branch from 3413f94 to 18ca9a9 Compare December 12, 2024 21:19

rafaeelaudibert marked this pull request as ready for review December 12, 2024 21:23

rafaeelaudibert and others added 8 commits December 13, 2024 12:40

Stop displaying unrelated-to-conversion-goal sections

cf6d55d

refactor: Simplify web_overview

97b6d64

Use same methods as we're using on `stats_table` to calculate previous metrics. We've extracted this up to `web_analytics_query_runner`

Implement comparison for Outbound clicks

c3b64da

test: Add missing tests for comparison

b51585f

fix: fix mypy error

6a869bb

fix: Remove default of -7d for comparefilter

3060585

This is breaking some of our code and it's also a bad default

fix: Regenerate mypy baseline

346e3ed

rafaeelaudibert force-pushed the implement-comparison-to-complex-past-period branch from 1ebdc47 to 346e3ed Compare December 13, 2024 17:01

test: Add new entry to insights default fields

7ac990d

rafaeelaudibert enabled auto-merge (squash) December 13, 2024 18:29

robbie-c reviewed Dec 14, 2024

View reviewed changes

rafaeelaudibert added 3 commits December 16, 2024 15:43

fix: Use session timestamp rather than first click timestamp

f2a8849

fix: Guarantee we wont break if called with empty number

a424b59

rafaeelaudibert force-pushed the implement-comparison-to-complex-past-period branch from cba8a94 to a424b59 Compare December 16, 2024 18:43

fix: fix broken test

581f895

rafaeelaudibert requested a review from robbie-c December 16, 2024 21:50

robbie-c approved these changes Dec 17, 2024

View reviewed changes

rafaeelaudibert merged commit 22cd345 into master Dec 17, 2024
96 checks passed

rafaeelaudibert deleted the implement-comparison-to-complex-past-period branch December 17, 2024 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(web-analytics): Allow customers to compare against a different period #26820

feat(web-analytics): Allow customers to compare against a different period #26820

rafaeelaudibert commented Dec 11, 2024 •

edited

Loading

github-actions bot commented Dec 11, 2024 •

edited

Loading

posthog-bot commented Dec 12, 2024

posthog-bot commented Dec 12, 2024

robbie-c left a comment

robbie-c Dec 13, 2024 •

edited

Loading

rafaeelaudibert Dec 16, 2024

robbie-c Dec 17, 2024

robbie-c Dec 13, 2024

robbie-c Dec 14, 2024

robbie-c Dec 14, 2024

rafaeelaudibert Dec 16, 2024 •

edited

Loading

rafaeelaudibert commented Dec 16, 2024

robbie-c left a comment

		@@ -1,67 +1,4 @@
		posthog/warehouse/models/ssh_tunnel.py:0: error: Incompatible types in assignment (expression has type "NoEncryption", variable has type "BestAvailableEncryption") [assignment]
		posthog/temporal/data_imports/pipelines/sql_database_v2/schema_types.py:0: error: Statement is unreachable [unreachable]

feat(web-analytics): Allow customers to compare against a different period #26820

feat(web-analytics): Allow customers to compare against a different period #26820

Conversation

rafaeelaudibert commented Dec 11, 2024 • edited Loading

Changes

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

How did you test this code?

github-actions bot commented Dec 11, 2024 • edited Loading

posthog-bot commented Dec 12, 2024

📸 UI snapshots have been updated

posthog-bot commented Dec 12, 2024

📸 UI snapshots have been updated

robbie-c left a comment

Choose a reason for hiding this comment

robbie-c Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

rafaeelaudibert Dec 16, 2024

Choose a reason for hiding this comment

robbie-c Dec 17, 2024

Choose a reason for hiding this comment

robbie-c Dec 13, 2024

Choose a reason for hiding this comment

robbie-c Dec 14, 2024

Choose a reason for hiding this comment

robbie-c Dec 14, 2024

Choose a reason for hiding this comment

rafaeelaudibert Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

rafaeelaudibert commented Dec 16, 2024

robbie-c left a comment

Choose a reason for hiding this comment

rafaeelaudibert commented Dec 11, 2024 •

edited

Loading

github-actions bot commented Dec 11, 2024 •

edited

Loading

robbie-c Dec 13, 2024 •

edited

Loading

rafaeelaudibert Dec 16, 2024 •

edited

Loading