feat: TTID pre-launch capture for cross-platform SDKs support - #3635
feat: TTID pre-launch capture for cross-platform SDKs support#3635sbarrio wants to merge 13 commits into
Conversation
sbarrio
changed the title
29d98ba to
75a5402
Compare
75a5402 to
f21cea2
Compare
![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: f21cea22f5
ℹ️ 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".
Was this happening only on React Native or on ordinary Android as well? |
I believe this was fixed in #3349 |
| } | ||
| } | ||
|
|
||
| private fun subscribeToFirstFrameDrawn( |
There was a problem hiding this comment.
Are you sure we need to do these changes? I did them when I fixed a memory leak (#3349) and it seems you reverted it to some prior state.
There was a problem hiding this comment.
I know, we preserved the fix but in this PR the first frame subscription responsibility is moved out of RumAppStartupDetectorImpl entirely.
For the pre-launch path, AppLaunchPreInitCollector owns the subscription and uses a Handle (same as your #3349 fix) to unsubscribe on activity destroy. For the warm-launch path, RumFeature manages handles in a WeakHashMap<Activity, RumFirstDrawTimeReporter.Handle>. The subscription is just owned at a higher level since the pre-launch module needs direct access to the first-frame timing.
You can see the changes at 9bbeb22
|
I had a look at this PR and have the following concerns:
IIUC the main problem for RN SDK is that it is initialized much later than
I didn't thoroughly check that there are no hard issues with this approach, but at the first glance it should work. It will improve code reuse and should be simpler than the current solution. |
| * On API 23, falls back directly to DdRumContentProvider.createTimeNs. | ||
| */ | ||
| @Suppress("NewApi") // Process.getStartElapsedRealtime is guarded by isAtLeastN check | ||
| internal fun computeProcessStartNs(): Long { |
There was a problem hiding this comment.
Looks like this function duplicates logic from DefaultAppStartTimeProvider. Is it possible to avoid that my reusing code in any way (extracting to a function/class/etc)?
There was a problem hiding this comment.
Indeed, thanks for raising.
I extracted it to guardedProcessStartNs(computed, fallback, thresholdNs) in ProcessStartUtils.kt. Both AppLaunchPreInitCollector and DefaultAppStartTimeProvider call this shared function. They use different clocks (elapsedRealtime vs uptime) but the guard logic is identical.
Changes at f288419
| val weakActivity = collector.activity!! | ||
| val hasSavedInstanceStateBundle = collector.hasSavedInstanceState | ||
|
|
||
| return if (collector.isFirstActivityForProcess) { |
There was a problem hiding this comment.
This looks like duplication of logic from RumAppStartupDetectorImpl. Is it possible to avoid it?
There was a problem hiding this comment.
Agree.
Changes at 5a6267f
The cold/warm classification logic is now in a single RumStartupScenario.build(...) factory method on the companion object of RumStartupScenario. Both RumAppStartupDetectorImpl.onBeforeActivityCreated() and RumFeature.constructScenario() call this factory.
| return null | ||
| } | ||
|
|
||
| if (!configuration.appStartupActivityPredicate.shouldTrackStartup(activity)) { |
There was a problem hiding this comment.
Question. Suppose AppStartupActivityPredicate skips the first Activity. Will we detect the app launch from the second Activity? Because IIUC the new AppLaunchPreInitCollector only subscribes to the first Activity's first frame.
There was a problem hiding this comment.
Yes, the second activity is correctly tracked. AppLaunchPreInitCollector records the very first activity unconditionally (it runs pre-SDK init, so it can't apply the predicate yet). When RumFeature initializes, constructScenario() checks the predicate against the captured activity, if its rejected, it returns null.
initRumAppStartupDetector() treats null as "pre-launch data not usable" and falls back to the normal RumAppStartupDetector, which handles predicate-excluded activities via onNextActivityCreated.
I also added a specific unit test for this.
You can see the changes at 5a6267f
| // GlobalRumMonitor is not yet registered during onInitialize. Rum.kt calls | ||
| // pendingPreLaunchAction on the main thread after GlobalRumMonitor.registerIfAbsent(), | ||
| // guaranteeing the real monitor is available regardless of which thread Rum.enable() | ||
| // is called on (main thread for native Android, background thread for RN/Flutter). | ||
| // | ||
| // Additionally, the Activity has already completed its full lifecycle before the | ||
| // SDK initialized (e.g. a cross-platform bridge delay). The view tracking strategy | ||
| // missed onActivityStarted/onActivityResumed, so no RUM view has been started yet. | ||
| // We replay the relevant lifecycle callback here so startView is queued before | ||
| // AppStart/TTID. | ||
| val capturedStrategy = viewTrackingStrategy |
There was a problem hiding this comment.
I'm having hard time understanding this comment and code afterwards. It looks weird that we need to replay viewTrackingStrategy here. Very complicated and error-prone. Do we really need it? Could you give an example when it is needed?
There was a problem hiding this comment.
We need this because when the SDK initializes after the app has already launched, the viewTrackingStrategy has never received onActivityStarted/onActivityResumed for the startup Activity. If we send AppStart/TTID without first opening a RUM view, those events are dropped with no view to attach to.
However I agree that it was too complicated so I've simplified it. I've introduced an opt-in interface named ReplayableViewTrackingStrategy { fun onLateActivityReady(activity: Activity) }. Both ActivityViewTrackingStrategy and NavigationViewTrackingStrategy implement it. RumFeature now just calls (viewTrackingStrategy as? ReplayableViewTrackingStrategy)?.onLateActivityReady(activity). That way we can have a clean fallback and no type checking.
@aleksandr-gringauz Thanks a ton for the thorough review and all the comments 🙌 Let me process them all and I'll come back to you shortly 🙇 |
…on activity destroy
… guard duplication
…ewTrackingStrategy
f21cea2 to
732b1f4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 732b1f4dc5
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 859ab0dfc1
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01391aa86f
ℹ️ 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".
@aleksandr-gringauz This was only happening on React Native (and most probably on Flutter, but I didn't test it there). The Android SDK initializes in Application.onCreate, before the first Activity is created, so there's always an open RUM view before sendTTIDEvent is called, that's not true for RN (and other Cross-platform SDKs), since they initialize their bridge much later. In the case of RN this happens when initialize is called and the native Rum.enable() takes place. |
Yes, and the fix from #3349 is preserved in this branch. We extended the same Handle pattern you introduced there to cover the new AppLaunchPreInitCollector subscription: subscribeToFirstFrameDrawn now returns a Handle, and we call handle.unsubscribe() if the activity is destroyed before its first frame fires. The RumFirstDrawTimeReporter.Handle interface follows the exact same design as your fix. You can see the changes at 9bbeb22 |
Sorry for the delay, it took me a while to get the PR back in shape. The main problem with your approach is that if we delegate the logic to each Cross Platform SDK we will end up having to write and maintain the same Content provider and logic on each independent SDK instead of having it written once by having it centralized on the Android SDK. Our approach also allows cross platform SDKs to immediately benefit from this by simply adding the prelaunch module dependency, without any further changes. I do however agree that we could rename the Regarding the other issues you raised, I've refactored, removed deduplication and solved several issues on the latest commits, so hopefully it all looks a bit better now. |
4 participants
What does this PR do?
Builds on the work developed by @marco-saia-datadog here: #3371 and updates it so it properly works and reports TTID on Android on cross platform SDKs that include the new
com.datadoghq:dd-sdk-android-rum-prelaunchmodule.This PR adds three things:
New
AppLaunchPreInitCollectorindd-sdk-android-internal. It's a singleton that collects timing data before the SDK initializes — process start time, firstActivity.onCreate, and first frame drawn. State transitions use atomic compare-and-swap (NOT_INSTALLED → IDLE → CAPTURING / CLAIMED → COMPLETE) so the collector and the SDK can't race on who drives startup.New
dd-sdk-android-rum-prelaunchmodule with a singleContentProvider(AppLaunchCollectorProvider) that installs the collector automatically at process start. No customer code required.RumFeature.initRumAppStartupDetector()now checks collector state on init: if data is already captured, read it; if capture is in progress, subscribe; if not installed or the SDK got there first, fall back to the existingRumAppStartupDetectorflow unchanged.RumFirstDrawTimeReporterandWindowCallbacksRegistryare also moved todd-sdk-android-internal, since both paths need them now. The originals indd-sdk-android-rumare deleted.On top of Marco's work, this PR fixes two issues that prevented the feature from working correctly:
1. TTID/TTFD events not assigned to a view
The app start and TTID events were being sent before the first RUM view was started, so they weren't attached to any view in the session. The fix defers those events via a
pendingPreLaunchActionthat is dispatched on the main thread afterGlobalRumMonitor.registerIfAbsent()runs, ensuring the view is already open when the events arrive.2. Memory leak in
RumFirstDrawTimeReporterImplWhen subscribing to first-frame events for an activity that never calls
setContentView()(e.g. an interstitial that just callsstartActivity + finish()),WindowCallbacksRegistrywraps theActivity'sWindow.Callbackand stores it in aWeakHashMap<Activity, WindowCallback>. BecauseActivityitself implementsWindow.Callback,WindowCallbackends up holding a strong reference back to the map key, preventing GC from ever collecting it. TheWindowCallbackListenerthat would clean up this entry only fires ononContentChanged— which never happens ifsetContentViewis never called. This causedNoLeakAssertionFailedErrorin all TTID auto-forwarding integration tests.The fix registers an
Application.ActivityLifecycleCallbacksalongside eachWindowCallbackListener. If theActivityis destroyed beforesetContentViewis called, the callback removes the listener and breaks the strong reference. Both theActivityand the listener are held asWeakReferenceinside the cleanup callback so the registration itself creates no new retention path.Motivation
React Native and Flutter initialize the Datadog SDK from JS/Dart, well after the first activity has launched. TTID goes unreported for those SDKs unless you add native initialization (
DdSdkNativeInitialization.initFromNative()), which means native Android code in a cross-platform project.The collector sidesteps this. By the time the SDK starts, the timing data is already waiting.
Native Android apps are unaffected. If the SDK initializes before the first activity, it claims the collector and the existing
RumAppStartupDetectorpath runs exactly as before.Additional Notes
dd-sdk-android-rum-prelaunchis opt-in. Cross-platform SDKs depend on it; native apps don't.RumFirstDrawTimeReporterImpltakes an injectablewarnLoggerlambda.RumFeaturepasses one that routes throughsdkCore.internalLoggerwithTarget.TELEMETRY + Target.USER. The pre-init path defaults toLog.wsince there's no SDK available at that point.Review checklist (to be filled by reviewers)