feat(time): Add Timestamp, EpochClock and AnchoredClock (JAVA-572) - #6045
feat(time): Add Timestamp, EpochClock and AnchoredClock (JAVA-572)#6045runningcode wants to merge 1 commit into
Conversation
|
📲 Install BuildsAndroid
|
aa53f2e to
7255d5e
Compare
deb42e1 to
271cf66
Compare
a372da0 to
bec70fb
Compare
271cf66 to
ab5fba1
Compare
bec70fb to
686cc80
Compare
686cc80 to
655adfb
Compare
0xadam-brown
left a comment
There was a problem hiding this comment.
Nice! 🥇
Some comments for your consideration. (Apologies in advance for the doc suggestions, but hope they'll be helpful in a cut-and-paste way – seems worthwhile one-up the clanker because these APIs are so important.) Looking great though 💯
| import org.jetbrains.annotations.NotNull; | ||
|
|
||
| /** | ||
| * One wall-clock reading pinned to one monotonic tick, from which related instants are projected. |
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).)
| } | ||
|
|
||
| /** The anchor itself — the one instant here that was read rather than projected. */ | ||
| public @NotNull Timestamp start() { |
There was a problem hiding this comment.
l: My vote would be for started() here, since we're returning a timestamp from the past rather than staring a timer.
There was a problem hiding this comment.
I'm following Guava's convention here where static methods have the ed ending and instance methods don't.
I do see your point that this isn't the best name though as it doesn't distinguish between this and now(). I've updated to origin() to make it more clear what it does.
| } | ||
|
|
||
| /** | ||
| * How far this anchor's projection has fallen behind or ahead of the wall clock, in nanoseconds. |
There was a problem hiding this comment.
...or really, the relative distance between the original anchor wall clock time and an updated reading of that wall clock. (To my ear, the current wording makes it sound like any returned value besides 0 means the anchor has become less accurate relative to "true time", when it could have been perfectly accurate to begin with and has since skewed.)
...which raises the more fundamental question: How exactly do we expect folks to use this method? Do we plan on taking a bunch of these measures and using a computed average (or whatever) to offset what we actually report to Relay?
There was a problem hiding this comment.
This is a good point. I had a branch where I was using it to attach metadata to spans (we could also calculate sleep time by subtracting the difference of the two monotonic clocks) but both blow up the scope significantly. I will remove this!
| * How far this anchor's projection has fallen behind or ahead of the wall clock, in nanoseconds. | ||
| * | ||
| * <p>Zero means the wall clock advanced by exactly the time this clock measured. Anything else is | ||
| * a clock step, or — where {@link MonotonicClock} and the wall clock disagree about suspend — |
There was a problem hiding this comment.
l: Not quite sure I understand what "a clock step" means, and I'm skeptical that we can claim "anything else" is caused by one of these two things. (Clankers gonna clank, apparently.)
| * a clock step, or — where {@link MonotonicClock} and the wall clock disagree about suspend — | ||
| * device sleep. Reads the epoch and the tick in the same order as {@link #create}, so the gap | ||
| * between the two reads biases the result the same way it biased the anchor. | ||
| */ |
There was a problem hiding this comment.
l: Worth mentioning that this can return negative values.
| import org.jetbrains.annotations.NotNull; | ||
|
|
||
| /** | ||
| * The source of wall-clock time. |
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.
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.
|
|
||
| private InstantEpochNanos() {} | ||
|
|
||
| static long read() { |
There was a problem hiding this comment.
l: Thoughts about an API like this instead:
InstantUtils.toEpochNanos(Instant)
...fwiw, that^^ seems a bit more intuitive to me than InstantEpochNanos.read(), and it also lets callers supply their own Instant made at call time or at some other time.
Up to you of course.
There was a problem hiding this comment.
The issue with callers supplying their own Instant is that it won't compile since we still have min SDK < 26. I also personally dislike classes named with Utils after working at Square ;)
| public final class Timestamp { | ||
|
|
||
| private final long epochNanos; | ||
| private final @Nullable AnchoredClock anchor; |
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?
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
Without this, we don't have any way to check that two timestamps are being compared against the same anchor. Otherwise the timestamps are not relative to each other and the comparison is invalid.
Choice two: Single anchor or multiple anchors
An alternative design would have been where we have a single Anchor for the entire process. In that world, we don't need a way to check that we are comparing two Timestamps against the same anchor. We made the choice here to have multiple anchors. The idea was to make it easier to mesh with our current span design.
If it helps understand how the anchor method is used, check the usage here: https://github.com/getsentry/sentry-java/pull/6055/changes#diff-8fb51c60f3cd9eba198cd0702cf5b06a1b4e0ff676dccaa371840a44a5a5bd8eR334
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 Timestamps in clock and make it thread safe?
| private final @NotNull LazyEvaluator<SentryDateProvider> dateProvider = | ||
| new LazyEvaluator<>(() -> new SentryAutoDateProvider()); | ||
|
|
||
| private final @NotNull LazyEvaluator<EpochClock> epochClock = |
There was a problem hiding this comment.
l: Curious as to why we evaluate lazily here but not for the monotonic clock exposed via SentryOptions?
There was a problem hiding this comment.
that's slop, thanks for pointing this out.
| * io.sentry.time.AnchoredClock} built on this and {@link #getMonotonicClock()}. | ||
| */ | ||
| @ApiStatus.Internal | ||
| public @NotNull EpochClock getEpochClock() { |
There was a problem hiding this comment.
m: Similarly to here, do we want to expose this via SentryOptions, or just AnchoredClock instead?
There was a problem hiding this comment.
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 in Breadcrumb and SentryEvent. Do you mean to not expose this and then all the callers grab a singleton directly from whichever implementation of EpochClock they need?
There was a problem hiding this comment.
There are some uses cases where we want an epoch to serialize from EpochClock.now() for example in Breadcrumb and SentryEvent.
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.)
ab5fba1 to
d7bc897
Compare
655adfb to
0969abb
Compare
SentryDate is asked to be four things at once: an epoch instant to
serialize, one endpoint of a monotonic interval, a carrier of a hidden
System.nanoTime() reading, and an opaque foreign timestamp. Nothing in the
type separates them, so the guarantees are decided by the runtime class of
both operands -- SentryNanotimeDate.diff() is monotonic only when the other
date is also a SentryNanotimeDate, and silently subtracts two wall-clock
readings otherwise. On the JVM, where SentryAutoDateProvider picks
SentryInstantDate, neither endpoint has a monotonic component and span
durations are not monotonic at all.
The fix is not to type the instants more carefully. It is to stop producing
them independently. A group of instants that will be compared against each
other -- the spans of a transaction, the samples of a profile chunk, the
segments of a replay -- reads the epoch once and projects the rest through
the monotonic clock:
Timestamp an epoch instant, plus the anchor that projected it, or
null when it was read or stated directly. No arithmetic
between instants; equality is by instant.
EpochClock the wall clock, for stamping a moment that leaves the
process. Deliberately cannot report a duration.
AnchoredClock one epoch reading pinned to one tick. now() and at(tick)
project, tickOf() inverts exactly, driftNanos() reports how
far the projection has fallen behind the wall clock.
Subtracting two instants from one anchor is subtracting two ticks, so a
duration is monotonic by construction rather than by convention, and a clock
step cannot make a child span start before its parent. It also gives Android
nanosecond resolution it cannot read directly, the epoch being
millisecond-granular there -- the workaround SentryNanotimeDate describes,
applied once per group instead of between each pair of readings.
OpenTelemetry's SDK anchors per local root span for the same two reasons.
tickOf() refusing an instant it did not project is what makes this safer
rather than merely tidier: mixing domains becomes an exception instead of a
plausible-looking wrong number, the same guard Deadline.isAfter applies to
clocks.
Timing is dropped rather than kept. It paired one Timestamp with one
Stopwatch, which is what AnchoredClock does for a whole group, and no call
site would have wanted the single-interval version.
Nothing calls any of it yet.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0969abb to
005fa73
Compare
PR Stack (Clock semantics hardening)
📜 Description
Adds the wall-clock half of the time API —
Timestamp,EpochClockandAnchoredClock— on top ofthe
MonotonicClock/Stopwatchprimitives from #6028. Nothing calls any of it yet, like #6028.SentryOptions.getEpochClock()is the injection point.💡 Motivation and Context
Why anchoring, rather than more careful types
The bug class is not "wall vs monotonic". It is pairwise arithmetic between two independently
produced instants. Better types narrow that; they do not close it, because a mixed pair is still
reachable and still has to answer.
So the fix is to stop producing the instants independently. A group that will be compared against
each other — the spans of a transaction, the samples of a profile chunk, the segments of a replay —
reads the epoch once and projects the rest through the monotonic clock:
The projection is affine with slope 1, so subtracting two instants from one anchor is subtracting
two ticks. A duration is then monotonic by construction rather than by convention, and a clock step
cannot make a child span start before its parent. It also buys resolution the wall clock does not
have: Android's epoch is millisecond-granular, so a directly read instant is truncated while a
projected one carries nanoseconds. OpenTelemetry's SDK does the same thing, per local root span, for
the same two reasons.
tickOf()refusing an instant it did not project is what makes this safer rather than merelytidier, and it is why
Timestampreferences its anchor at all: mixing domains raises anIllegalArgumentExceptioninstead of returning a plausible-looking wrong number. An instant readstraight from the wall clock, or stated by something outside the process, has no anchor and can only
be serialized.
The epoch clock does not go through SentryDateProvider
SystemEpochClockreads the wall clock directly, picking precision the waySentryAutoDateProviderdoes:
Instant.now()on JVM 9+,System.currentTimeMillis()otherwise. Android is always thelatter —
Instantis millisecond-granular there whether or not the build desugars it (#2451). Thevalues are byte-identical to what the provider returns on every platform; what changes is that
reading one no longer allocates a
SentryDate, and on Android no longer takes aSystem.nanoTime()reading that an
EpochClocknever looks at.setDateProvidertherefore does not reach the epoch clock. Faking time means overridinggetEpochClock()onSentryOptions, the waySentryAndroidOptionsalready overridesgetMonotonicClock(). There is nosetEpochClockbecause nothing consumes it yet.💚 How did you test it?
./gradlew :sentry:test— 3551 tests, 0 failures.spotlessApply apiDumpclean; the.apidiffagainst the merge base is additions only, with no
<init>leaks.14 new tests on
AnchoredClock, including the ones that were impossible to write before:and the differences between them do not move
tickOfinverts a projection exactly, and throws for a bare instant and for another anchor's instantdriftNanos()is zero while the wall clock keeps pace, and reports the signed size of a step📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
SentryTracercreates oneAnchoredClockper transaction, before its rootSpan, and everySpanbelow projects from that anchor.
Spanthen holds twoTimestamps instead of twoSentryDates,serialization reads
end().epochNanos()instead oflaterDateNanosTimestampByDiff, and the twosentinel hacks call
anchor.tickOf(...). That retireslaterDateNanosTimestampByDiff,SentryNanotimeDate'sdiff/compareTo/nanotimeDiffoverrides,SentryAutoDateProvider/SentryInstantDate, and twoSentryDateallocations per span endpoint.Two things that PR has to settle, flagged here so they get argued before code exists:
Choreographerhands us frame timestamps in theSystem.nanoTime()timebase, whileAndroidMonotonicClockisSystemClock.elapsedRealtimeNanos(); the two differ by accumulatedsuspend. The plan is to keep a single
MonotonicClockand put that last hop insideSpanFrameMetricsCollector, which already has anonSpanStartedhook to capture both readings andcan detect sleep by comparing the deltas — skipping attribution honestly rather than returning a
plausible-but-wrong projection.
start_timestampby sub-millisecondamounts (root spans are unaffected — the anchor is read at root start). Under the "serialized values
are frozen until the major" rule that puts the flip behind v9, even though it is an improvement.
Separately, JAVA-572's original subject — the tracer idle/deadline timeout — is stale on the timer half
(
SentryTraceralready usesgetTimerExecutorService()), but "clamp the finish timestamp when thedeadline fires late" is still real and is the actual fix for the multi-hour
ui.loadartifact. Itwants its own ticket.
QueuedThreadPoolExecutor's wall-clock backoff is internal control flow, so itcan be fixed before the major, like #6030.