584203 - Should add Z to ISO timestamps included in MozMill reporting

Status: VERIFIED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

The reporting added to MozMill in bug 562822 includes ISO timestamps.  Without the Z, those will be interpreted by ISO parsers to be local time.  

This probably doesn't matter for us for server-side processing because servers will be UTC, and that's why we left it out.  However, if anyone does any client-side processing of report output it can be a problem as it will apply DST rules to date math.

Since we know these are UTC, we should add the Z offset specifier to avoid client-side issues.

Actual:

   time_end: '2010-08-03T21:10:00',
   time_start: '2010-08-03T21:09:51',

Expected:

   time_end: '2010-08-03T21:10:00Z',
   time_start: '2010-08-03T21:09:51Z',

Reference: http://en.wikipedia.org/wiki/ISO_8601

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This slipped through on bug 562822. The change doesn't have any impact or risk for existing tests. It will make sure that we have a consistent time on the dashboard side.

Thanks for catching this Geo!

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Patch v1

Comment 3

[Jeff Hammel]

If we're going to have a timestamp string, it should be declared in one place and utilized everywhere.  I recommend having a utils.py file and putting it in there, since __init__ is already overused.  I recommend against copy+pasting the string.  It will give N places that must be changed if we ever change how the string should be used or if we decide to make it configurable.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I would be against starting any refactoring or module creation such late in the cycle. Especially for this little change. All that can be done for Mozmill 2.0. But what we can do is to declare it in the Mozmill class, so any member function can make use of it.

Comment 5

[Jeff Hammel]

