Utc time

[matthiashanel]

I went through all structs with json fields and replaced them with a custom type UtcTime.
From there I changed all occurrences of time.Time that end up in such a struct as well.
Because if was a different type the changes where rather large.
Because these changes made it all the way to events/monitoring, the new type would break backwards compatibility for the embedding use case. (See commit one of this PR)
Therefore I reverted and stuck with time.Time but added .UTC wherever it was missing.

If we wanted we could use the type (pasted below) in jetstream only where it does not interfere with embedding yet.
The implementation would make sure we use UTC when writing json and only accept times when in UTC.
Comments?

// Time that assures json parsing only emits/accepts times in UTC
type UtcTime time.Time

func (t UtcTime) MarshalJSON() ([]byte, error) {
	return []byte(`"` + time.Time(t).UTC().Format(time.RFC3339Nano) + `"`), nil
}

func (t *UtcTime) UnmarshalJSON(data []byte) error {
	if len(data) <= 2 {
		return fmt.Errorf("time not formated properly: %q", (*time.Time)(t).String())
	}
	pt, err := time.Parse(time.RFC3339Nano, string(data[1:len(data)-1]))
	if err != nil {
		return err
	}
	if pt.Location() != time.UTC {
		return fmt.Errorf("time not in UTC: %q", (*time.Time)(t).String())
	}
	*t = UtcTime(pt)
	return nil
}

[wallyqs]

fyi for Java client that this one would change /fyi @ColinSullivan1 @scottf

[wallyqs]

@matthiashanel can we switch this change into a different branch? I think there is consensus on changing this one to duration so we can merge and integrate the change into the clients, but not sure right now on the side effects of changing some of the other ones.

[matthiashanel]

ok, will move the duration into a separate branch

[matthiashanel]

@wallyqs moved the duration change into #1944 and removed it from here

[kozlovic]

This is quite a change, not talking about code changes, but may surprise some users.

As for having a custom type with own Unmarshal, I would not go there if possible. I had a case where I added a custom marshal, but this because really tricky when using embedded types. For instance if Foo embeds Bar and you have a (un)marshaljson for Bar, then it will apply to Foo which means that all fields from Foo will not be marshaled/unmarshaled, etc..

[matthiashanel]

This is quite a change, not talking about code changes, but may surprise some users.

As for having a custom type with own Unmarshal, I would not go there if possible. I had a case where I added a custom marshal, but this because really tricky when using embedded types. For instance if Foo embeds Bar and you have a (un)marshaljson for Bar, then it will apply to Foo which means that all fields from Foo will not be marshaled/unmarshaled, etc..

@kozlovic yeah with Unmarshal this became quite unruly. I moved the duration change out as per Wally's suggestion. Now this deals with time only.

[kozlovic]

@kozlovic yeah with Unmarshal this became quite unruly. I moved the duration change out as per Wally's suggestion. Now this deals with time only.

But still, this is what I meant, that changing time to UTC() may still cause issues/surprises, no? I mean what if users had apps/scripts checking those? That may break no? That being said, 2.2.0 would be a good time and could be listed as a [CHANGED] entry in the release notes.

[ripienaar]

I think just converting to UTC everywhere we need is fine, that's what we should have done before. We might also look at the places we receive timeTime from the user and converting those before storing, this should be fine and safe.

So to me this seems fine - if we already also force all user input to be utc before sticking it into our internal structures? Definitely would not be fine with custom marshallers this late.

I wonder if we need a utility function for getting time.Now().UTC() to normalise thinking about that utilty instead of always having to remember the .UTC() something that I also forget quite often

[matthiashanel]

@ripienaar I will add utcNow() and replace time.Now().UTC() with it.

[ripienaar]

@ripienaar I will add utcNow() and replace time.Now().UTC() with it.

well best ask Ivan or Derek what they think about that :)

[matthiashanel]

@ripienaar I will add utcNow() and replace time.Now().UTC() with it.

well best ask Ivan or Derek what they think about that :)

@derekcollison do you have an opinion on this?

[derekcollison]

I agree just make sure all time.Now() have a UTC() on the end is best for now.

[matthiashanel]

This change adds .UTC wherever needed (time is exposed externally).
I manually checked all 48 remaining invocations of time.Now().
These are only used locally (mostly to compute timeouts).
@derekcollison Should I add .UTC() to these as well? (Double checking on this. So far I've changed the necessary ones and you mentioned all in you last comment)

Where we end up using .Unix() or .UnixNano() we essentially convert to UTC already.
Where such a time is converted back using time.Unix(...) the resulting time will be in UTC.
Because of this, these calls where skipped.
Initializations for seeds and setting deadlines where skipped too.

List remaining calls: grep time.Now server/*.go -n | grep -e "_test.go:" -e ".UTC\(\)" -e "rand.NewSource.time.Now" -e "rand.Seed" -e "SetReadDeadline.time.Now" -e "SetWriteDeadline.time.Now" -e "time.Now.*.Unix" -v

[kozlovic]

LGTM