Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: more tz/date fiddling to get this property thing working #27423

Merged
merged 4 commits into from
Jan 9, 2025
Merged

fix: more tz/date fiddling to get this property thing working #27423

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dd28632
fix: more tz/date fiddling to get this property thing working
pauldambra Jan 9, 2025
011eb29
i think we just need a wider window
pauldambra Jan 9, 2025
cf8ea3b
Update UI snapshots for `chromium` (1)
github-actions[bot] Jan 9, 2025
e7a26e8
Merge branch 'master' into fix/more-tz-fixing
pauldambra Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions frontend/src/scenes/session-recordings/player/sessionRecordingDataLogic.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -684,15 +684,16 @@ export const sessionRecordingDataLogic = kea<sessionRecordingDataLogicType>([
kind: NodeKind.HogQLQuery,
query: hogql`SELECT properties, uuid
FROM events
WHERE timestamp > ${dayjs(earliestTimestamp - 1000)
.utc()
.format('YYYY-MM-DD HH:mm:ss.SSS')}
AND timestamp < ${dayjs(latestTimestamp + 1000)
.utc()
.format('YYYY-MM-DD HH:mm:ss.SSS')}
-- the timestamp range here is only to avoid querying too much of the events table
-- we don't really care about the absolute value,
-- but we do care about whether timezones have an odd impact
-- so, we extend the range by a day on each side so that timezones don't cause issues
WHERE timestamp > ${dayjs(earliestTimestamp).subtract(1, 'day')}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rafaeelaudibert always best to comment on a file or line so we can thread 😊

Don't you need a .format() call to avoid the problem we had previously?

I tried with and without and I don't believe it affects the HogQL query output

We don't want to pass in the number value earliesTimestamp since then CH doesn't use the index

But passing in dayjs object constructed from it seems to get to the same place as formatting ourselves

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, probably because that's the default implementation for toString(). I'd still prefer the format call because I can understand what's actually interpolated. If I look at the code right now I have no idea if we're sending an UNIX timestamp, a UTC string, some ISO string, etc.

It's not blocking, though :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pulling into this tread in one place with @joshsny too 😊

with this dayjs alone I get

greater(toTimeZone(events.timestamp, 'UTC'), '2025-01-08 09:57:56')

if I do dayjs(earliestTimestamp).subtract(1, 'day').utc()

then I get greater(toTimeZone(events.timestamp, 'UTC'), '2025-01-08 09:57:56')

and if I do the format on top then ofc I get whatever I formatted

the value is already UTC... the problem actually comes because HogQL is wrapping the column in the project TZ when I would love to say... this is all under my control and already UTC

I think I prefer passing in the dayjs object to the hogql string cos then we could fix things in one place in that template for all queries that pass a dayjs whereas if we pass strings then we always have to edit everywhere we pass the strings in

but 🤷 it's late and i just want to fix for users for now 🙈 🤣

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I prefer passing in the dayjs object to the hogql string cos then we could fix things in one place in that template for all queries that pass a dayjs whereas if we pass strings then we always have to edit everywhere we pass the strings in

That's a great argument, and I've actually learned something. We're doing that already, that's why you're getting the same thing

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

finally found something confusing about timezones 🫠

AND timestamp < ${dayjs(latestTimestamp).add(1, 'day')}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about utc + formatting, won't this return a dayjs object, or is its default string representation the correct format?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

answered here 😊 https://github.com/PostHog/posthog/pull/27423/files#r1909583592

AND event in ${eventNames}
AND uuid in ${eventIds}`,
}

const response = await api.query(query)
if (response.error) {
throw new Error(response.error)
Expand Down
Loading