Enhance Date Interval Handling and Add "Yesterday" Interval Support #98
Conversation
|
|
|
Should I have made this on the v2 route? |
|
Thanks for the PR. Sorry, I've been travelling for the past few weeks and haven't had an opportunity to review it.
Nope – you're good. That branch became |
app/analytics/query.ts
Outdated
| while (startDateTime.getTime() < Date.now()) { | ||
| let endIntervalDate = Date.now(); | ||
| if (interval === "yesterday") { | ||
| endIntervalDate = startDateTime.getTime() + 25 * 60 * 60 * 1000; |
There was a problem hiding this comment.
There's potentially a bug here: you can't rely on +24 hours math to get endIntervalDate, because in timezones where daylight savings exists, it might actually be +23 hours or +25 hours on days when it changes.
But turns out, I thought dayjs handled this but apparently it does not: iamkun/dayjs#1271. So this is likely an issue everywhere.
Still, it might be a good idea to just use dayjs to give you the explicit "end of day" time, in case this is ever patched, so that it matches the calculations elsewhere.
There was a problem hiding this comment.
Might be worth adding test cases for daylight savings. I'll look into the workaround mentioned in that PR and see where I can apply it
There was a problem hiding this comment.
I changed it to use dayjs
localDateTime = localDateTime.tz(tz).startOf("day").subtract(1, "day");
localEndDateTime = localDateTime.endOf("day").add(2, "ms");
app/analytics/query.ts
Outdated
| @@ -188,6 +203,7 @@ export class AnalyticsEngineAPI { | |||
|
|
|||
| // note interval count hard-coded to hours at the moment | |||
| const initialRows = generateEmptyRowsOverInterval( | |||
| interval, | |||
There was a problem hiding this comment.
It strikes me that you're passing in interval into a few functions to strictly do if (interval) === "yesterday".
I think it would be better to just pass an optional endDateTime, just as startDateTime is being passed. Basically, determine the value in dashboard.tsx and pass it in. That way any arbitrary endDateTime could be used.
Thoughts?
There was a problem hiding this comment.
That's a good point. I'll refactor
app/analytics/query.ts
Outdated
|
|
||
| const filterStr = filtersToSql(filters); | ||
|
|
||
| const _column = ColumnMappings[column]; | ||
| const query = ` | ||
| SELECT ${_column}, SUM(_sample_interval) as count | ||
| FROM metricsDataset | ||
| WHERE timestamp > ${intervalSql} | ||
| WHERE timestamp < ${endIntervalSql} AND timestamp >= ${startIntervalSql} |
There was a problem hiding this comment.
Nit: I notice in some inconsistency in these WHERE clauses where sometimes you start the statement off with timestamp >= ${startIntervalSql} and sometimes you start them off with timestamp < ${endIntervalSql} (for example L231).
I'd prefer if it was consistent all the way through, starting with the startInterval boundary, and ending with the endInterval boundary.
There was a problem hiding this comment.
Nice catch! I have switched them around
|
Going to merge this. As a follow on, IMO, the query should be both |
|
Thank you @DylanPetrey! |
Refs #3
Description:
This pull request introduces several improvements and new features:
1. Enhanced Date Interval Handling
intervalToSqlfunction to return both start and end interval SQL for better accuracy.2. "Yesterday" Interval Support
Changes:
app/analytics/__tests__/query.test.tsapp/analytics/query.tsapp/routes/dashboard.tsxpackage-lock.json