(In reply to comment #4)
> I would be against starting any refactoring or module creation such late in the
> cycle. 

Any particular reason?  We will have a utils.py going forward no doubt at some point.

> Especially for this little change. All that can be done for Mozmill 2.0.
> But what we can do is to declare it in the Mozmill class, so any member
> function can make use of it.

Why as a class member?  It doesn't seem to have anything to do with the Mozmill class.

Comment 6

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

I noticed the duplication too when I looked at the patch, but that structure was pre-existing and, more importantly, vetted.  Though I haven't looked at the surrounding code, my guess is there may be more to refactor there than just that string.  If we're defining that twice, we're probably implementing other things twice.

I'm all for refactoring, but it'll slow down bug fixes if we take an attitude of "touch the code, make it perfect."  Aside from making the fix larger, it touches more code and thus raises the chance of destabilization.  I generally think it's more productive to separate bug fix effort from refactoring effort unless the current factoring is causing the bug.  

In this particular case, I'm in favor of adding the Z as-is in Henrik's patch and filing a followup bug to refactor that area.

Comment 7

[Jeff Hammel]

See also bug 584524

Comment 8

[Jeff Hammel]

(In reply to comment #6)
> I noticed the duplication too when I looked at the patch, but that structure
> was pre-existing and, more importantly, vetted.  

Sure, but that is equally an argument not to include feature changes for 1.4.2 as close as we are to release.  I don't think its a good idea not to fix things the right way just because it's close to release time and you want an extra feature, at least not if the right way is 10 lines of code and the quick way is 2.

> Though I haven't looked at the
> surrounding code, my guess is there may be more to refactor there than just
> that string.  If we're defining that twice, we're probably implementing other
> things twice.
> 
> I'm all for refactoring, but it'll slow down bug fixes if we take an attitude
> of "touch the code, make it perfect."  Aside from making the fix larger, it
> touches more code and thus raises the chance of destabilization.  I generally
> think it's more productive to separate bug fix effort from refactoring effort
> unless the current factoring is causing the bug.  

I don't think anyone is talking about perfect code here.  While I wouldn't go so far as to take the attitude of "you touch it, you own it", by the same token not much gets fixed if no one does more than the bare minimum to meet some expectations of a bug number.  For instance, if 500 occurrences of the string "This is an errors" need to be changed to "This is an error", it is probably better to refactor to record the new string a single way rather than to do M-% replace . So I do think there is some responsibility.  The fact that we have two places instead of 500 should just make the task easier.

> In this particular case, I'm in favor of adding the Z as-is in Henrik's patch
> and filing a followup bug to refactor that area.

Comment 9

[cmtalbert]

See comment 16 on bug 583834, minusing.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Clint, can you please explain why bug 583834 is stopping us here? This is a minimal change of existing code to make sure we are sending the correct timezone information in the timestamp for reports. This is a fallout of my work on bug 562822. This is not a new feature request which needs a refactoring as given on bug 583834.

I have read Jeff's comments and I agree for a refactoring, but do we really wanna block sending timestamps in the correct format in reports, only by having a refactoring done?

Right now I have no idea how changes like that will play nicely together with the code in the dashboard Couchapp. Timestamps are one of our central pieces for report analysis.

Comment 11

[cmtalbert]

(In reply to comment #10)
> Clint, can you please explain why bug 583834 is stopping us here? This is a
> minimal change of existing code to make sure we are sending the correct
> timezone information in the timestamp for reports. This is a fallout of my work
> on bug 562822. This is not a new feature request which needs a refactoring as
> given on bug 583834.
> 
> I have read Jeff's comments and I agree for a refactoring, but do we really
> wanna block sending timestamps in the correct format in reports, only by having
> a refactoring done?
> 
> Right now I have no idea how changes like that will play nicely together with
> the code in the dashboard Couchapp. Timestamps are one of our central pieces
> for report analysis.

Because it's model-view-controller separation.  I'm not convinced (given python's inability to properly format timestamps on its own) that we ought to send things up from mozmill with our own timestamps, we ought to use language agnostic timestamps, and about the only thing all languages os's agree on is seconds from EPOC.  

This is all related to having a better set of pluggable interfaces for better reporting so that specific reporting needs do not require changes to Mozmill core code.  We have to draw the line somewhere if we are ever going to ship, and this is one of those unnecessary changes that fall on the wrong side of the line.  I realize it is a fallout of your earlier patch, maybe we should just take this, but I really don't like putting "view level" items into mozmill's core code.  This should be handled by the view application which is the couch instance, not Mozmill.

Comment 12

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

I don't get how this is view-level.  We've chosen ISO datestamps as the datatype for the model to communicate with the view.  We're misrepresenting the timestamp per that choice.  Right now, it means either local or unknown timezone, depending on context, but it doesn't mean UTC.

It seems to me that it's tight coupling for it to know that mozmill leaves the Z off and for the view to insert them later.  I don't really think that couchapp should have to know how mozmill works.

And yeah, totally agree POSIX time solves that problem, but it's not human-readable which is a huge downside in some ways.  In the interest of actually being able to debug result handling, I think we should use ISO with a Z.

Comment 13

[Jeff Hammel]

The fact that so much discussion has occurred concerning this bug is enough reason, IMHO, not to include it in an upcoming release.  More substantive reasons include:

 - as pointed out before, this bug fixes two identical strings. The appropriate fix is to have a single unified string which is used in two places.  Seeing a patch that did this would make me much happier;  since the existing patch doesn't do this, it feels like a last-minute fix. If it weren't for the long amount of discussion (which I'm contributing to only because my computer is out of order at the moment), then it doesn't feel as much like a clamor to include it last minute in a release

 - as Clint pointed out, the matter of why this is the canonical datestamp is of course up in the air.  The fact is:  it is because it has been.  Since we haven't had the 'Z' and lost it, this isn't a regression, its a poor up front decision which wasn't informed by iso-8601.  It can be worked around.  Is that ideal?  No, it's awful.  But its also important that we release software and not just argue about it.  

 - reporting will be radically changed and pluggable.  Whether we fix this change or not, this change will immediately be thrown away when the new reporting infrastructure comes it.  Datestamps will be delegated to whatever provides the information will be able to send whatever datestamp that they want.
 
 - of course, seconds since epoch is both more precise and platform independent as well as requiring lower bandwidth to send.

Are any of these arguments that important?  Not really, except that last-minute feature creep is something to be avoided. If something requires a lot of talking about to decide, it probably points to bigger issues than just a couple of 'Z's on strings

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This bug was intended to be a quick fix of something I forgot to address by a review from Clint. Nothing more. I never thought that it would become such a lengthy discussion. Sorry, but I'm off.

Comment 15

[cmtalbert]

Comment on attachment 462576 [details] [diff] [review]
Patch v1

We shouldn't be copy/pasting the time format stuff around.  We need a time format as a class or member variable somewhere, and that timeformat should contain the Z.  We don't want to copy around this raw format string.  This might need to wait for a component break up so that we have a real report component instead of having a bunch of report methods on the mozmill class, which is the wrong architecture in my opinion.

We can take this change in the meantime if it's changed as above, but you should understand all this code is going to get ripped out and refactored into a sane architecture.

Comment 16

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Refactoring for a cleaner architecture sounds like a great plan!

I'd say we want the behavior correct for the next released version, so whether or not to take this patch depends on whether such refactoring will happen before then.  If we're still unsure on that timing, I'd ask that you consider putting this fix in anyway.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #15)
> We can take this change in the meantime if it's changed as above, but you
> should understand all this code is going to get ripped out and refactored into
> a sane architecture.

This bug is more about the data consistency and not the underlying code. Any refactoring for Mozmill 2.0 will not change the date format, so I don't care about on the couchdb side.

Beside that I'm fully aware that it is a temporary solution only but it introduces no risk of breakage on our stability branch compared to your proposed final solution, which should be definitely taken care of for 2.0.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

One thing I was wondering over the weekend regards our planned performance tests... shouldn't we better format the time including the milliseconds? It was removed by my initial patch and now I think it could be useful.

Comment 19

[Jeff Hammel]

(In reply to comment #18)
> One thing I was wondering over the weekend regards our planned performance
> tests... shouldn't we better format the time including the milliseconds? It was
> removed by my initial patch and now I think it could be useful.

I think any more restructuring for date formats should wait until after reporting is pluggable.  For one, the date format is set in a single place.  For another, the date format is choosable via a command line argument (or __init__ argument if calling programmatically).

Comment 20

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Depends on whether you think those timestamps will ever make it into this report.  Generally, I'd rather send full data and cut it down for display.  

UI performance metrics probably won't be meaningfully consistent to the millisecond anyway, though, due to sync delays and whatnot, so this isn't a huge deal for me either way on that front.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ok, so what do we want to do for 1.5.1 now? Simply landing the patch attached to this bug?

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Clint, as said in the last Mozmill meeting you were about to land this patch. Is that correct?

Comment 23

[cmtalbert]

(In reply to comment #22)
> Clint, as said in the last Mozmill meeting you were about to land this patch.
> Is that correct?

Yes, you have worn me down.  Landed: http://github.com/mozautomation/mozmill/commit/f442b04c32ad52621c8119717722702f05f8bfaa

Comment 24

[Jeff Hammel]

The 2.0 form of this patch will be significantly different.  Since reporting is now a pluggable event handler, the date format is set only in one place (see: http://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/report.py) and in addition is overridable via the command line.  So adding the 'Z' string to the end of def __init__(self, report, date_format="%Y-%m-%dT%H:%M:%S"): should be sufficient

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with:

http://mozmill-release.brasstacks.mozilla.com/#/general/report/4a2217a8a22f02aec478e6684a0ce9cb

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Landed on master:
https://github.com/mozautomation/mozmill/commit/a33eec4b27ef64b27d1308790f1b8a7caaded6b3