Skip to content
This repository was archived by the owner on Jan 31, 2018. It is now read-only.

[Bug 799691] Use UTC, not local timezone.#52

Closed
mythmon wants to merge 1 commit intomozilla:masterfrom
mythmon:align-x-799691
Closed

[Bug 799691] Use UTC, not local timezone.#52
mythmon wants to merge 1 commit intomozilla:masterfrom
mythmon:align-x-799691

Conversation

@mythmon
Copy link
Contributor

@mythmon mythmon commented Oct 23, 2012

This makes the graph lines and the graph data points line up, and since we are only showing days, and not times, has no effect on the perceived data. It just makes things like up nicely.

@willkg
Copy link
Member

willkg commented Oct 23, 2012

So, one thing I did for some code I wrote recently was this:

        calendar.timegm(time_struct) * 1000, max(datapoints[key])

Which takes a time_struct, does the timegm thing on it which converts it to GMT and seconds, and multiply that by 1000 because flot likes milliseconds.

That gets me in the correct day.

Are the time values we're using in GMT or Pacific Time? Can data points end up in the wrong day slot because we're not converting to GMT?

@mythmon
Copy link
Contributor Author

mythmon commented Oct 23, 2012

All the datetimes are done on the server, not the client, so should always be UTC. Additionally, the timestamps used for the histogram come not from the data, but from elasticsearch, so they should also be in UTC. The problem in this pull request is that for some reason when I made the histogram originally, I told it to use local time on the client, instead of UTC. So the timestamps sent by elastic were 7 or 4 hours off, depending on your local time zone. Telling flot to fall back to it's default of UTC makes it draw the grid lines on midnights of UTC days, instead of local days, which lines up with the data elasticsearch generated.

In other words: no, the data should never be stored in pacific time, or any other timezone accept UTC/GMT, and the points on the graph should not end up wrong, unless the settings for elastic search change.

It is possible that the individual opinions might have the wrong timestamps, under the condition that the webserver they are collected on have a system timezone of something besides UTC (such as a development laptop). This will not cause the points on the graph to become misaligned, it will only cause the opinions to be lumped into the wrong day in the histogram. This should not happen in product, because the server's timezone should be set to UTC.

@mythmon
Copy link
Contributor Author

mythmon commented Oct 23, 2012

So, today I learned: Mozilla servers use PST. Awesome. However, for some reason this still fixes the issue on my PST running laptop. I'm not totally sure why this is, but I'm going to dig a bit.

brb, investigating 🔎

@jsocol
Copy link

jsocol commented Oct 23, 2012

Mozilla servers use PST.

Strictly speaking, and for posterity, this isn't correct. Technically, they use the "America/Los_Angeles" timezone, meaning they observe DST. So, today it's PDT. In two weeks it'll be PST. :neckbeard:

YAY. ┻━┻ ︵ ¯(ツ)/¯ ︵ ┻━┻

@mythmon
Copy link
Contributor Author

mythmon commented Oct 24, 2012

Having done some research, I can confirm that everything is storing timestamps in localtime, and that somehow the histogram still outputs in UTC. It is still splitting things right though, so it's all OK.

I would like to switch to forcing everything into UTC at some point, instead of just relying on localtime playing nice, but not as part of this bug.

@mythmon
Copy link
Contributor Author

mythmon commented Oct 24, 2012

So are there any objections to merging this small change? It changes flot to plot things in UTC. This is because the data that comes from elastic in the case of the histogram is going to be UTC, even on a Pacific time server.

This should not need to be reverted, even if we decide to force a particular time zone later, as elastic is still going to be returning UTC in this case.

r?

(On a side note, it makes me quite sad that @jsocol's unicode art doesn't render well here.)

@willkg
Copy link
Member

willkg commented Oct 24, 2012

We talked about this last night. What's going on is that you removed the code that was telling Flot to translate the timestamps from UTC to whatever timezone the browser was in. Since it's no longer converting timestamps, the data points are lining up with the lines. However, the data points and what the graph is showing is Pacific Time--not UTC.

Because of that, the commit message is wrong, but otherwise I'm ok with landing this especially now that I wrote up the bug to figure out what to do with the situation.

This makes the graph lines and the graph data points line up. This only
changes flot's expectations to match what elastic is ending it.

When elasticsearch returns histogram data with a period of 1 day (like
we are doing on the histogram) the timestamps it returns have the hours
of a UTC midnight date, regardless of what timezone the aggregate set
represents.

This means that when the server is working in Pacific time, it
aggregates a Pacific day worth of data, sums it up, and then returns it
labeled with a timestamp. This timestamp, when output, is the midnight
of a UTC day (calculating the hours past midnight only returns 0 if you
assume UTC), even if localtime on the server is Pacific.

Even if we change to forcing a particular timezone in the future, this
will still be true, and elasticwill still label this data with UTC,
regardless of the input.

Because of this, flot needs to be told the data is UTC, not browser
local time.
@mythmon
Copy link
Contributor Author

mythmon commented Oct 24, 2012

As I demonstrated last night, the data being plotted is actually UTC data, even on a Pacific time server. And it doesn't really matter, because these are just days, not hours, so there isn't really a timezone, conceptually. So as long as things line up visually the user doesn't really care, when it comes to the histogram.

I'll change the commit message to reflect the reason for this paradoxical behavior.

@willkg
Copy link
Member

willkg commented Oct 24, 2012

No... it's actually not UTC data. If it was UTC data, then your code that generated 24 feedback items in the PDT timezone would show up across two days in UTC. Ergo, it's not showing UTC data in the graph.

@mythmon
Copy link
Contributor Author

mythmon commented Oct 24, 2012

The data in the graph is the list of pairs of timestamps and number of feedbacks corresponding to that timestamp. I don't care about the individual opinions represented by that count. Yes, the individual opinions represent midnight to midnight in a Pacific time zone (actually just in server local time), but we are not dealing with the individuals, only the aggregate number returned by elasticsearch. The data for the flot graph is a list of UTC timestamps and counts. Always. This doesn't say that it is midnight to midnight in UTC, only that for the date (ie, year, month, day, without hours or minutes) there is a particular value. Elastic just chooses to communicate these dates as a UTC number of milliseconds.

This is not something the UI would portray. The UI might say Pacific time, or whatever the server is in, since that is the information that is relevant to the user, but that doesn't change that the timestamps given to flot are UTC.

@willkg
Copy link
Member

willkg commented Oct 24, 2012

We talked about it on irc for a silly amount of time. My bad--I see what's going on now and the commit message is fine.

r+

@mythmon
Copy link
Contributor Author

mythmon commented Oct 24, 2012

Landed in bab4e72.

@mythmon mythmon closed this Oct 24, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants