949017 - Autophone - update flot to 0.8.2 and display build dates in Mountain View time.

Status: RESOLVED FIXED

(Testing Graveyard :: Autophone, defect)

Description

[Bob Clary [:bc] (inactive)]

http://www.flotcharts.org/

Flot 0.8.2 has support for displaying dates in specific time zones. This is a must have in order to display build dates in Mountain View time rather than UTC.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: bug-949017-flot.patch v1

I didn't use the minimized versions since I like to be able to step through the original code when needed. I included the entire tz directory as well.

The patch is pretty big with the new files but the relevant changes are pretty minor.

Comment 2

[Mark Côté [:mcote]]

Comment on attachment 8346083 [details] [diff] [review]
bug-949017-flot.patch v1

Review of attachment 8346083 [details] [diff] [review]:
-----------------------------------------------------------------

::: html/index.html
@@ +10,4 @@
>      <link rel="stylesheet" href="bootstrap/css/bootstrap.min.css"/>
>      <link rel="stylesheet" href="style/Aristo/jquery-ui-1.8.7.custom.css"/>
>      <link rel="stylesheet" href="style/main.css"/>
> +    <script src="scripts/flot/jquery.js"></script>

Actually this probably belongs right in the scripts dir, since the rest of the app relies on it, not just flot.  Won't make a difference as long as it's included first, but organizationally probably best to be in the root.

