tobyweston / temperature-machine

Data logger for multiple DS18B20 temperature sensors on one or more machines
Apache License 2.0
67 stars 22 forks source link

Dates: UTC vs LocalDateTime #56

Closed tobyweston closed 6 years ago

tobyweston commented 6 years ago

Some of the system uses UTC whilst others local data times.

Log is generated by Log4J and the pattern %d{yyyy-MM-dd HH:mm:ss:SSS} and so local time.

Logs

Instant is UTC and the LogParser and LogMessage use Instant, so it probably makes sense to convert the log code to use LocalDateTime.

Or, we could store UTC in the log file (add a Z to the log4j.properties) and present in the local time zone of the client.

Charts / RRD

RRD itself will use UTC as it uses System.currentTimeMillis under the covers (RrdUpdate.scala L16) with the below.

  public static long getTime() {
        return (System.currentTimeMillis() + 500L) / 1000L;
  }

The /temperatures endpoint returns seconds for the event time (used by the main current temperature display) whereas the /temperature.json endpoint returns millis (for the chart). Both as UTC.

tobyweston commented 6 years ago

55 switched the log time format to UTC which also paves the way for switching timezones on the UI (as per @hobie22670 use case)

hobie22670 commented 6 years ago

Additional info regarding time zones. I currently have multiple servers / clients with probes running in Northern Michigan (all in EST) UTC +5. I'm monitoring these from within the same time zone and charts showing local time is perfect. Problem came up when another team in California (PST) UTC +8 was viewing the same systems - I was seeing a 10º C rise at 11:00 - they were seeing the jump at 08:00. Confusion. What I would like to see as default at all UI's (regardless of their TZ) would use the local TZ where the server is running, thus all viewers would be presented with the same chart. If a user required they could switch to use alternate local TZ.

tobyweston commented 6 years ago

Looks like chartjs doesn't support changing timezone (https://github.com/chartjs/Chart.js/issues/4334 ) but we could map the UTC values coming from the server to an offset value set by the user.

So, if we know the browsers offset, we could reverse and overwrite the raw chart data. Taking a source UTC of +0000 and a target browser +0400, we could moment(x).minus("4", "hours").valueOf (or similar). Bit annoying to have to pre-process all data points though.

The other option would be to globally set the moment offset and as long as the charting library support it, it should just work.

moment.tz.setDefault("UTC");

This has the advantage of automatically affecting the logs :+1:.

tobyweston commented 6 years ago

I've got something basic working, is this what you need? UTC is the default but can be switched via a dropdown. It's not pushed yet as it needs some styling attention...

screenflow

tobyweston commented 6 years ago

One thing I don't like when the server is not in a UTC zone. I'd prefer the default to be in the server's timezone.

tobyweston commented 6 years ago

This is basically fixed in 18bdc61 now. A timezone picker is available that affects the log page and main chart.

Remaining issues:

  1. RRD charts don't update - I don't think I can do much about this
  2. I'd like to default the timezone (in the picker) to display the servers timezone

I'll leave the issue open for now until we resolve the above.

It looks like this:

screenflow

tobyweston commented 6 years ago

Default Timezone to Server's Timezone

This is a bit trickier than I thought. I'm in two minds how to do this. I don't really want to modify the Host class to include if it's the server of not but I don't think a JavaScript client can easily work out where it's been served from. If it could, it would know which of the /connections hosts it is and pick that timezone...

Options:

tobyweston commented 6 years ago

Went for the server returning a X-Forwarded-Host header which if found in the /connections call, will be used for the default timezone. 👊