Change all time formatting to UTC and off of time.RFC3339Nano

[jacksontj]

Prometheus has issues parsing RFC3339Nano timestamps if the year has more than 4 digits, in addition it is the second-pass parse attempt. Since this is a client library and the interface is a time.Time it makes sense that we pick the clearest simplest format-- so I propose we use the model.Time representation of time in our communications to prometheus. This (1) removes the issues with timezones in those queries going downstream and (2) completely works around this #614 issue as the parsing mechanism in prometheus can handle those times in this format.

Related to #614

[krasi-georgiev]

As discussed this would be fixed on the server side so that clients in other languages don't hit the same problem.

[brian-brazil]

I think using unixtime makes more sense, and greatly reduces the chances we'll hit issues compared to a more human readable format.

We're trying to kill off model, so please avoid increasing usage of it.

[jacksontj]

I understand the want to reduce model usage bit this clients API is already based on it, so at this point removal of common from this specific client is a significantly larger project.

[brian-brazil]

I'm not saying to remove existing uses right now, I'm saying let's not add more.

[jacksontj]

That change seems entirely pointless to me. At this point there are already 88 occurances of model.* in this package-- why would I take the time to copy/paste that single struct over (which BTW is also used in all the model.Value etc.) to remove 1 occurance? At this point that seems like wasted effort, not to mention duplicating the code (since we'd use both model.Time and a copy of it in the same package).

[beorn7]

@brian-brazil:

I think using unixtime makes more sense,

Do I understand this correctly that you are in general in favor of this change?

@jacksontj: return model.TimeFromUnixNano(t.UTC().UnixNano()).String() is just a very elaborate way of turning t into a string-formatted float with the unixtime as a value. I think what @brian-brazil wants is to simply replace that line with

return strconv.FormatFloat(float64(t.UnixNano())/1e9, 'f', -1, 64)

There is really no reason to involve any logic from any other Prometheus packages.

[brian-brazil]

Do I understand this correctly that you are in general in favor of this change?

Yes.

[beorn7]

OK, cool. I'd say, let's just use the line I suggested above instead of the model.TimeFromUnixNano call, and then this is good to go. (Modulo one tiny note added below.)

[jacksontj]

@beorn7 PR updated with comments, should be ready for merge

[beorn7]

@jacksontj many thanks.
@krasi-georgiev does this work for you, too?

[krasi-georgiev]

yep all good.

[beorn7]

Thanks everyone!

[jacksontj]

@beorn7 seems that the formatTime function you have me doesn't quite work

Doing some local testing this is the output of:

fmt.Println(startTime, formatTime(startTime))	fmt.Println(endTime, formatTime(endTime)) 

-292273086-05-16 16:47:06 +0000 UTC 6795364580.679345 292277025-08-18 07:12:54.999 +0000 UTC -6795364579.680346 

[jacksontj]

Here's the same on go playground: https://play.golang.org/p/qMLQcs_WG2J

It seems that going to this unix format has similar formatting issues where it won't go negative properly :/

[jacksontj]

#619 has the fix :)

[beorn7]

Ooof, yeah, of course, because UnixNano is rather limited in its time range.
(Would have been the same problem if using the model package.)
Thanks for the fix.