[RUM-14471] Add predicate to filter Activities for TTID reporting#3173
[RUM-14471] Add predicate to filter Activities for TTID reporting#3173
Conversation
This comment has been minimized.
This comment has been minimized.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3173 +/- ##
===========================================
- Coverage 71.30% 71.26% -0.04%
===========================================
Files 928 929 +1
Lines 34444 34457 +13
Branches 5812 5813 +1
===========================================
- Hits 24558 24553 -5
- Misses 8252 8263 +11
- Partials 1634 1641 +7
🚀 New features to boost your workflow:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0bf8d44986
ℹ️ 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".
...id-rum/src/main/kotlin/com/datadog/android/rum/internal/startup/RumAppStartupDetectorImpl.kt
Outdated
Show resolved
Hide resolved
...c/main/kotlin/com/datadog/android/rum/internal/startup/DefaultAppStartupActivityPredicate.kt
Outdated
Show resolved
Hide resolved
...id-rum/src/main/kotlin/com/datadog/android/rum/internal/startup/RumAppStartupDetectorImpl.kt
Outdated
Show resolved
Hide resolved
...id-rum/src/main/kotlin/com/datadog/android/rum/internal/startup/RumAppStartupDetectorImpl.kt
Outdated
Show resolved
Hide resolved
...id-rum/src/main/kotlin/com/datadog/android/rum/internal/startup/RumAppStartupDetectorImpl.kt
Outdated
Show resolved
Hide resolved
0xnm
left a comment
There was a problem hiding this comment.
It would be nice maybe to add some instrumented test here to make sure that:
- logic added indeed does what it is intended to do in the real application
- there is no regression
I understand that it will be hard to assert the output of such test, because I guess we want to validate TTID which may go left-right from run to run. But having something like this would be nice.
Not a blocker though, just an additional safety measure.
Nice suggestion @0xnm. Thanks! I've added a couple of integration tests. |
0xnm
left a comment
There was a problem hiding this comment.
I briefly went through PRs and left some comments. For the logic details I let @aleksandr-gringauz do the judgement :)
| override fun reset() | ||
| override fun close() | ||
| interface com.datadog.android.rum.startup.AppStartupActivityPredicate | ||
| fun shouldTrackStartup(android.app.Activity): Boolean |
There was a problem hiding this comment.
very nit: all other predicate interfaces in RUM module have similar method called accept. Doesn't matter much though since it is a functional interface, so it most probably will be used as lambda.
...st/kotlin/com/datadog/android/sdk/integration/rum/startup/AppStartupActivityPredicateTest.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
I left some comments. Overall - LGTM.
Before merging I suggest we spend some time investigating a "just-works" approach. We might discover something that could lead to changes in the proposed API of the predicate. Unlikely, but I think there is no need to rush.
Also I'd like to avoid telling customers to use the predicate to fix the case of "Activity launches another Activity in onCreate" if we manage to come up with a better solution.
| * Use this to exclude "interstitial Activities" - Activities that are launched during | ||
| * app startup but immediately launch another Activity in their `onCreate()` method | ||
| * and call `finish()` on themselves (e.g., splash screens, authentication screens). | ||
| * | ||
| * These interstitial Activities may never draw a frame, which can prevent TTID from | ||
| * being reported when the actual main Activity is displayed. |
There was a problem hiding this comment.
and call
finish()on themselves (e.g., splash screens, authentication screens).
If you implement a splash screen as activity, it probably draws a frame and doesn't call finish in Activity.onCreate.
I suggest we change this phrase a bit like this:
During application launch the SDK tracks the first Activity that is created and uses the time it draws its first frame to compute the TTID value. Use this if you want to skip some Activities and instead make the SDK consider the 2nd, 3rd Activity's first frame. Example use cases are:
- Splash screens implemented as
Activity. - Activities that launch another Activity in their
onCreate()method and callfinish()on themselves.
There was a problem hiding this comment.
Also if we come up with a "just-works" solution we don't want to tell customers to use AppStartupActivityPredicate for the case of "Activities that launch another Activity in their onCreate() method and call finish() on themselves.".
There was a problem hiding this comment.
I think all suggestions have been implemented. Let's pause this PR until I finish with the "just-works" approach.
...id-rum/src/main/kotlin/com/datadog/android/rum/internal/startup/RumAppStartupDetectorImpl.kt
Outdated
Show resolved
Hide resolved
features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/RumConfiguration.kt
Show resolved
Hide resolved
cbbc8d5 to
8794580
Compare
...integration/src/androidTest/kotlin/com/datadog/android/sdk/integration/rum/ExpectedEvents.kt
Show resolved
Hide resolved
| @After | ||
| fun tearDown() { | ||
| // Ensure SDK is stopped and wait for cleanup to complete | ||
| Datadog.stopInstance() | ||
| InstrumentationRegistry.getInstrumentation().waitForIdleSync() | ||
| } |
There was a problem hiding this comment.
FYI this change affects some other tests as well. Not that I'm against it though, seems fine. Just a heads-up.
...id-rum/src/main/kotlin/com/datadog/android/rum/internal/startup/RumAppStartupDetectorImpl.kt
Outdated
Show resolved
Hide resolved
a9af87b to
5538b89
Compare
Introduce AppStartupActivityPredicate to allow customers to exclude interstitial Activities (e.g., auth screens) that launch another Activity in onCreate() and finish themselves. This fixes TTID not being reported when such Activities never draw a frame.
Use WeakHashMap to cache shouldTrackStartup() result at activity creation and reuse it at destruction, ensuring symmetric counter increment/decrement. This prevents counter corruption when predicates depend on mutable state (e.g., activity flags, runtime toggles) that may change between lifecycle callbacks.
…interstitial activities from TTID measurement
Problem: When running multiple startup tests together, Datadog.stopInstance() in afterActivityFinished() triggered async cleanup operations that didn't complete before the next test initialized. This caused RumSessionScopeStartupManager state (ttidSentForSession = true) to leak between tests, preventing TTID from being reported in subsequent tests. Solution: Added @after tearDown() method in RumTest base class that: 1. Calls Datadog.stopInstance() to trigger cleanup 2. Calls InstrumentationRegistry.getInstrumentation().waitForIdleSync() to ensure main thread becomes idle before proceeding The waitForIdleSync() approach is superior to sleep() because it waits for actual completion rather than an arbitrary timeout. This ensures: - All pending UI operations complete - Main thread work finishes - Async cleanup operations have time to complete - Proper test isolation The double-stop (once in @after, once in afterActivityFinished) is safe due to Datadog.stopInstance() being idempotent. Verified: Both AppStartupActivityPredicateTest and AppStartupMainActivityDirectTest pass reliably when run together.
Added viewArguments parameter to ExpectedVitalAppLaunchEvent to allow verifying view attributes in vital app launch events. Updated startup tests to verify that TTID and TTFD events originate from MainContentActivity by checking the view name attribute using type-safe class references.
…king behavior and use cases
…rImpl and add safe methods to "detetk_custom_safe_call.yml"
5538b89 to
faf2b5e
Compare
What does this PR do?
Adds AppStartupActivityPredicate to RumConfiguration allowing customers to filter out interstitial Activities (e.g., auth screens) from TTID (Time To Initial Display) reporting.
Example of use:
Motivation
TTID was not being reported when interstitial Activities that launch another Activity in
onCreate()and finish themselves never draw a frame. This allows customers to exclude such Activities from startup detection and get the right TTID.Additional Notes
AppStartupActivityPredicateinterfaceDefaultAppStartupActivityPredicatethat includes all Activities (backward compatible)Review checklist (to be filled by reviewers)