Fix hourly time labels dropping an hour on DST transition days - #6626
Fix hourly time labels dropping an hour on DST transition days#6626jaideeppyne wants to merge 1 commit into
Conversation
time_labels/1 built the time:hour labels by adding a fixed number of hours to the naive start of the range, with the count taken from the absolute length of the range. ClickHouse buckets these rows with toStartOfHour(toTimeZone(timestamp, tz)), so on a day that gains or loses an hour the two disagree: the last hour of a spring-forward day gets no label and its traffic drops off the graph, and a nonexistent hour is labelled instead. Walk the local hours of the range and keep the ones that occur in the site timezone.
da29f1d to
c92fcfd
Compare
|
Thanks, @jaideeppyne! The buckets we generate app-side (for the response spine) are wrong indeed. Steps to reproduce
Expected
Actual
Extra info
How does this PR handle queries with an hourly interval that cover days that are 25h long locally? It seems to me we can't handle them properly without making changes in what we query from Clickhouse (~CH). If the data from CH is labeled with local timestamps, and local timestamp is the basis for mapping the CH value to the response spine, then for a sparse CH response returning one item My gut feel is that a full fix would involve
SELECT toStartOfHour(toTimeZone(timestamp, 'Europe/Tallinn')) h,
toDateTime(toUnixTimestamp(h)) d,
uniq(user_id) visitors
FROM events_v2
WHERE site_id = 1
AND name != 'engagement'
AND timestamp >= toDateTime('2025-10-25 21:00:00') -- 2025-10-26 00:00:00 EEST
AND timestamp <= toDateTime('2025-10-26 21:59:59') -- 2025-10-26 23:59:59 EET
GROUP BY h
ORDER BY h[{
"h": "2025-10-26 03:00:00",
"d": "2025-10-26 00:00:00",
"visitors": 219
},
{
"h": "2025-10-26 03:00:00",
"d": "2025-10-26 01:00:00",
"visitors": 196
}] |
|
Thanks, that reading is right. This PR fixes the app-side hourly spine generation, including not inventing nonexistent spring-forward hours and not stopping at 22:00 on 23-hour days, but it does not fully solve the 25-hour fall-back case if result mapping is keyed only by the local timestamp string. For a fall-back day with two I can take either direction from here: broaden this PR to add that CH-side disambiguator and cover both 23h and 25h days end-to-end, or narrow this PR to the spring-forward/dropped-bucket case and leave the fall-back ambiguity for a separate change. |
|
@jaideeppyne Thanks for your quick reply! Of the two, I'd personally prefer the comprehensive solution. I think the ideal solution highlights how important it is to build the response spine correctly app-side if we don't want to use WITH FILL Clickhouse-side. At the moment and even after this PR, it seems to fly under the radar as labels-related, when in reality, as you found, invalid buckets are injected and legitimate buckets returned by CH are dropped in-app. It's a significant change though and work will likely spill over to comparisons. Comparisons are a bit of a sore spot with difficult decisions. For example, when comparing hourly visitors for the last 7 days with hourly visitors from day -15 to day -8, and one of those periods has more or less hours than the other, how to align the comparison? TBH I'm actually curious if it'd be possible to get CH to do the spine using WITH FILL. It would eliminate a class of errors completely. If most of the stats requests are not sparse, are increased HTTP payloads between CH / app from use of WITH FILL comparable to requesting an additional disambiguating timestamp column? |
|
Agreed, I’ll broaden this beyond a label-only fix. I’ll first check whether ClickHouse WITH FILL can generate the hourly spine correctly for the requested timezone without making sparse series/comparison payloads worse. If that does not hold up, I’ll switch the app-side mapping to a stable absolute bucket key and keep the local hour label display-only, with comparison behavior called out explicitly rather than hidden in this PR. |
Changes
On a day that a site's timezone moves its clocks forward, the hourly graph silently loses the last hour of the day.
time_labels/1built thetime:hourlabels by adding0..nhours to the naive start of the range, wherencame fromDateTime.diff(last, first, :hour). That diff is absolute, so a 23 hour day yields 23 labels ending at 22:00. ClickHouse buckets the rows withtoStartOfHour(toTimeZone(timestamp, tz)), which does produce a 23:00 bucket, so that row has no label to land on. The same range also emits a label for 02:00, an hour that never happened. Fall-back days get the opposite: 25 labels for 24 buckets, the last one belonging to the next day.New York, 2024-03-10, before:
after:
I found this with a property check: every hour bucket ClickHouse can produce for a range must have exactly one label, and no label may exist for an hour that doesn't. I used
Tzdataas the oracle for what the local hours of a day really are. Sweeping all 596 zones over every day from 2015 to 2030 gives 3,483,024 (zone, day) pairs; 6,284 of them were wrong before, 0 after. Both directions of the bug show up in every zone that observes DST, andPacific/ChathamandAntarctica/Caseywere also wrong because their transitions aren't on the hour.time:minutealready handles this correctly, and the day/week/month labels are pureDatearithmetic, so this only touchestime:hour.Scope I did not cover: a sub-day window that straddles a backwards transition in a zone whose transitions aren't on the hour can still miss one bucket. In practice that is
Pacific/Chathamon its April switch, reachable via "Last 24 Hours" or a custom API range. Fixing it properly needs the bucket-to-range overlap test rather than a naive hour walk, which is a lot more code. Happy to add it if you want it covered.The three new tests fail on master and pass here. I also ran
test/plausible/stats/,main_graph_test.exsandtest/plausible_web/controllers/api/external_stats_controller/before and after: identical results, no new failures.I used an AI assistant to help write the property-check harness and this patch. I ran everything and checked the results myself.
Tests
Changelog
Documentation
Dark mode