-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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.
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
[SIP-15] Making client time use UTC as the local time #8450
[SIP-15] Making client time use UTC as the local time #8450
Conversation
8ae21c6
to
3c5a612
Compare
Codecov Report
@@ Coverage Diff @@
## master #8450 +/- ##
=======================================
Coverage 66.58% 66.58%
=======================================
Files 448 448
Lines 22492 22492
Branches 2364 2364
=======================================
Hits 14976 14976
Misses 7378 7378
Partials 138 138
Continue to review full report at Codecov.
|
While I agree UTC is the natural starting point for |
@villebro I agree regarding adding a section to There are three different clocks:
in addition to possible timezone encoding in the data. Unless ISO 8601 times are encoded with the timezone (which can be specified in the time range picker) the time logic can become quite complex especially if all three clocks are not using the same timezone. Currently for the time range picker absolute times are specified using the client clock (or per this PR use UTC as the local time) without a designated timezone. Similarly relative times are evaluated on the Superset server using the server clock without a designated timezone. When the query is run the times are in reference to the local time of the database engine. This can get quite messy if the timezones differ as it’s not apparent (especially for relative times) what the reference point is. |
@@ -951,6 +951,7 @@ export const controls = { | |||
freeForm: true, | |||
label: t('Time range'), | |||
default: t('Last week'), | |||
description: t('The time range for the visualization. Client times are expressed using UTC as the local time.'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wording nit: I prefer The time range for the visualization. Times are in UTC
. Gets the same point across but is much more succinct
fa17578
to
365e534
Compare
@villebro I addressed your comments and updated the tooltip to explicitly mentioned where times were evaluated and how timezones were used. |
365e534
to
121502a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm with one nit
UPDATING.md
Outdated
@@ -23,6 +23,9 @@ assists people when migrating to a new version. | |||
|
|||
## Next Version | |||
|
|||
* [8450](https://github.com/apache/incubator-superset/pull/8450): The time ranger picker | |||
now uses UTC for the tooltips and default placeholder timestamps (san timezone). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spelling nit: sans
PR apache#8450: Making client time use UTC as the local time PR apache#7667: Remove non-UTC epoch logic
PR apache#8450: Making client time use UTC as the local time PR apache#7667: Remove non-UTC epoch logic
* [fix] Making client time UTC * Update UPDATING.md
CATEGORY
Choose one
SUMMARY
Whilst playing with the time range picker I discovered that for relative times ("Last Week" etc.) the reference time (either shown in default or via a tooltip) would use the client timezone rather than the server timezone. This was problematic say at 2019-10-24:18:00:00 PDT the UI would reference
2019-10-24 00:00:00
in various tooltips however when the query was executed it would use2019-10-25 00:00:00
.Initially I was unsure whether we should parse the server timezone to the client but it seems in other places there's the assumption that times are expressed in UTC (irrespective of the server timezone).
Note the time range picker does not explicitly set the timezone by default for absolute dates, i.e., the query will actually run in reference to the timezone of the executing database engine. However one can specify the timezone via the ISO 8601 format. I’m not certain whether Superset should be more opinionated with regards to timezones.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
I added a tooltip to explicitly define where/how times are evaluated.
TEST PLAN
CI.
ADDITIONAL INFORMATION
REVIEWERS
to: @betodealmeida @etr2460 @mistercrunch
cc: @vylc