#13295: Instrumenter.getNanos() returns non-epoch timestamp. #13535
Conversation
|
| @@ -266,7 +266,7 @@ private void doEnd( | |||
|
|
|||
| private static long getNanos(@Nullable Instant time) { | |||
| if (time == null) { | |||
| return System.nanoTime(); | |||
| time = Instant.now(); | |||
There was a problem hiding this comment.
time measurements should use a monotonic time source, Instant.now is not monotonic
There was a problem hiding this comment.
This is not a return statement. It is to replace the null input arguement with the same Instant object with current time, then goes to line 272 to calculate the a monotonic time.
There was a problem hiding this comment.
I think you have misunderstood what monotonic means here. The problem with using the "wall clock" time for like Instant.now for measuring elapsed time is that it is affected by system time adjustments. If system time is adjusted either by user or ntp between start or stop then stop can be before start which would result in a negative duration. Monotonic time only goes forward and is not adjusted.
There was a problem hiding this comment.
If system time is adjusted either by user or ntp between start or stop, then your same argument applies to line 272, where the input argument time is not null. Are we saying that we should not use the input argument time as all? If so, the whole logic in this getNanos() should be changed accordingly.
There was a problem hiding this comment.
Since this is a private method it seems to be working correctly for the desired public and internal API behaviors. If it helps, maybe just add a comment This must be called for start/end times either both null or both non-null without other changes?
There was a problem hiding this comment.
If system time is adjusted either by user or ntp between start or stop, then your same argument applies to line 272, where the input argument time is not null. Are we saying that we should not use the input argument time as all? If so, the whole logic in this getNanos() should be changed accordingly.
It would apply if both the start and end instances were constructed by calling Instant.now, but that is not what we do. Have a look at https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/Timer.java
| Instant startTime = Instant.now(); | ||
| Instrumenter<Map<String, String>, Map<String, String>> instrumenter = | ||
| builder.buildInstrumenter(); | ||
| InstrumenterUtil.startAndEnd( |
There was a problem hiding this comment.
startAndEnd doesn't have Nullable time parameters and is an internal method, so this seems invalid
| @@ -266,7 +266,7 @@ private void doEnd( | |||
|
|
|||
| private static long getNanos(@Nullable Instant time) { | |||
| if (time == null) { | |||
| return System.nanoTime(); | |||
| time = Instant.now(); | |||
There was a problem hiding this comment.
Since this is a private method it seems to be working correctly for the desired public and internal API behaviors. If it helps, maybe just add a comment This must be called for start/end times either both null or both non-null without other changes?
No description provided.