Repository navigation
feat: TTID pre-launch capture for cross-platform SDKs support - #3635
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. |
| // 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 🙇 |
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. |
|
I had another look at this PR.
Got it. For me it is ok to have The main problems I have (still after the fixes) with the current PR:
I created a draft PR (#3795) where I created a PoC implementation. Heavily used AI, I didn't check if it really works with RN sdk. But just to illustrate what I suggested in my previous comment. Some notes about the PR:
Hope it clarifies and helps a bit. And again - I didn't thoroughly check the PR, I might have forgotten something important. |
01391aa to
1dcc7d4
Compare
|
✅ All CI checks and tests passed. Datadog automation helped this PR pass. 🎉 All green!🧪 All tests passed 🔄 Datadog retried 2 tests - 2 passed on retry 🎯 Code Coverage (details) 🔗 Commit SHA: 0efc644 | Docs | View more details | Give us feedback! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1dcc7d400a
ℹ️ 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: a3d18ddd2c
ℹ️ 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: 24551662a0
ℹ️ 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".
hamorillo
left a comment
hamorillo
left a comment
There was a problem hiding this comment.
Thanks for the changes! I left some comments, but nothing critical.
aleksandr-gringauz
left a comment
aleksandr-gringauz
left a comment
There was a problem hiding this comment.
I had another look and it seems almost ready! I left some more comments, but I believe they should be easy to fix.
| scenario: RumStartupScenario, | ||
| durationNs: Long, | ||
| wasForwarded: Boolean = false, | ||
| forwardedActivity: WeakReference<Activity>? = null |
There was a problem hiding this comment.
forwardedActivity seems to be unused.
| private val handler: Handler | ||
| private val handler: Handler, | ||
| private val logTag: String = "DD/AppLaunch", | ||
| private val warnLogger: (message: String, throwable: Throwable) -> Unit = { message, throwable -> |
There was a problem hiding this comment.
Let's remove the default warnLambda value from here and instead specify InternalLogger.UNBOUND in PreLaunchRumAppStartupDetector.kt when creating an instance of RumFirstDrawTimeReporterImpl.
| val importance = DdRumContentProvider.processImportance | ||
| if (importance != ActivityManager.RunningAppProcessInfo.IMPORTANCE_FOREGROUND) { | ||
| return false | ||
| } |
There was a problem hiding this comment.
I don't think that we need to filter out non-foreground importance here. There is actually no guarantee that for foreground launches the process importance in content provider will be foreground on all versions on Android.
RumAppStartupDetector will take care of determining what type of start (cold/warm) it is.
You don't have to write any specific logic here. Just calling PreLaunchRumAppStartupDetector should be enough.
| // Rum.enable() was called on (the main thread for native Android, a background thread for | ||
| // React Native / Flutter). | ||
| if (rumFeature.usePreLaunchDetector) { |
There was a problem hiding this comment.
Is Rum.enable really called on non-ui thread in react-native/flutter?
In any case, our current RumFeature implementation isn't written for multi-threaded usage (and never was). So I suggest we simply ignore this problem for now here. Either we need to fix it everywhere or not fix at all.
The way it is now (with handler.post) in the PR it introduces too much additional complexity.
And I believe attachPreLaunchRumAppStartupDetector can be made private and called from RumFeature.onInitialized without any handler.post.
There was a problem hiding this comment.
AFAIK Flutter does it on the main thread, while RN does in a background one. We are aware of this on RN but it's the only choice we have today. I agree that this is too much additional complexity for what it is.
Removed at 0efc644
…kt_custom_safe_calls_third_party.yml.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0efc644b74
ℹ️ 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".
5 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.Updated implementation (post-review)
The original approach, a CAS state-machine collector in
dd-sdk-android-internal, has been replaced. The module layout, the hand-off mechanism, and the reason TTID lands on a view are all different now. The two fixes described in the superseded description below are also outdated: one is obsolete, the other was the wrong diagnosis.This follows the proposal made by @aleksandr-gringauz on #3795.
Two-module split replaces the single pre-launch module
AppLaunchPreInitCollectorand itsNOT_INSTALLED → IDLE → CAPTURING / CLAIMED → COMPLETEstate machine are gone, along withRumFirstDrawTimeReporterandWindowCallbacksRegistryindd-sdk-android-internal(and their tests).dd-sdk-android-internalno longer carries any RUM startup code.In their place, two modules:
features/dd-sdk-android-rum-internal(new, published) — the shared startup code both consumers need:PreLaunchRumAppStartupDetector,RumAppStartupDetector+Impl,RumFirstDrawTimeReporter+Impl,WindowCallbacksRegistry,RumStartupScenario,RumTTIDInfo,domain/Time. It depends only ondd-sdk-android-internal— deliberately not on core, so it can load before the SDK exists.features/dd-sdk-android-rum-prelaunch— now justAndroidManifest.xml+AppLaunchCollectorProvider.kt.The split exists to break a circular dependency:
-rumneeds the startup types, and-rum-prelaunchneeds them before-rumis initialized.Buffer-and-drain instead of a state machine
PreLaunchRumAppStartupDetectoris a process-scoped singleton. TheContentProviderinstalls it at process start; it constructs a realRumAppStartupDetectorImpland buffers every callback it observes.RumFeature.initRumAppStartupDetector()now checksPreLaunchRumAppStartupDetector.isInstalled. If installed, it reuses that detector rather than constructing a second one — no claiming, no racing, no duplicateApplication.ActivityLifecycleCallbacks. If absent, the existing path runs unchanged.Rum.enable()then postsattachPreLaunchRumAppStartupDetector()to the main thread afterGlobalRumMonitor.registerIfAbsent(), which hands the feature's listener over and drains the buffer. This replacespendingPreLaunchAction.Activity predicate handling: the pre-launch detector must accept every Activity, since the user's
AppStartupActivityPredicateisn't known before the SDK configures. At init, if the Activity it captured is one the configured predicate excludes, the capture is discarded andRumFeaturefalls back to its own detector.3.
onLateActivityReadyremoved — this is a public API changeReplayableViewTrackingStrategyis deleted, andActivityViewTrackingStrategy/NavigationViewTrackingStrategyno longer declare it or itsonLateActivityReadyoverride. API surfaces regenerated accordingly.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 pre-launch module sidesteps this. By the time the SDK starts, the timing data is already captured and waiting to be drained.
Native Android apps are unaffected: without
dd-sdk-android-rum-prelaunchon the classpath,PreLaunchRumAppStartupDetector.isInstalledisfalseand the existingRumAppStartupDetectorpath runs exactly as before.Original description (superseded)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 newcom.datadoghq:dd-sdk-android-rum-prelaunchmodule.This PR adds three things:NewAppLaunchPreInitCollectorindd-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.Newdd-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 viewThe 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 apendingPreLaunchActionthat is dispatched on the main thread afterGlobalRumMonitor.registerIfAbsent()runs, ensuring the view is already open when the events arrive.2. Memory leak inRumFirstDrawTimeReporterImplWhen subscribing to first-frame events for an activity that never callssetContentView()(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 anApplication.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.MotivationReact 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 existingRumAppStartupDetectorpath runs exactly as before.Additional Notesdd-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.API surfaces updated for both modules. No public API change.Tested on the example React Native SDK example app here: [RUM 16664] Report TTID and TTFD on RN apps dd-sdk-reactnative#1336Review checklist (to be filled by reviewers)