Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

time: add Time.UnixMilli and Time.UnixMicro (like Time.UnixNano) #44196

Closed
willyrgf opened this issue Feb 10, 2021 · 27 comments
Closed

time: add Time.UnixMilli and Time.UnixMicro (like Time.UnixNano) #44196

willyrgf opened this issue Feb 10, 2021 · 27 comments

Comments

@willyrgf
Copy link

@willyrgf willyrgf commented Feb 10, 2021

I would like to implement a function time.UnixMills() to return t in int64 of milliseconds, like in the time.UnixNano() returns t in int64 of nanoseconds.
Today If I want to get the value of the time in milliseconds we need to do a lot of calculations, but I know other languages have this function and researching about that I've implemented that function in the time package.
It's a simple function with two const needed to easier understood.

Here is the simplest way that I've found to implement: https://go-review.googlesource.com/c/go/+/290772/1/src/time/time.go

This implementation is based on Rust as_millis(): https://doc.rust-lang.org/src/core/time.rs.html#393


EDITED:

First, thanks all for the contributions with all discuss, I had some problems and I couldn't contribute much during the discussions and initially not being able to expose all my points. Now all points have been discussed and we apparently have a consensus.

Just to format the proposal correctly with what was agreed.
Based on the discussions we will have both directions of the converters, generators and consumers and we probably will have something like that:

func (t Time) UnixMilli() int64 {}
func (t Time) UnixMicro() int64 {}

func UnixMilli(msec int64) Time {}
func UnixMicro(usec int64) Time {}
@gopherbot gopherbot added this to the Proposal milestone Feb 10, 2021
@willyrgf willyrgf changed the title proposal: time: add a UnixMills() to return a millisecond proposal: time: add a UnixMills() to return t in milliseconds Feb 10, 2021
@cespare
Copy link
Contributor

@cespare cespare commented Feb 10, 2021

This was previously considered and rejected at #18935 and #27782.

I still think it's worth doing, though. (I think the name would be UnixMilli to match UnixNano, not UnixMills.)

One small thing that has changed since #27782 is that we added time.Duration.Milliseconds (and Microseconds) in #28564. That might lend a bit of weight toward UnixMilli.

To recapitulate some points I made on #27782 after it was closed:

  1. I believe this is a common need (at least it has been in my work): #27782 (comment)
  2. The fact that there's no popular third-party package implementing this isn't evidence of anything other than good taste (left-pad is not the Go way): #27782 (comment)
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Feb 10, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Feb 10, 2021

@willyrgf, what is the context where you need to do time calculations in Unix milliseconds?
#27782 was closed because we are unaware of a compelling need that justifies adding to the API.
We've heard a few people say "we use them at work" but nothing more widespread than that.
We are looking for some kind of standard reason you'd need that exact format.

@rsc rsc moved this from Incoming to Active in Proposals Feb 10, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Feb 10, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@cwaldren
Copy link

@cwaldren cwaldren commented Feb 10, 2021

At work, we use milliseconds in protobuf messages, databases, and log messages. I can't recall any usage of time.UnixNano() besides converting to milliseconds. In many of these places, authors have debated/searched/reluctantly settled on different approaches, usually one of:

time.UnixNano() / int64(time.Millisecond)
time.UnixNano() / 1e6

This confuses new Go developers ("Am I doing this wrong? I must be missing something obvious.."), and makes normally straightforward code harder to mentally parse. Adding a dedicated user-defined library call to solve this problem isn't satisfactory:

  1. It makes the code asymmetric (time.UnixMilli() vs millis.From(time.Now()))
  2. It requires all developers to discover this package, then call it out in code reviews or add automatic linting
  3. It requires perpetual maintenance, as was called out in another comment in relation to maintaining the standard library. But instead of having one set of maintainers, we have N for every team out there in the world developing with Go.
@ulikunitz
Copy link
Contributor

@ulikunitz ulikunitz commented Feb 10, 2021

Please have a look at package https://pkg.go.dev/github.com/ulikunitz/unixtime, which I wrote for #27782.

On pkg.go.dev it has no reported imports, but on github.com there are 865 clones and 145 unique cloners.

