-
Notifications
You must be signed in to change notification settings - Fork 325
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
Use java.time.Instant to get micros accurate timestamps #261
Changes from all commits
d3c6bf1
053911f
b6b8692
f04e2f9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
/*- | ||
* #%L | ||
* Elastic APM Java agent | ||
* %% | ||
* Copyright (C) 2018 Elastic and contributors | ||
* %% | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
* #L% | ||
*/ | ||
package co.elastic.apm.impl.transaction; | ||
|
||
import org.codehaus.mojo.animal_sniffer.IgnoreJRERequirement; | ||
|
||
import java.time.Clock; | ||
import java.time.Instant; | ||
|
||
public interface SystemClock { | ||
|
||
long getEpochMicros(); | ||
|
||
enum ForCurrentVM implements SystemClock { | ||
INSTANCE; | ||
private final SystemClock dispatcher; | ||
|
||
ForCurrentVM() { | ||
SystemClock localDispatcher; | ||
try { | ||
// being cautious to not cause linking of ForJava8CapableVM in case we are not running on Java 8+ | ||
Class.forName("java.time.Clock"); | ||
localDispatcher = (SystemClock) Class.forName(SystemClock.class.getName() + "$ForJava8CapableVM").getEnumConstants()[0]; | ||
} catch (Exception e) { | ||
localDispatcher = ForLegacyVM.INSTANCE; | ||
} | ||
dispatcher = localDispatcher; | ||
} | ||
|
||
@Override | ||
public long getEpochMicros() { | ||
return dispatcher.getEpochMicros(); | ||
} | ||
} | ||
|
||
@IgnoreJRERequirement | ||
enum ForJava8CapableVM implements SystemClock { | ||
INSTANCE; | ||
|
||
private static final Clock clock = Clock.systemUTC(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could be risky - this class cannot be fully loaded (pass linkage phase) on JRE 7. This means that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
@Override | ||
public long getEpochMicros() { | ||
// escape analysis, plz kick in and allocate the Instant on the stack | ||
final Instant now = clock.instant(); | ||
return now.getEpochSecond() * 1_000_000 + now.getNano() / 1_000; | ||
} | ||
} | ||
|
||
enum ForLegacyVM implements SystemClock { | ||
INSTANCE; | ||
|
||
@Override | ||
public long getEpochMicros() { | ||
return System.currentTimeMillis() * 1_000; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,9 +7,9 @@ | |
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
|
@@ -37,9 +37,9 @@ void setUp() { | |
|
||
@Test | ||
void testEpochMicros() { | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unit-test JRE 7 code as well (regardless of the actual JRE running the tests) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
final int nanoTime = 1000; | ||
assertThat(epochTickClock.getEpochMicros(nanoTime)).isEqualTo(epochMillis * 1000 + TimeUnit.NANOSECONDS.toMicros(nanoTime)); | ||
assertThat(epochTickClock.getEpochMicros(nanoTime)).isEqualTo(epochMicros + TimeUnit.NANOSECONDS.toMicros(nanoTime)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
package co.elastic.apm.impl.transaction; | ||
|
||
import org.junit.jupiter.api.Test; | ||
|
||
import java.util.concurrent.TimeUnit; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.assertj.core.data.Offset.offset; | ||
|
||
class SystemClockTest { | ||
|
||
@Test | ||
void testClocks() { | ||
final long currentVmEpochMicros = SystemClock.ForCurrentVM.INSTANCE.getEpochMicros(); | ||
final long java8EpochMicros = SystemClock.ForJava8CapableVM.INSTANCE.getEpochMicros(); | ||
final long java7EpochMicros = SystemClock.ForLegacyVM.INSTANCE.getEpochMicros(); | ||
assertThat(java8EpochMicros).isCloseTo(java7EpochMicros, offset(TimeUnit.SECONDS.toMicros(10))); | ||
assertThat(java8EpochMicros).isCloseTo(currentVmEpochMicros, offset(TimeUnit.SECONDS.toMicros(10))); | ||
assertThat(java7EpochMicros % 1000).isEqualTo(0); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.