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

Channelz should use milliseconds for timestamps #5494

Closed
zhangkun83 opened this issue Mar 24, 2019 · 5 comments · Fixed by #11604
Closed

Channelz should use milliseconds for timestamps #5494

zhangkun83 opened this issue Mar 24, 2019 · 5 comments · Fixed by #11604
Milestone

Comments

@zhangkun83
Copy link
Contributor

#4883 changed the TimeProvider.currentTimeNanos()'s precision to milliseconds, because there isn't a way to get the current time in nanoseconds precision. After #5056 is fixed, Channelz/ChannelTracer will be the only users of TimeProvider, and they all convert the time to proto Timestamp which doesn't mandate the unit. It will only be misleading to keep TimeProvider.currentTimeNanos() as is. It should be changed to currentTimeMillis().

@carl-mastrangelo
Copy link
Contributor

I'm going to disagree a little here. Two reasons:

  1. When we (eventually) move to Java 8, the time API does provide nanosecond precise timestamp
  2. Keeping all timestamps in gRPC as the same unit simplifies reading the code, and no conversions are necessary. Nanoseconds everywhere (except on API boundaries) are the most reasonable choice.

@carl-mastrangelo carl-mastrangelo added this to the Next milestone Mar 26, 2019
@dapengzhang0
Copy link
Member

One more factor to consider: the channelz service uses timestamp.proto, so when you create a timestamp message, you have to use its nano precision API, and if you have a millis variable, you still have to cast it to nano at some point.

@vinodhabib
Copy link
Contributor

vinodhabib commented Oct 4, 2024

Started Analysis and going through previous comments/details.

@zhangkun83
Copy link
Contributor Author

I agree with @dapengzhang0's last comment. It is more convenient to use nanos to create timestamp.proto timestamps. We can now change SYSTEM_TIME_PROVIDER to use Java 8 Instant.getNano() to get nano precision. I didn't find a way to get the current epoch nanos directly, so you'd still have to combine the seconds part and the nanos part from Instant.

@ejona86
Copy link
Member

ejona86 commented Oct 17, 2024

It is more convenient to use nanos to create timestamp.proto timestamps

The TimeProvider API is already using nanos, so there's no change needed here. It's just what resolution the actual values use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants