Use java.time.Instant to get micros accurate timestamps #261
Conversation
fall back to System.currentTimeMillis() for Java 7
Codecov Report
@@ Coverage Diff @@
## master #261 +/- ##
============================================
+ Coverage 72.96% 73.04% +0.07%
Complexity 1134 1134
============================================
Files 121 122 +1
Lines 4221 4240 +19
Branches 422 422
============================================
+ Hits 3080 3097 +17
- Misses 943 945 +2
Partials 198 198
Continue to review full report at Codecov.
|
| import java.time.Clock; | ||
| import java.time.Instant; | ||
|
|
||
| public interface SystemClock { |
There was a problem hiding this comment.
Let's mark with a TODO or something that will let us quickly find all places we can cut unnecessary code when dropping support to 1.7.
There was a problem hiding this comment.
We just have to look out for the usages of the @IgnoreJRERequirement annotation. The animal sniffer maven plugin would not let us verify non-Java 7 code which is not annotated with this annotation.
| final long epochMillis = System.currentTimeMillis(); | ||
| epochTickClock.init(epochMillis, 0); | ||
| final long epochMicros = SystemClock.ForJava8CapableVM.INSTANCE.getEpochMicros(); | ||
| epochTickClock.init(epochMicros, 0); |
There was a problem hiding this comment.
Unit-test JRE 7 code as well (regardless of the actual JRE running the tests)
| enum ForJava8CapableVM implements SystemClock { | ||
| INSTANCE; | ||
|
|
||
| private static final Clock clock = Clock.systemUTC(); |
There was a problem hiding this comment.
Could be risky - this class cannot be fully loaded (pass linkage phase) on JRE 7. This means that ForCurrentVM cannot be fully linked as well as it depends on it. You may not experience any problem using a lazy-linkage JVM, one that allows only linking parts of the class's constant pool when required, but I am not sure it is safe to assume that will be the case in all JVMs as I don't think the lazy-linkage is part of the JVM spec.
Using reflection referencing a Method or MethodHandle would be a safer approach
There was a problem hiding this comment.
You are completely right - that would be safer. But we don't support such a VM so I would not want to pay the cost of the increased complexity and potentially higher overhead yet. We can't claim to support a particular VM if we don't have an integration test for it. If we are to support such a VM in the future at that time, we might even have already dropped Java 7 support so that we can just use Instant right away.
|
Sorry for not laying out the motivation of this change. With the current approach, the timestamp can be up to 1ms off, as it just uses This can lead to misaligned spans in the UI: Go is using an accurate epoch micros timestamp but the Java agent sort of truncates the timestamp to milliseconds. When making calls across different hosts, the clocks can be misaligned by serveral milliseconds and the UI has to do compensating actions anyway, but at least when making calls on the same host, the timestamps should be consistent. |
|
Yeah, only read through the conversation today, so removed the comment on "thinking it is too early to use Java 8 stuff" :) |
fall back to System.currentTimeMillis() for Java 7