::: html/scripts/plot.js
@@ +11,2 @@
>  function dateStr(d) {
>    function pad(n) { return n < 10 ? '0' + n : n; }  

Might as well get rid of the trailing space here while you're in this function.

Comment 3

[Bob Clary [:bc] (inactive)]

https://github.com/markrcote/phonedash/commit/b84c00302f910fe09dc7eb81c873e0ecda170fd8

Comment 4

[Bob Clary [:bc] (inactive)]

Attachment: bug-949017-2.patch v1

The builddates are stored on phonedash as UTC dates since phonedash's timezone is UTC. But when we load them in app.js:getDataPoints(...) we parse them in the user's timezone.

buildtime = new Date(builddate).getTime();

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/parse
http://www.w3.org/TR/NOTE-datetime

We can force the builddate to be considered as a UTC date if we append the timezone to the value before converting it into a date:

buildtime = new Date(builddate + '+00:00').getTime();

before patch:

2013-12-18 14:49:58 (7ec385eb27f5)

after patch:

2013-12-18 06:49:58 (7ec385eb27f5)

checking the build itself for this revision:

grep 7ec385eb27f5 builds/*/metadata.json
builds/aHR0cDovL2Z0cC5tb3ppbGxhLm9yZy9wdWIvbW96aWxsYS5vcmcvbW9iaWxlL3RpbmRlcmJveC1idWlsZHMvbW96aWxsYS1jZW50cmFsLWFuZHJvaWQvMTM4NzM3ODE5OC9mZW5uZWMtMjkuMGExLmVuLVVTLmFuZHJvaWQtYXJtLmFwaw==/metadata.json:{"buildid": "20131218064958", "androidprocname": "org.mozilla.fennec", "version": "29.0a1", "cache_build_dir": "builds/aHR0cDovL2Z0cC5tb3ppbGxhLm9yZy9wdWIvbW96aWxsYS5vcmcvbW9iaWxlL3RpbmRlcmJveC1idWlsZHMvbW96aWxsYS1jZW50cmFsLWFuZHJvaWQvMTM4NzM3ODE5OC9mZW5uZWMtMjkuMGExLmVuLVVTLmFuZHJvaWQtYXJtLmFwaw==", "blddate": 1387378198, "tree": "mozilla-central", "bldtype": "opt", "revision": "7ec385eb27f5"}

I haven't pushed this to the repo, but have applied the patch to phonedash for testing. You can test it live there.

Comment 5

[Bob Clary [:bc] (inactive)]

Comment on attachment 8349914 [details] [diff] [review]
bug-949017-2.patch v1

This isn't sufficient. There are many places where we need to deal with the user's timezone. Also, I think the current display code will display the build date in the user's time zone instead of MVT.

Comment 6

[Bob Clary [:bc] (inactive)]

Attachment: bug-949017-tz.patch

Comment 7

[Mark Côté [:mcote]]

Comment on attachment 8350772 [details] [diff] [review]
bug-949017-tz.patch

Review of attachment 8350772 [details] [diff] [review]:
-----------------------------------------------------------------

::: html/scripts/app.js
@@ +159,5 @@
> +  var enddate = makeMountainViewDate($('#enddate').attr('value'));
> +  startdate.setTimezone('UTC');
> +  enddate.setTimezone('UTC');
> +  var startdatestr = ISODateString(startdate);
> +  var enddatestr = ISODateString(enddate);

I think a comment might be in order here.  Are you switching to MV time, then pretending it's UTC to get a clean output from ISODateString?  Maybe?

::: server/handlers.py
@@ +16,5 @@
>  
>  import autophonedb
>  
> +tz_utc = pytz.timezone('UTC')
> +tz_pac = pytz.timezone('America/Los_Angeles')

Where is this used?

Comment 8

[Bob Clary [:bc] (inactive)]

(In reply to Mark Côté ( :mcote ) from comment #7)
> Comment on attachment 8350772 [details] [diff] [review]
> bug-949017-tz.patch
> 
> Review of attachment 8350772 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: html/scripts/app.js
> @@ +159,5 @@
> > +  var enddate = makeMountainViewDate($('#enddate').attr('value'));
> > +  startdate.setTimezone('UTC');
> > +  enddate.setTimezone('UTC');
> > +  var startdatestr = ISODateString(startdate);
> > +  var enddatestr = ISODateString(enddate);
> 
> I think a comment might be in order here.  Are you switching to MV time,
> then pretending it's UTC to get a clean output from ISODateString?  Maybe?
> 

The user enters a Mountain View time, then we convert it to a UTC time since the autophone handler expects to get UTC from the browser. We use ISODateString to just get the CCYY-MM-DD format.

> ::: server/handlers.py
> @@ +16,5 @@
> >  
> >  import autophonedb
> >  
> > +tz_utc = pytz.timezone('UTC')
> > +tz_pac = pytz.timezone('America/Los_Angeles')
> 
> Where is this used?

tz_pac isn't. I'll drop that.

Comment 9

[Bob Clary [:bc] (inactive)]

Attachment: bug-949017-2-v2.patch

You were right to call out that code. This version makes sure that the date values retrieved from the controls is parsed as Mountain View time and passes the dates to the server as Mountain View dates. The server now converts the dates to UTC properly while taking into account DST.

I temporarily put some debugging print statements in to see what was going on. An example output is:

192.168.1.50:36679 - - [09/Jan/2014 06:52:51] "HTTP/1.1 GET /api/s1s2/info/" - 200 OK
0.01 (1): SELECT throbberstart,starttime,blddate,revision,phoneid,cached FROM rawfennecstart WHERE testname='local-blank' and productname='org.mozilla.fennec' and blddate >= '2013-08-23 07:00:00' and blddate < '2013-08-26 07:00:00' and cached='f' and throbberstart>0
192.168.1.50:36679 - - [09/Jan/2014 06:52:51] "HTTP/1.1 GET /api/s1s2/data/" - 200 OK
http://192.168.1.50:8100/
before tz_pac: startdate: 2013-08-23 00:00:00, enddate: 2013-08-25 00:00:00
after tz_pac: startdate: 2013-08-23 00:00:00-08:00, enddate: 2013-08-25 00:00:00-08:00
after normalize: startdate: 2013-08-23 01:00:00-07:00, enddate: 2013-08-25 01:00:00-07:00
after dst adjust: startdate: 2013-08-23 00:00:00-07:00, enddate: 2013-08-25 00:00:00-07:00
after tz_utc: startdate: 2013-08-23 07:00:00+00:00, enddate: 2013-08-25 07:00:00+00:00
after adjusting enddate: startdate: 2013-08-23 07:00:00+00:00, enddate: 2013-08-26 07:00:00+00:00
isodates: start: 2013-08-23 07:00:00, end: 2013-08-26 07:00:00

Comment 10

[Mark Côté [:mcote]]

Comment on attachment 8357767 [details] [diff] [review]
bug-949017-2-v2.patch

Review of attachment 8357767 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good though I would like an answer to my last query in particular.

::: html/scripts/app.js
@@ +162,5 @@
> +  var tzhours = Math.floor(Math.abs(tzoffset)/60);
> +  var tzminutes = Math.abs(tzoffset) - 60*tzhours;
> +  var tzstring = 'T00:00:00' + (tzoffset < 0 ? '+' : '-') + pad(tzhours) + ':' + pad(tzminutes);
> +  var startdatestr = ISODateString(new Date($('#startdate').attr('value') + tzstring));
> +  var enddatestr = ISODateString(new Date($('#enddate').attr('value') + tzstring));

If I'm reading this correctly, this will use the *current* timezone offset, which may be different from startdate/enddate due to DST... but there's not much you can do about that if we don't store the actual timezone with the dates.

@@ +183,5 @@
> +            '&cached=' + ($('#cached').attr('checked')?'cached':'notcached') +
> +            '&errorbars=' + ($('#errorbars').attr('checked')?'errorbars':'noerrorbars') +
> +            '&errorbartype=' + params.errorbartype, function(data) {
> +              makePlot(params, data);
> +            });

Ah much more readable. :) Maybe put the function down on a separate line though?

::: server/handlers.py
@@ +151,5 @@
> +        startdate = tz_pac.normalize(startdate)
> +        enddate = tz_pac.normalize(enddate)
> +        # adjust dst
> +        startdate = startdate - startdate.dst()
> +        enddate = enddate - enddate.dst()

I don't understand why you need to subtract the DST value here.  After normalizing, do you not have a proper datetime (with a guessed DST value, I guess) that can be converted to UTC?

Comment 11

[Bob Clary [:bc] (inactive)]

(In reply to Mark Côté ( :mcote ) from comment #10)
> Comment on attachment 8357767 [details] [diff] [review]
> bug-949017-2-v2.patch
> 
> Review of attachment 8357767 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good though I would like an answer to my last query in particular.
> 
> ::: html/scripts/app.js
> @@ +162,5 @@
> > +  var tzhours = Math.floor(Math.abs(tzoffset)/60);
> > +  var tzminutes = Math.abs(tzoffset) - 60*tzhours;
> > +  var tzstring = 'T00:00:00' + (tzoffset < 0 ? '+' : '-') + pad(tzhours) + ':' + pad(tzminutes);
> > +  var startdatestr = ISODateString(new Date($('#startdate').attr('value') + tzstring));
> > +  var enddatestr = ISODateString(new Date($('#enddate').attr('value') + tzstring));
> 
> If I'm reading this correctly, this will use the *current* timezone offset,
> which may be different from startdate/enddate due to DST... but there's not
> much you can do about that if we don't store the actual timezone with the
> dates.
> 

I should probably use each date instead of the current date when I get the timezone
offset. Instead of just one, I could potentially have two. I'm not totally convinced this is absolutely necessary though since the actual string value of the date won't change.

> @@ +183,5 @@
> > +            '&cached=' + ($('#cached').attr('checked')?'cached':'notcached') +
> > +            '&errorbars=' + ($('#errorbars').attr('checked')?'errorbars':'noerrorbars') +
> > +            '&errorbartype=' + params.errorbartype, function(data) {
> > +              makePlot(params, data);
> > +            });
> 
> Ah much more readable. :) Maybe put the function down on a separate line
> though?
> 

Ok.

> ::: server/handlers.py
> @@ +151,5 @@
> > +        startdate = tz_pac.normalize(startdate)
> > +        enddate = tz_pac.normalize(enddate)
> > +        # adjust dst
> > +        startdate = startdate - startdate.dst()
> > +        enddate = enddate - enddate.dst()
> 
> I don't understand why you need to subtract the DST value here.  After
> normalizing, do you not have a proper datetime (with a guessed DST value, I
> guess) that can be converted to UTC?

normalize was the wrong choice. localize doesn't not have the same issue.

Comment 12

[Bob Clary [:bc] (inactive)]

Attachment: bug-949017-2-v3.patch

Comment 13

[Bob Clary [:bc] (inactive)]

https://github.com/markrcote/phonedash/commit/d790eaa510e21a91de1c7b59246b6b4eaca05b30