Fix hourly bucket mapping across DST transitions - #6626
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. |
|
Implemented the comprehensive follow-up in Hourly event/session queries now return an absolute Unix bucket key in the existing dimension. It remains the lookup key through sparse result mapping and comparisons; local timestamps are display-only. The per-day response spine matches ClickHouse buckets across 23h/25h days, multi-day ranges and partial-hour transitions. Legacy timeseries, sparklines and CSV exports now use result indices too. For comparisons, this preserves positional/elapsed-bucket alignment rather than silently switching to wall-clock alignment. Unequal series lengths remain intact, and an extra comparison bucket keeps its metrics with I also tested plain Validation: 41 focused time/DST tests pass. The broader statistics/main-graph/CSV/external-API run passed 1,346 of 1,347 tests; the remaining journey-exploration failure also reproduces on the original |
Changes
Fix hourly bucket identity and the response spine together, including sparse data on 23-hour and 25-hour days and multi-day ranges.
Comparison behavior
Preserve the existing positional alignment: bucket 0 compares with bucket 0, bucket 1 with bucket 1, and so on. This is elapsed-bucket alignment, not same-wall-clock-hour alignment after a DST transition. Both series keep their own length; an unmatched final comparison bucket retains its metrics and has
change: nil. The frontend already uses the longer series length.Why not plain WITH FILL?
A single
WITH FILL STEP toIntervalHour(1)over a multi-day range does not necessarily matchtoStartOfHouraround half-hour offset changes. In a local ClickHouse 26.7.5.10 experiment spanning October 6-7, 2024 in Australia/Lord_Howe, a sparse fill produced 49 buckets, including 24 spurious half-hour buckets on October 7; aggregating minute samples produced 48 buckets and no such October 7 buckets.This implementation leaves metric rows sparse and replaces the existing dimension value rather than adding a second timestamp column. It does not claim a production bandwidth benchmark or rule out a more elaborate ClickHouse-generated spine.
Validation
Prepared with AI assistance; these are tool-run results, not a claim of independent human review.
ExplorationTestatexploration_test.exs:781, reproduces unchanged on the original PR headc92fcfdin the same environment.git diff --checkpass.The earlier Tzdata-only exhaustive-sweep claim is not used as evidence for this revision: the relevant oracle is ClickHouse's actual bucket behavior.
Tests
Changelog
Documentation
Dark mode