RUM-15483: Fix clock mismatch in DefaultAppStartTimeProvider app start time computation#3587
Conversation
…t time computation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2483ebd1e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| buildSdkVersionProvider.isAtLeastN -> { | ||
| val timeProvider = timeProviderFactory() | ||
| val diffMs = timeProvider.getDeviceElapsedRealtimeMillis() - Process.getStartElapsedRealtime() | ||
| val diffMs = timeProvider.getDeviceUptimeMillis() - Process.getStartUptimeMillis() |
There was a problem hiding this comment.
In the sleep-after-process-start case this now stores a sleep-excluding uptime/nanoTime value, but RUM still converts sdkCore.appStartTimeNs to a Time via asTimeNs() and uses scenario.initialTime.timestamp for TTID/TTFD startup events in RumSessionScopeStartupManager. Since wall time advances during device sleep while uptimeMillis()/nanoTime() do not, those startup events are timestamped late by exactly the sleep duration; keeping only this uptime-domain value fixes durations but corrupts event timing, so the timestamp path needs a separate boottime/wall-clock process start value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I don't think this PR is making it worse or better, but I could use a second opinion here if anyone thinks otherwise.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3587 +/- ##
========================================
Coverage 72.52% 72.53%
========================================
Files 978 978
Lines 36052 36046 -6
Branches 6038 6040 +2
========================================
- Hits 26146 26143 -3
+ Misses 8234 8228 -6
- Partials 1672 1675 +3
🚀 New features to boost your workflow:
|
5 participants
What does this PR do?
Fixes a clock-domain mismatch in
DefaultAppStartTimeProviderwhere the app start time computation mixedCLOCK_BOOTTIME(elapsedRealtime/Process.getStartElapsedRealtime()) withCLOCK_MONOTONIC(System.nanoTime()). The fix replaces theCLOCK_BOOTTIMEpair with a fullyCLOCK_MONOTONICcomputation usingSystemClock.uptimeMillis()/Process.getStartUptimeMillis().To support this,
getDeviceUptimeMillis()is added to theTimeProviderinterface and implemented inBaseTimeProviderasSystemClock.uptimeMillis().Motivation
On devices that sleep between process start and the first lazy access of
appStartTimeNs(background launches, OEM battery optimization), the old formula produced a value that was too small by exactly the sleep duration. This silently inflated TTID, app startup duration,timeSinceAppStartNson fatal/ANR errors, and NDK crash timestamps. The bug was identified while investigating the CI flake fixed in RUM-15269 (PR #3339).Additional Notes
Process.getStartUptimeMillis()is available from API 24+ (confirmed from AOSP source) — no API level branching is required. It is already pre-approved indetekt_custom_safe_calls_android.yml.Related to: #3339
Review checklist (to be filled by reviewers)