feat(time): Add Timestamp, EpochClock and AnchoredClock (JAVA-572) #6045
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| package io.sentry.time; | ||
|
|
||
| import org.jetbrains.annotations.ApiStatus; | ||
| import org.jetbrains.annotations.NotNull; | ||
|
|
||
| /** | ||
| * One wall-clock reading pinned to one monotonic tick, from which related instants are projected. | ||
|
Member
There was a problem hiding this comment. If you're up for improving on the clanker, maybe something like: * An {@link EpochClock} and a {@link MonotonicClock} that together let us generate multiple {@link
* Timestamp}s (aka, instants) that share a common epochal anchor.
*
* <p>Lets us:
*
* <ol>
* <li>safely create accurate interval timings (e.g., for spans) by avoiding clock drift, NTP
* resets, or other sources of skew that intervals are subject to when they're computed from
* multiple wall clock readings; and
* <li>produce intervals with nanosecond precision.
* </ol>
*
* <p>(Item (2) especially helpful on Android, as nano-precise system time is only available for API
* 33+.)
*
* <p>Because {@code AnchoredClock} uses a single epochal time source, the timestamps it generates
* are accurate relative to one another, but are always subject to any initial inaccuracy of the
* epochal anchor. The {@link #driftNanos} method lets you estimate that inaccuracy by comparing
* against more current wall clock readings.Up to you of course... (If your curious, you can see the comment here for one of the unexpected – and impactful – consequences of not having (2).) |
||
| * | ||
| * <p>Exists because a group of instants that will be compared against each other — the spans of a | ||
| * transaction, the samples of a profile chunk, the frames of a replay segment — must not each read | ||
| * the wall clock. Two independent readings differ by whatever the device's clock did in between, so | ||
| * a duration taken across them can shorten, lengthen or go negative, and a child can appear to | ||
| * start before its parent. Reading the epoch once and projecting the rest through {@link | ||
| * MonotonicTicker} makes every instant an image of the same tick, so subtracting any two of them | ||
| * reports measured time. The span protocol needs exactly that: it carries a start and an end | ||
| * instant and no duration field, so the server subtracts them. | ||
| * | ||
| * <p>Projection also buys resolution the wall clock does not have: on Android the epoch is | ||
| * millisecond-granular, so an instant read directly is truncated, whereas one projected from a tick | ||
| * carries nanoseconds. That is the workaround {@link io.sentry.SentryNanotimeDate} describes, | ||
| * applied once per group rather than to every reading. OpenTelemetry's SDK anchors per local root | ||
| * span for the same two reasons. | ||
| * | ||
| * <p>The cost is that a projection ages: it reports what the wall clock said when the anchor was | ||
| * taken plus the time measured since, so a later correction to the device's clock — an NTP sync, or | ||
| * the user setting the time — never reaches it. Anchor something short-lived. | ||
| */ | ||
| @ApiStatus.Internal | ||
| public final class AnchoredClock { | ||
|
|
||
| private final @NotNull MonotonicTicker ticker; | ||
| private final long epochNanos; | ||
| private final long anchorTick; | ||
|
|
||
| private AnchoredClock( | ||
| final @NotNull MonotonicTicker ticker, final long epochNanos, final long anchorTick) { | ||
| this.ticker = ticker; | ||
| this.epochNanos = epochNanos; | ||
| this.anchorTick = anchorTick; | ||
| } | ||
|
|
||
| /** Takes the anchor now: one epoch reading, one tick, as close together as a call allows. */ | ||
| public static @NotNull AnchoredClock create( | ||
| final @NotNull EpochClock epoch, final @NotNull MonotonicTicker ticker) { | ||
| return new AnchoredClock(ticker, epoch.now().epochNanos(), ticker.tickNanos()); | ||
| } | ||
|
|
||
| /** | ||
| * The instant the anchor was taken — the one instant here that was read rather than projected. | ||
| * | ||
| * <p>Reads no clock and never changes. Every other instant this class returns is this one plus | ||
| * measured time. | ||
| */ | ||
| public @NotNull Timestamp origin() { | ||
| return Timestamp.anchoredAt(epochNanos, this); | ||
| } | ||
|
|
||
| /** The current instant: {@link #origin()} plus the time the ticker has measured since. */ | ||
| public @NotNull Timestamp now() { | ||
| return at(ticker.tickNanos()); | ||
| } | ||
|
|
||
| /** | ||
| * The instant a tick corresponds to, for placing something already measured on this ticker — a | ||
| * frame, a profiler sample — on the same timeline as the instants projected here. | ||
| */ | ||
| public @NotNull Timestamp at(final long tickNanos) { | ||
| return Timestamp.anchoredAt(epochNanos + (tickNanos - anchorTick), this); | ||
| } | ||
|
|
||
| /** | ||
| * The tick an instant was projected from. Exact, and reads no clock: projection adds a tick | ||
| * difference to a fixed epoch, so subtraction inverts it. | ||
| * | ||
| * @throws IllegalArgumentException if this clock did not project the instant. Its epoch bears no | ||
| * arithmetic relation to these ticks, so converting it would silently produce a tick derived | ||
| * from a wall-clock difference. | ||
| */ | ||
| public long tickOf(final @NotNull Timestamp timestamp) { | ||
| if (timestamp.anchor() != this) { | ||
| throw new IllegalArgumentException( | ||
| "Timestamp was not projected by this AnchoredClock: " + timestamp); | ||
| } | ||
| return anchorTick + (timestamp.epochNanos() - epochNanos); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| package io.sentry.time; | ||
|
|
||
| import org.jetbrains.annotations.ApiStatus; | ||
| import org.jetbrains.annotations.NotNull; | ||
|
|
||
| /** | ||
| * The source of wall-clock time. | ||
|
Member
There was a problem hiding this comment. Maybe: * A source of wall-clock {@link Timestamp}s (aka, instants).
*
* <p>This class should <b>not</b> be used to compute durations. Instead, see {@link AnchoredClock}
* if you need shareable timestamps for an interval's start and end point, or {@link Deadline} if
* you need a monotonic endpoint. Again, up to you – but could be worth avoiding the clanker for these Javadocs as clarity is esp valuable for such central APIs.
Contributor
Author
There was a problem hiding this comment. I think the wording on "will leave this process" is important. i'll add your point about not computing durations but otherwise i thought the existing comment was good. |
||
| * | ||
| * <p>Stamps a moment that will leave this process — an event, a breadcrumb, a session — and nothing | ||
| * else. It deliberately cannot report a duration: measuring belongs to {@link Stopwatch}, and a | ||
| * group of instants that will be subtracted from each other belongs to an {@link AnchoredClock}, | ||
| * which reads this once and projects the rest. | ||
| */ | ||
| @ApiStatus.Internal | ||
| public interface EpochClock { | ||
|
|
||
| /** The current instant. Serialize it; do not subtract it from another one. */ | ||
| @NotNull | ||
| Timestamp now(); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| package io.sentry.time; | ||
|
|
||
| import io.sentry.DateUtils; | ||
| import java.time.Instant; | ||
| import org.jetbrains.annotations.ApiStatus; | ||
|
|
||
| /** | ||
| * Reads the epoch from {@link Instant}. | ||
| * | ||
| * <p>A class of its own so the reference to {@code java.time} is loaded only where {@link | ||
| * SystemEpochClock} decided to use it. Android's minSdk is below the API 26 that introduced {@code | ||
| * Instant}. | ||
| */ | ||
| @ApiStatus.Internal | ||
| @SuppressWarnings("NewApi") | ||
| final class InstantEpochNanos { | ||
|
|
||
| private InstantEpochNanos() {} | ||
|
|
||
| static long read() { | ||
|
Member
There was a problem hiding this comment. l: Thoughts about an API like this instead: ...fwiw, that^^ seems a bit more intuitive to me than Up to you of course.
Contributor
Author
There was a problem hiding this comment. The issue with callers supplying their own |
||
| final Instant now = Instant.now(); | ||
| // No long overflow until year 2262 | ||
| return DateUtils.secondsToNanos(now.getEpochSecond()) + now.getNano(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| package io.sentry.time; | ||
|
|
||
| import io.sentry.DateUtils; | ||
| import io.sentry.util.Platform; | ||
| import org.jetbrains.annotations.ApiStatus; | ||
| import org.jetbrains.annotations.NotNull; | ||
|
|
||
| /** | ||
| * The {@link EpochClock} backed by the system wall clock. | ||
| * | ||
| * <p>Reads the epoch at the best precision the platform offers: {@link java.time.Instant} where it | ||
| * is sub-millisecond, {@link System#currentTimeMillis()} everywhere else. Android is always the | ||
| * latter — {@code Instant} is millisecond-granular there whether or not the build desugars it, see | ||
| * https://github.com/getsentry/sentry-java/pull/2451. | ||
| * | ||
| * <p>A millisecond anchor loses less than it looks: an {@link AnchoredClock} adds nanosecond ticks | ||
| * to one anchor, so only the anchor is coarse. | ||
| */ | ||
| @ApiStatus.Internal | ||
| public final class SystemEpochClock implements EpochClock { | ||
|
|
||
| private static final boolean INSTANT_IS_SUB_MILLISECOND = | ||
| Platform.isJvm() && Platform.isJavaNinePlus(); | ||
|
|
||
| private static final SystemEpochClock instance = new SystemEpochClock(); | ||
|
|
||
| public static @NotNull EpochClock getInstance() { | ||
| return instance; | ||
| } | ||
|
|
||
| private SystemEpochClock() {} | ||
|
|
||
| @Override | ||
| public @NotNull Timestamp now() { | ||
| return Timestamp.ofEpochNanos( | ||
| INSTANT_IS_SUB_MILLISECOND | ||
| ? InstantEpochNanos.read() | ||
| : DateUtils.millisToNanos(System.currentTimeMillis())); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| package io.sentry.time; | ||
|
|
||
| import org.jetbrains.annotations.ApiStatus; | ||
| import org.jetbrains.annotations.NotNull; | ||
| import org.jetbrains.annotations.Nullable; | ||
|
|
||
| /** | ||
| * An instant on the wall clock, as nanoseconds since the Unix epoch. | ||
| * | ||
| * <p>Unlike a {@link MonotonicTicker} tick, a timestamp means something outside this process: it | ||
| * can be serialized, stored, and compared against a value from another machine. | ||
| * | ||
| * <p>It deliberately offers no arithmetic between instants. Subtracting two independent wall-clock | ||
| * readings gives a duration the device's clock can lengthen, shorten or make negative. Durations | ||
| * come from a {@link Stopwatch}, or from two instants an {@link AnchoredClock} projected from the | ||
| * same tick. | ||
| * | ||
| * <p>{@link #anchor()} records which of those this is. An instant read straight from the wall | ||
| * clock, or stated by something outside this process, has no anchor and can only be serialized. One | ||
| * an {@link AnchoredClock} produced references that clock, which lets {@link AnchoredClock#tickOf} | ||
| * recover the tick it came from and reject instants it did not produce. | ||
| * | ||
| * <p>Nanoseconds since the epoch overflow a long in the year 2262. | ||
| */ | ||
| @ApiStatus.Internal | ||
| public final class Timestamp { | ||
|
|
||
| private final long epochNanos; | ||
| private final @Nullable AnchoredClock anchor; | ||
|
Member
There was a problem hiding this comment. m: This is surprising to me, as I'd expect the dependency to be solely from AnchoredClock -> Timestamp, rather than circular. It's also an instance that can't be gc'd when it otherwise might be. Thoughts about living without this field? Or do we really need AnchoredClock.tickOf() (its sole caller at present via Timestamp.anchored())? Not sure about what you had in mind for tickOf() and anchored(), but maybe we could live with a general purpose tickOf() that returns a long based on its AnchoredClock without caring where it came from / trusting the caller?
Contributor
Author
There was a problem hiding this comment. This is a good point. 😄 There are two design decisions that impact this here. The second one we discussed on the call but I'll repeat it here. Choice one: Ability to verify we are comparing against the same anchor Choice two: Single anchor or multiple anchors If it helps understand how the I didn't follow your point about GC, can you explain it to me? If we do reverse the dependency, what data structure would we use to keep track of all the
Member
There was a problem hiding this comment.
B/c each Timestamp has a reference to its AnchoredClock, no AnchoredClock can be gc'd until all the Timestamps it created are gone too. If a Timstamp escapes somewhere, the corresponding AnchoredClock has to stick around with it.
We could use an ID or token of some sort if we wanted to avoid passing the whole AnchoredClock to Timestamp. Even a marker object would do. Happy to leave it to your discretion – I was mainly surprised to see us passing AnchoredClock, but nothing that has to block us.
Quoting my original question again just so it doesn't get lost in the back-and-forth: do we know that we need tickOf() at all? Put another way, do we want to support inverse mappings from Timestamps back to ticks / longs? I'm fine with that if we have a good use case or two (and thanks for providing the link in your last comment!). I just don't know (yet) how easily that example could be worked around vs how much we need the inverse mappings, as I haven't dug into the application PRs much. Feel free to disregard if we really do need it, but worth a second thought in case we can do without it. (Do java.time, kotlin.time, or Guava support Instant -> tick mappings? They'll be lots more insightful than I am, of course...) |
||
|
|
||
| private Timestamp(final long epochNanos, final @Nullable AnchoredClock anchor) { | ||
| this.epochNanos = epochNanos; | ||
| this.anchor = anchor; | ||
| } | ||
|
|
||
| /** An instant read straight from a wall clock, or stated by something outside this process. */ | ||
| public static @NotNull Timestamp ofEpochNanos(final long epochNanos) { | ||
| return new Timestamp(epochNanos, null); | ||
| } | ||
|
|
||
| static @NotNull Timestamp anchoredAt(final long epochNanos, final @NotNull AnchoredClock anchor) { | ||
| return new Timestamp(epochNanos, anchor); | ||
| } | ||
|
|
||
| public long epochNanos() { | ||
| return epochNanos; | ||
| } | ||
|
|
||
| /** The clock that projected this instant, or null if it was read or stated directly. */ | ||
| @Nullable | ||
| AnchoredClock anchor() { | ||
| return anchor; | ||
| } | ||
|
|
||
| /** | ||
| * Equality is by instant. The anchor records how the instant was obtained, not what it denotes, | ||
| * so two readings of the same moment are equal whether or not they were projected. | ||
| */ | ||
| @Override | ||
| public boolean equals(final @Nullable Object other) { | ||
| if (this == other) { | ||
| return true; | ||
| } | ||
| if (!(other instanceof Timestamp)) { | ||
| return false; | ||
| } | ||
| return epochNanos == ((Timestamp) other).epochNanos; | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return (int) (epochNanos ^ (epochNanos >>> 32)); | ||
| } | ||
|
|
||
| @Override | ||
| public @NotNull String toString() { | ||
| return "Timestamp{epochNanos=" + epochNanos + '}'; | ||
| } | ||
| } | ||
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.
m: Similarly to here, do we want to expose this via SentryOptions, or just AnchoredClock instead?
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.
I understand that there's no PR right now that uses this so maybe it is confusing.
There are some uses cases where we want an epoch to serialize from
EpochClock.now()for example inBreadcrumbandSentryEvent. Do you mean to not expose this and then all the callers grab a singleton directly from whichever implementation ofEpochClockthey need?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.
That sounds a good enough reason to expose this to me. (Just wanted to make sure we had considered whether hiding some of these abstractions was possible / desired. No concerns about exposing them if we have specific use cases supporting it.)