@andig
Copy link
Contributor

@andig andig commented Feb 10, 2021

Used ms timestamps on a couple of php projects, never used nanos except in Go for converting to mill8s.

@msiebuhr
Copy link
Contributor

@msiebuhr msiebuhr commented Feb 10, 2021

JavaScript works in milliseconds; e.g. Date.now() and x = new Date(); x.getTime() returns unix-time in milliseconds, as well as new Date(<number>) expecting milliseconds. Any significant JavaScript code-base I ever worked on has made use of milliseconds for date-serialization (and timezone-independent time-comparison).

I've only written a few go-programs with considerable JavaScript-interactions, but in all of those, time.UnixNano() / 1e6 was a fairly common occurrence (and source of bugs if one does not use it often enough to remember if it's 1e3, 1e6 or 1e9...)

@josharian
Copy link
Contributor

@josharian josharian commented Feb 10, 2021

Another API option is

func (Time) SinceUnix() time.Duration

Better name welcome.

Then you could write t.SinceUnix().Milliseconds().

Or t.SinceUnix().Seconds() or t.SinceUnix().Nanoseconds(), for that matter.

Other name ideas:

  • SinceEpoch
  • SinceUnixEpoch
  • UnixDuration (via @benhoyt)
@willyrgf
Copy link
Author

@willyrgf willyrgf commented Feb 10, 2021

Thanks everyone for all suggestions, ideas and the questions. I'll edit the proposal and present a more completed proposal with all my arguments for each suggetions, ideas and questions sent here.

@randall77
Copy link
Contributor

@randall77 randall77 commented Feb 10, 2021

Or t.Sub(time.Unix(0,0)).Milliseconds(). No API changes required.

@benhoyt
Copy link
Contributor

@benhoyt benhoyt commented Feb 10, 2021

@josharian I definitely prefer that suggestion, though my name suggestion would be UnixDuration, as in t.UnixDuration().Milliseconds(). Would have been nice if Unix had returned a duration to begin with, and then we would have only needed one, but oh well.

@randall77's suggestion is not bad, though it's a bit awkward.

@ulikunitz
Copy link
Contributor

@ulikunitz ulikunitz commented Feb 11, 2021

Any method to compute Unix time in milliseconds from a nanosecond int64 value, which includes values of type Duration, limits the supported time range approximately from 1677-09-21 to 2262-04-11, which is much smaller than the Javascript range which is "approximately -273,790 to 273,790 years relative to 1970.". See https://tc39.es/ecma262/2020/#sec-time-values-and-time-range.

Since Javascript is the driver for the millisecond requirement, I think this should be taken into consideration. But if the full int64 millisecond Unix time range is supported an additional function UnixMillis to compute a Time value from Unix time in milliseconds will be required.

@ConradIrwin
Copy link
Contributor

@ConradIrwin ConradIrwin commented Feb 12, 2021

re "looking for some kind of a standard reason".

