feat: Add human friendly comparison periods toggle #27176
Conversation
|
https://github.com/PostHog/posthog/actions/runs/12520844708/job/34927105170?pr=27176 is failing but this is wrong. We can safely add a non-null field with a default on Postgres v11+ - which we are. See https://www.reddit.com/r/programming/comments/bw0vgm/psa_postgresql_11_allows_you_to_create_a_nonnull/ How can we sort this out? |
rafaeelaudibert
force-pushed
the
human-friendly-month-and-year-comparison-periods
branch
from
0460a11 to
a298ea9
Compare
rafaeelaudibert
force-pushed
the
human-friendly-month-and-year-comparison-periods
branch
from
455be0d to
603eeba
Compare
This toggle in the backend allows us to do "human friendly" comparisons, i.e. consider month as 52 weeks and month as 4 weeks. This makes the comparison table slightly nicer to look at, at the cost of not aligning 100% of the time because a month = 30d and a year=365s are slightly more than 4 weeks and 52 weeks, respectively Follow-up commit will add the toggle to the UI
You can now toggle that in the UI on `settings/environment-product-analytics#human-friendly-comparison-periods`
rafaeelaudibert
force-pushed
the
human-friendly-month-and-year-comparison-periods
branch
2 times, most recently
from
3e3488f to
fba4e2f
Compare
rafaeelaudibert
force-pushed
the
human-friendly-month-and-year-comparison-periods
branch
from
fc76378 to
06711be
Compare
Postgres 11+ can easily add a new non-nullable column with a default using nothing but an extremely short lock. https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=16828d5c0273b4fe5f10f42588005f16b415b2d8
rafaeelaudibert
force-pushed
the
human-friendly-month-and-year-comparison-periods
branch
from
24cf9fb to
2f4bf70
Compare
📸 UI snapshots have been updated16 snapshot changes in total. 0 added, 16 modified, 0 deleted:
Triggered by this commit. |
We can't set a default at the DB level in Django 4.2. I really don't understand how this is possible. Rails >>>>>>>> Django. So, to workaround that, we'll guarantee that we're converting the value to a boolean before sending it to our conversion functions.
|
It's not enough for CI to be flaky, GitHub is also failing |
|
Size Change: 0 B Total Size: 1.11 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Period handling code LGTM
See the comment about disabling the warning about adding non-null columns
On the product side - I'm a bit iffy on this. I wonder if we could make the customer happy by changing the default date dropdown from 30 days -> 28 days and adding a 52-week option under 1 year. Wdyt?
posthog/settings/data_stores.py
Outdated
| @@ -55,6 +55,13 @@ def postgres_config(host: str) -> dict: | |||
| } | |||
|
|
|||
|
|
|||
| # Skip the check for non-nullable fields with default because we're on PG11+ | |||
There was a problem hiding this comment.
Is there a link to a longer discussion on this? I've checked the docs, and it should be true, but the cost of getting it wrong is an outage, so the bar for merging needs be higher than my PR approval. If there's a thread where there's consensus from people with more postgres/django experience than me, then go ahead :) though I'd strongly consider making this change a standalone PR so that it's easier to refer back to if there's a problem
There was a problem hiding this comment.
Actually, this isn't needed anymore, I'll remove this. Django 4 does not allow us to do this, but I'll be a very happy man once we can upgrade to Django 5.
As for reference on why this is safe, I hope anecdotal evidence is enough: I've done it hundreds (literally) of times before. I checked in Slack and only Michael replied and he was ok with it.
Anyway, this is not needed for now, we can worry about doing this in a safer way once we are on Django 5.
This won't be needed as we can't create a non-null default column in Django 4, so there's no point in disabling the warning if it won't work at all.
🎉
I've removed it, that wasn't necessary.
I dislike changing the default value because this would change all of the existing insights behaviour. Also, it might make sense to do 30/31-day months sometimes. I don't think giving customers more options is ever a bad idea (well, maybe there are cases, but I don't think this is one of them). My only concern with my approach is that maybe we should allow people to choose this at the date selector level rather than the project level, but we can update this if we ever hear complaints about it. Thoughts on merging this as is? I don't think many people will even be using this, but it'll be helpful for Purple Wave |
rafaeelaudibert
deleted the
human-friendly-month-and-year-comparison-periods
branch
* master: (659 commits) fix: more tz/date fiddling to get this property thing working (#27423) fix(hogql): support virtual tables with lazy tables (#27404) fix: Removing target options for survey resets its value to remove validation error (#27139) chore: Use `actions/{download,upload}-artifact@v4` (#27413) feat(clickhouse): when HTTP pass ca_cert and verify to pool manager (#27399) fix: typo in dashboard template configuration (#27417) fix(experiments): Force refresh when `start_date` is provided (#27396) feat(data-warehouse): Reset the pipeline source files when resync is selected on the frontend (#27402) chore(data-warehouse): Added SSL error as a non retryable error (#27395) chore(data-warehouse): Upgrade deltalake package (#27393) chore(data-warehouse): Kill the delta subprocess if need be (#27392) feat: Add P75 quantile (#27409) fix: property labels need to be known about in the backend but are defined in the front end (#27328) fix(editor-3001): show header on mobile for editor (#27373) chore: Enable web vitals capture (#27394) feat: Add human friendly comparison periods toggle (#27176) feat: promote data warehouse from taxonomic filter component (#27364) chore(experiments): Improve result state resetting (#27391) fix: Track records completed in heartbeat (#26686) fix(data-warehouse): Handle NaN values from SQL sources (#27360) ...
Problem
Some customers have seasonal data, where data repeats on specific days of the month/year and they'd like to compare against them. The problem is that if you set "compare against last year" but look at data day to day, then it will be misaligned because the days of the week aren't the same year after year.
Changes
This toggle allows us to do "human friendly" comparisons, i.e. consider month as 52 weeks and month as 4 weeks. This makes the comparison table slightly nicer to look at, at the cost of not aligning 100% of the time because a month = 30d and a year=365s are slightly more than 4 weeks and 52 weeks, respectively
I've put the configuration inside "Product Analytics" rather than "Web Analytics" because this applies globally - it's a team-based setting.
Does this work well for both Cloud and self-hosted?
Yep
How did you test this code?
Automated tests + manual UI testing