grpc-java icon indicating copy to clipboard operation
grpc-java copied to clipboard

Channelz should use milliseconds for timestamps

Open zhangkun83 opened this issue 6 years ago • 5 comments

#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().

zhangkun83 avatar Mar 24 '19 21:03 zhangkun83

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 avatar Mar 25 '19 17:03 carl-mastrangelo

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.

dapengzhang0 avatar Mar 29 '19 18:03 dapengzhang0

Started Analysis and going through previous comments/details.

vinodhabib avatar Oct 04 '24 12:10 vinodhabib

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.

zhangkun83 avatar Oct 04 '24 21:10 zhangkun83

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.

ejona86 avatar Oct 17 '24 14:10 ejona86