We convert timestamps into milliseconds since the epoch (and vice versa) for talking to external APIs (I'm sure there are more, this is just what we happen to use, and which showed up when I grepped for the manual conversion):

  • Apple's Appstore
  • Amplitude's
  • Airtable
  • BigQuery
  • Gmail

I always think of this as a "representation of a timestamp" not as "the duration since 1970" (though I realize both definitions are equivalent), so the framing of UnixMillis makes more sense to me than the intermediate duration conversion.

I'd also love a symmetrical method to create a new time from a timestamp formatted this way, so that the conversion into and out-of this format is similarly easy.

// UnixMilli returns t as a Unix time, the number of milliseconds elapsed since January 1, 1970 UTC
func (t Time) UnixMilli() int64 {
  return t.UnixNano() / 1e6
}

// UnixMilli returns the local Time corresponding to the given Unix time m milliseconds since January 1, 1970 UTC. 
func UnixMilli(m int64) Time {
  return Unix(0, m * 1e6)
}

Similar to @cespare, here, most uses of (*Time) UnixNano or time.Unix in our codebase in contexts where the source or destination is actually in milliseconds; and in the course or searching I found an incorrect conversion that had slipped past code review: time.Unix(m/1000, (m%1000)*1000).

(Every time I have to implement this, I end up on this StackOverflow post (https://stackoverflow.com/questions/24122821/go-golang-time-now-unixnano-convert-to-milliseconds) which has a bunch of debate on whether the accepted answer is actually correct, or just happens to work given the way go is setup today...)

@andig
Copy link
Contributor

@andig andig commented Feb 12, 2021

and in the course or searching I found an incorrect conversion that had slipped past code review

I can confirm having done the same, including looking at SO first .

I love @randall77 's solution in #44196 (comment) but I don't think it has come up on SO. For performance reasons, it might also be nice to avoid the float division of the naive time.UnixNano() / int64(time.Millisecond) approach.

@rsc
Copy link
Contributor

@rsc rsc commented Feb 17, 2021

@ConradIrwin thanks for those references, and thanks @msiebuhr for pointing out JavaScript.
Although there are clever ways to do without, it does seem like we should just add UnixMilli.
And maybe UnixMicro both for PHP and to fill in the strange gap between UnixMilli and UnixNano.

@rsc rsc changed the title proposal: time: add a UnixMills() to return t in milliseconds proposal: time: add Time.UnixMilli and Time.UnixMicro (like Time.UnixNano) Feb 17, 2021
@benhoyt
Copy link
Contributor

@benhoyt benhoyt commented Feb 17, 2021

Why Duration.Milliseconds and Duration.Microseconds spelled in full, but Time.UnixMilli and Time.UnixMicro abbreviated?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 17, 2021

For better or for worse we should follow the existing example of UnixNano.

@benhoyt
Copy link
Contributor

@benhoyt benhoyt commented Feb 17, 2021

Fair enough -- I noticed there's also StampMilli and StampMicro. But you're right, makes sense to follow the pattern "closest" (the Time methods).

@slrz
Copy link

@slrz slrz commented Feb 17, 2021

There's more stuff in time with second and nanosecond variants only. Should those also start to grow millisecond-flavored neoplasms?

This is cluttering the package docs for everyone just to get rid of a single division in (some) user code.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 17, 2021

Change https://golang.org/cl/293349 mentions this issue: time: add UnixMilli and UnixMicro

@rsc
Copy link
Contributor

@rsc rsc commented Feb 24, 2021

The CL that was sent originally included constructors time.UnixMilli and time.UnixMicro but they have been removed.
Still, it seems incomplete to have the converters in only one direction. If you have to use this format, you probably have to both generate and consume it.
But what to call it?

I originally gave time.Unix two arguments to avoid having separate time.Unix and time.UnixNano.
And I think we can still get away without having time.UnixNano.
But perhaps we should add time.UnixMilli and time.UnixMicro, so we'll have Unix (does seconds + nanos), UnixMilli, and UnixMicro.

Does anyone object to this?

@rsc
Copy link
Contributor

@rsc rsc commented Mar 10, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals Mar 10, 2021
@willyrgf
Copy link
Author

@willyrgf willyrgf commented Mar 16, 2021

I updated the proposal based on all discussions that took place here.

@ulikunitz
Copy link
Contributor

@ulikunitz ulikunitz commented Mar 16, 2021

Some tiny detail regarding the updated proposal: Please use µsec or at least usec for microsecond. The former uses the proper SI unit prefix and the latter is the typical abbreviation in programming languages restricted to ASCII. For example struct timeval of <sys/time.h> has a field tv_usec. The name mcsec appears at least to me quite unusual.

@willyrgf
Copy link
Author

@willyrgf willyrgf commented Mar 19, 2021

In fact it's not a good name, I didn't think when I wrote this.
Adjusted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals Mar 24, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Mar 24, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: time: add Time.UnixMilli and Time.UnixMicro (like Time.UnixNano) time: add Time.UnixMilli and Time.UnixMicro (like Time.UnixNano) Mar 24, 2021
@rsc rsc modified the milestones: Proposal, Backlog Mar 24, 2021
@ConradIrwin
Copy link
Contributor

@ConradIrwin ConradIrwin commented Mar 25, 2021

I updated https://golang.org/cl/293349 to match

@gopherbot gopherbot closed this in 49dccf1 Mar 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet