Fix timing of ApplicationLaunch view and application_start events#1305
Conversation
The events were incorrectly using the nano time for their timestamp.
fe5c4f1 to
062424a
Compare
| @WorkerThread | ||
| private fun startApplicationLaunchView(event: RumRawEvent, writer: DataWriter<Any>) { | ||
| val applicationLaunchViewTimeNs = appStartTimeProvider.appStartTimeNs | ||
| // appStartTimeNs is based on elapsed system time. Timestamp needs to be currentMs. |
There was a problem hiding this comment.
I find this description confusing. timestamp doesn't need to be current time.
Basically Time#timestamp and Time#nanoTime are pointing to the same instant (moment of the time), while having different base - timestamp is based on the Unix epoch start, nanoTime is based on the JVM start time (for Android I think it is effectively the process start/fork time).
So I guess the comment wanted to say that timestamp should have the same base as System.currentTimeMillis (just the shortened abbreviation is used)?
So what we are doing below is first calculating the different between 2 instant representations, which is event.eventTime.timestamp - TimeUnit.NANOSECONDS.toMillis(event.eventTime.nanoTime) to find out when actually process has started in terms of epoch time, and then apply the time since this process start (which is applicationLaunchViewTimeNs) to find when event happened in the epoch time format.
So I would suggest for the clarity to introduce the variable processStartNs with the description what is this (also maybe we need to keep calculation in the ns and convert to the ms at the very end, better probably precision-wise (although 10^6 different between ns and ms will erase this precision gain anyway)), because I think without it the expression below is not quite clear.
There was a problem hiding this comment.
👍 I'll take another stab at it.
|
@0xnm took another stab at it. Let me know if that's clearer. |
| // to convert it to milliseconds since epoch provided by System.currentTimeMillis. | ||
| // To do so, we take the offset of those times in the event time, which should be consistent, | ||
| // then add that to our processStartTime to get the correct value. | ||
| val timestampNs = (TimeUnit.MILLISECONDS.toNanos(event.eventTime.timestamp - event.eventTime.nanoTime)) + |
There was a problem hiding this comment.
shouldn't it be like (TimeUnit.MILLISECONDS.toNanos(event.eventTime.timestamp) - event.eventTime.nanoTime) here? given that nanoTime is actually in nanoseconds.
79b13e3 to
5483718
Compare
dea1292 to
99e578a
Compare
Also fix updating RUM context on session start.
3c60f53 to
442ec60
Compare
| sessionState = if (keepSession) State.TRACKED else State.NOT_TRACKED | ||
| sessionId = UUID.randomUUID().toString() | ||
| sessionStartNs.set(nanoTime) | ||
| sdkCore.updateFeatureContext(RumFeature.RUM_FEATURE_NAME) { |
There was a problem hiding this comment.
good catch. While it probably makes sense to update session ID when session is renewed, in the end the only real top-level entity is RUM view, and once RUM view is created, it will be associated with the proper session anyway. So I guess the only impact without this change lies in the Logs/Traces domain, they can have old RUM session ID attached.
Codecov Report
@@ Coverage Diff @@
## develop #1305 +/- ##
===========================================
- Coverage 81.97% 81.93% -0.03%
===========================================
Files 362 362
Lines 12855 12859 +4
Branches 2155 2155
===========================================
- Hits 10537 10536 -1
- Misses 1662 1665 +3
- Partials 656 658 +2
|
4 participants
What does this PR do?
Fix the timing of ApplicaitonLaunch vie and application_start events. The events were incorrectly using the nano time for their timestamp instead of using an ms timestamp. We use the difference between the triggering event's timestamp and nano time to determine how far apart they are, and add that to the application start time.
Review checklist (to be filled by reviewers)