Hi,
I just noticed that HttpExchangeTracer.sendingResponse is working with milliseconds or more specifically with System.currentTimeMillis in order to track the taken time.
As System.currentTimeMillis can be subject of time drift, I wonder if System.nanoTime should be used. Unfortunately, it's not simply done with just switching to the latter because we can't work with nano-time and Instant like it's done at the moment. There need to be changes to HttpTrace as well, which potentially means migration work for people with a persistent store and a custom HttpTraceRepository.
In theory this would also obsolete HttpTrace.getTimestamp, but probably it's okay to keep it to have an actual time representation of when the request/response exchange happened.
I've done a first draft before I open an actual PR.
Let me know what you think.
Cheers, Christoph
Comment From: wilkinsona
Thanks, @dreis2211.
I'm not sure that we can use System.nanoTime() for this. The javadoc states that "the values returned by this method become meaningful only when the difference between two such values, obtained within the same instance of a Java virtual machine, is computed." I don't think we can use it to capture the time at which the request-response exchange occurred.
Comment From: dreis2211
Hi @wilkinsona . In this scenario we're actually inside the same JVM, so we can measure the TIME_TAKEN with nano-time. Or am I missing something here?
As written earlier, HttpTrace.getTimestamp will stay as the actual time representation of when the request/response exchange happened.
Having that said, the parameter doc of the new nanoTime parameter reads like it's used as time representation. In reality it's only needed to get the starting value for measuring the taken time. Probably making the nanoTime field transient is the best way forward here to don't need to persist it.
Comment From: wilkinsona
No, it's me that missed something. I'm with you now.
I'm in favour of switching to nano time to compute the time taken, but I think we could do it as an implementation detail with just a package-private accessor for the start time that the exchange tracer can then use when the response is being sent.
Comment From: dreis2211
@wilkinsona To get us on the same page. A package-private accessor on HttpTrace? So basically what I have in the WIP branch, just making it package-private instead of public. Is that what you had in mind?
Comment From: wilkinsona
Yeah, that's it. And without the additional constructor as I don't think the start time is important for a trace coming from a persistent store as we should already have the time take at that point.
Comment From: wilkinsona
Closing in favour of #22266.