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

[docs] Clarify that DateTimeKnownEncoding.unixTimestamp is seconds (not milliseconds) #2887

Closed
mikeharder opened this issue Feb 5, 2024 · 12 comments · Fixed by #2893
Closed
Assignees
Labels
docs Improvements or additions to documentation triaged:core
Milestone

Comments

@mikeharder
Copy link
Contributor

mikeharder commented Feb 5, 2024

I think the documentation for DateTimeKnownEncoding.unixTimestamp at https://github.com/microsoft/typespec/blob/main/docs/libraries/http/encoding.md#utcdatetime-and-offsetdatetime can be improved:

  1. Specify that the timestamp is the number of seconds since the epoch (not milliseconds like JavaScript Date).

  2. Update the example payload to use timestamp in seconds rather than milliseconds:

< "createdAtUnix": 1665559250520
> "createdAtUnix": 1665559250

Related:

@mikeharder
Copy link
Contributor Author

From @johanste:

@bterlson, this was intended as seconds, not milliseconds, right?

@bterlson
Copy link
Member

bterlson commented Feb 5, 2024

Unix time as far as I understand it is properly understood to be seconds since epoch and stored in a 32 bit integer.

@bterlson
Copy link
Member

bterlson commented Feb 5, 2024

And yeah, the docs should be updated. We called the scalar type unixTimestamp32 to avoid this confusion, but unfortunately DateTimeKnownEncoding does not make things as clear.

@mikeharder
Copy link
Contributor Author

mikeharder commented Feb 5, 2024

@bterlson: For the spec in question:

Azure/autorest.csharp#4189 (comment)
Azure/azure-rest-api-specs#26702 (comment)

The service does use milliseconds since epoch. How should this be represented in TypeSpec?

@bterlson
Copy link
Member

bterlson commented Feb 5, 2024

I think we have to add a known encoding for that? @timotheeguerin may know more.

Also note that you would probably encode it as a safeint instead of an int64 (53 bits is good enough for 285 ka with ms granularity).

@mikeharder mikeharder changed the title [docs] Clarify whether DateTimeKnownEncoding.unixTimestamp is seconds or milliseconds [docs] Clarify that DateTimeKnownEncoding.unixTimestamp is seconds (not milliseconds) Feb 5, 2024
@timotheeguerin
Copy link
Member

timotheeguerin commented Feb 5, 2024

yeah we'd have to define a new encoding as just saying the same encoding on an int64 target would still be seconds but with an int64 type. (basically unixtimestamp64)

If we want to have a type that allows second we should have a unixTimestampWithMilli or whatever else is the type usually called.

And yeah lets update the docs to be clear that this encoding means encoded as an integer of seconds

@timotheeguerin
Copy link
Member

This website seems to be showing a good set of examples how unixtimestamp with millisseconds is represented in different language
https://currentmillis.com/

Usually seems to either have no way to do it or just suffixed with milliseconds

@bterlson
Copy link
Member

bterlson commented Feb 5, 2024

I am not sure I like changing the semantics of the encoded value based on the data type you store the ecoded value in? Do we have other cases of that? Also I would prefer to see safeint in virtually all instances of this (int64 has interoperability issues in JSON), and it's less clear that safeint implies ms granularity.

@timotheeguerin
Copy link
Member

timotheeguerin commented Feb 5, 2024

I mean

@encode("unixTimestamp", int32)
scalar unixTimestamp32 extends utcDateTime;

@encode("unixTimestamp", int64)
scalar unixTimestamp64 extends utcDateTime;  // This is seconds it is consitent we are just saying that the data type used is int64 not int32

// Now here we define a different encoding
@encode("unixTimestampMilliseconds", int64)
scalar unixTimestampMilliseconds64 extends utcDateTime;

@bterlson
Copy link
Member

bterlson commented Feb 6, 2024

ah ok sorry I misread your comment. Yes, I think that's the general shape of things, though we could bikeshed name some (and also probably want to add a scalar for the safeint variant).

@markcowl markcowl added this to the [2024] March milestone Feb 7, 2024
@markcowl markcowl added the docs Improvements or additions to documentation label Feb 7, 2024
@markcowl
Copy link
Contributor

markcowl commented Feb 7, 2024

est: 1

@timotheeguerin timotheeguerin self-assigned this Feb 7, 2024
timotheeguerin added a commit that referenced this issue Feb 9, 2024
…g of unixTimestamp (#2893)

fix #2892 Clarify `numeric`, `integer`, `decimal`, `float`
fix #2887 Clarify unixTimestamp encoding

---------

Co-authored-by: Brian Terlson <[email protected]>
@mikeharder
Copy link
Contributor Author

@timotheeguerin: Can you please update https://github.com/microsoft/typespec/blob/main/docs/libraries/http/encoding.md#utcdatetime-and-offsetdatetime as well? As stated in the OP, the example is very misleading (shows a value of milliseconds since epoch).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to documentation triaged:core
Projects
None yet
4 participants