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.

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

Merged
merged 2 commits into from
Oct 28, 2019

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Oct 25, 2019

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

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 use 2019-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.

Screen Shot 2019-10-26 at 6 17 08 PM

TEST PLAN

CI.

ADDITIONAL INFORMATION

REVIEWERS

to: @betodealmeida @etr2460 @mistercrunch
cc: @vylc

@john-bodley john-bodley force-pushed the john-bodley--sip-15-client-utc branch 2 times, most recently from 8ae21c6 to 3c5a612 Compare October 25, 2019 04:52
@codecov-io
Copy link

codecov-io commented Oct 25, 2019

Codecov Report

Merging #8450 into master will not change coverage.
The diff coverage is 66.66%.

Impacted file tree graph

@@           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
Impacted Files Coverage Δ
superset/assets/src/explore/controls.jsx 41.26% <ø> (ø) ⬆️
.../explore/components/controls/DateFilterControl.jsx 37.9% <66.66%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eaeed0c...121502a. Read the comment docs.

@villebro
Copy link
Member

villebro commented Oct 25, 2019

While I agree UTC is the natural starting point for now, there will be many people relying on the current behaviour. So there should at least be a mention in UPDATING.md informing users of the change. An added bonus would be making this optional, i.e. being able to choose whether or not the reference point is local or UTC time, but that would require more backend -> frontend plumbing, so might cause unnecessary clutter in the codebase.

@john-bodley
Copy link
Member Author

john-bodley commented Oct 25, 2019

@villebro I agree regarding adding a section to UPDATING.md. The timezone logic is possibly more nuances especially if we’re talking about non-explicit timezone formats, i.e., not epoch which is in reference to UTC.

There are three different clocks:

  1. The database engine
  2. The Superset server
  3. The Superset client

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.'),
Copy link
Member

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

@john-bodley john-bodley changed the title [SIP-15] Making client time UTC [SIP-15] Making client time use UTC as the local time Oct 25, 2019
@pull-request-size pull-request-size bot added size/M and removed size/S labels Oct 27, 2019
@john-bodley
Copy link
Member Author

@villebro I addressed your comments and updated the tooltip to explicitly mentioned where times were evaluated and how timezones were used.

Copy link
Member

@etr2460 etr2460 left a 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).
Copy link
Member

Choose a reason for hiding this comment

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

spelling nit: sans

@john-bodley john-bodley merged commit f7f0be5 into apache:master Oct 28, 2019
@john-bodley john-bodley deleted the john-bodley--sip-15-client-utc branch October 28, 2019 20:05
shmsr added a commit to ThalesGroup/incubator-superset that referenced this pull request Nov 11, 2019
PR apache#8450: Making client time use UTC as the local time
PR apache#7667: Remove non-UTC epoch logic
shmsr added a commit to ThalesGroup/incubator-superset that referenced this pull request Nov 11, 2019
PR apache#8450: Making client time use UTC as the local time
PR apache#7667: Remove non-UTC epoch logic
shmsr added a commit to ThalesGroup/incubator-superset that referenced this pull request Nov 13, 2019
graceguo-supercat pushed a commit that referenced this pull request Nov 13, 2019
* [fix] Making client time UTC

* Update UPDATING.md
@dpgaspar dpgaspar added v0.35 and removed v0.35 labels Dec 20, 2019
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 labels Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 0.36.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants