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

Feat/timestamp example #262

Merged

Conversation

angelo-moreira
Copy link
Contributor

An example of how to work with Google Timestamp Protobuf type.


Helloworld.HelloReply.new(
message: "Hello #{request.name}",
today: %Google.Protobuf.Timestamp{seconds: seconds}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to make a small suggestion.

Generally, when we want to convey the idea of time, we do it in a way that the client receiving the message can decode the values in the best possible way. The timestamp type was designed for these cases and for this reason it has attributes to show the time both in seconds and in nano. As we see in the original proto documentation:

https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/timestamp.proto:

 Example 4: Compute Timestamp from Java `System.currentTimeMillis()`.

     long millis = System.currentTimeMillis();

     Timestamp timestamp = Timestamp.newBuilder().setSeconds(millis / 1000)
         .setNanos((int) ((millis % 1000) * 1000000)).build();

Note that in the original examples both attributes (seconds and nanos) are demonstrated.

I think it's interesting to add the values in nanos too since as an example it should be as complete as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @sleipnir thank you for your feedback, I have left it out because I didn't fully understand what's going on.

I have wrote the following code in IEx

iex(20)> d
~U[2022-08-05 14:19:23.263079Z]
iex(21)> d |> DateTime.to_unix(:second)                              
1659709163
iex(22)> d |> DateTime.to_unix(:nanosecond)
1659709163263079000
iex(23)> (d |> DateTime.to_unix(:millisecond) |> rem(1000)) * 1000000
263000000

I was expecting the nanosecond to be 263079000 but instead I get 263000000 with that calculation, I guess you followed the Java example here https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/timestamp.proto

Because I'm not 100% sure, I didn't want to include nanoseconds, is the calculation right? can I trust the 263000000 number?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @angelo-moreira I believe that the correct thing in Elixir to give us the Epoch Timestamp in nanoseconds would be if we used System.monotonic_time(:nanosecond).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we have the time in nano we can convert it to seconds using time |> System.convert_time_unit(:nanosecond, :second)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rem calculation would be correct if you used microseconds instead of milliseconds.

However, using System to get the time seems simpler

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would wrap the code to yield seconds directly from System.monotonic_time instead of converting from nanoseconds

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@polvalente I wanted to avoid two calls to monotonic_time and so I would convert from nano to seconds, but your suggestion works too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that makes sense. I think calculating from a single System call might be better indeed, because the actual second can increment between calls

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what I've read so far it seems that monotic time seems to be a measure to calculate the difference between 2 points in time without relying in the system clock, instead relying in the BEAM.

I might be wrong and I'm happy to be corrected, in the meantime I have updated the example to include nanos based in the Java example at https://github.com/protocolbuffers/protobuf/blob/79c3d07c63e711255e0d566c9b876706e978686a/src/google/protobuf/timestamp.proto#L90

Please let me know if you find this an acceptable solution.

examples/helloworld/.gitignore Outdated Show resolved Hide resolved
examples/helloworld/mix.exs Outdated Show resolved Hide resolved
@polvalente polvalente merged commit dd6cc87 into elixir-grpc:master Aug 5, 2022
@angelo-moreira angelo-moreira deleted the feat/timestamp-example branch August 5, 2022 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants