Repository navigation
fix: query elements from start of day - #9827
Conversation
|
the most recent five elements queries from PostHog cloud at some point on 17th May showed date_from as
so would not always match the equivalent Trend graph |
mariusandra
left a comment
mariusandra
left a comment
There was a problem hiding this comment.
Basically looks good, though you many need to add if _ != "": (or something nicer) to avoid triggering sentry
| _, date_to, date_params = parse_timestamps(filter, team=self.team) | ||
| date_from = date_from_clause("toStartOfDay", True) |
There was a problem hiding this comment.
There's too much magic happening behind those two lines for me to be able to confidently approve them. It's the first time I see this code, and I'm not sure of all the places it can be called. However, just following the thread, how do we know in date_from_clause that params['date_from'] is always a thing? I fear we might get unexpected sentry errors...
There was a problem hiding this comment.
We can blame @neilkakkar if it does go wrong 🤣
There was a problem hiding this comment.
More seriously, I'm pretty confident that the flow through this code is safe... Plus it's easy to revert if it does cause errors :)
There was a problem hiding this comment.
I'd guess the place where things can go wrong in either case (trends/stats) is if how the API being called is different? For example, we're reasonably sure the trends API wouldn't bugger out because the primary caller is our code which always puts in a date_from. Idk if we can say the same for stats ( haven't looked for call sites )
There was a problem hiding this comment.
Yep, it's only called from toolbar heatmap. The default case is that it isn't sent - in which case it defaults to seven days ago
* master: chore: start stack once in cloud tests (#9879) feat(apps): frontend apps (#9831) chore: Fix snapshots on master (#9885) chore(apps): rename plugins to apps (#9755) refactor: Remove constance library dependency, use json-encoded model (#9852) chore(clickhouse): avoid creating kafka_events, events_mv (#9863) fix(insights): Fix timezone date issues (#9678) refactor(plugin-server): refactor the event pipeline (#9829) feat(object storage): add unused object storage (#9846) fix: make kafka health check timeout test reliable (#9857) fix: query elements from start of day (#9827)
Problem
see #9770 - doesn't fix this issue by itself, but should help
when loading a trend view of an action (which in this case tracks clicks) we query including a
date_fromfilter likewhen loading the elements for the toolbar's heat map we query including a
date_fromfilter likewithout the
toStartOfDayClickHouse functionrunning this for a particular test action gave 72 results for the trend and 69 for the heat map
Changes
ensures the api/elements/stats endpoint queries from the start of the day
How did you test this code?
adding a unit test for the new behaviour