-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Correctly apply timezone to formatted dates and ticks #33831
Conversation
💔 Build Failed |
💚 Build Succeeded |
Pinging @elastic/kibana-app |
39c5b60
to
eb0088d
Compare
💔 Build Failed |
💚 Build Succeeded |
Thanks @markov00 for your nice solution to the problem! |
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.
code lgtm
💚 Build Succeeded |
💔 Build Failed |
💔 Build Failed |
Seems like unrelated failure, Jenkins, test this please. |
💚 Build Succeeded |
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.
Tested locally with an index that spans 5 years, using an area chart, date histogram with yearly interval.
I've tested using different combinations of timezone between the local system timezone and the kibana configured timezione.
Code LGTM
@flash1293 I am not sure if this is working on BC2 of 7.0.1: |
@bhavyarm There are some cases which unfortunately didn't get fixed (if the start of the time range is in another time zone that the ticks like it currently is the case for last 5 years, 2014-12-12 to 2019-12-12 shouldn't have this problem) I opened a follow up PR for that which should fix the remaining cases: #35577 |
Summary
Fixes #32252.
This PR correctly applies the current timezone to the date formatter and places the ticks on the axes accordingly to the local timezone instead of utc.This works in almost all cases except for the case when the timezone configured in Kibana is west of the local timezone of the browser, then the ticks are placed e.g. at the very end of the last year which looks like in the description of the bug.There doesn't seem to be an easy way to get d3 to place the ticks accordingly to a specific timezone, so maybe we should just merge this fix as it improves the current behavior (the bug gets triggered every time the local timezone is west of utc and configured timezone is ignored completely) and wait forelastic-charts
which shouldn't have this problem.New solution: d3 places the ticks to "nice" points in time (e.g. at the start of the day or at the very start of the year), but it does so for nice points in utc. If the current timezone of Kibana is something else than utc, the ticks are shifted away form the nice points in time - they are positioned correctly on the axis, but depending on the formatting this can be confusing, e.g. if a chart uses a date histogram agg with yearly intervals. In this case the ticks are e.g. positioned on
2018 Jan 1 00:00:00 UTC
, which is2017 Dec 31 18:00:00 US/East
- the tick formatter throws away everything but the year and places a tick2017
very close to the data point of the 2018 bucket - technically it's half of a pixel or something left of the data point and the tick formatter correctly shows 2017, but the user thinks that the tick describes the position of the data point. This PR fixes that by shifting the range which is passed to the axis by the UTC offset before the tick placement (which places them to nice points in UTC) and then shifting the resulting ticks back by the offset to get "nice" tick positions in your current timezone.This is illustrated in the functional test which had to be adjusted because of this (https://github.com/elastic/kibana/pull/33831/files#diff-eab114dfd01d64c5c74ffcde74a70afb) - before the fix, the ticks were at really strange positions in a US timezone, now they are neatly placed at
12:00
and00:00
o'clock.ping @markov00 @timroes
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers