RUMM-3500 View tracking gap #1578

xgouchet · 2023-08-16T14:16:36Z

What does this PR do?

Delay the view stopping event to prevent gaps between views

Motivation

A non zero gap (~100ms) exists when navigating between fragments/activities. This is because a Fragment/Activity's onResume is called some time after the previous one's onPause. This would leave a short period of time during which errors and resources would not be tracked because no Active view was available.

Additional Notes

Thoughts about additional implementation of this feature :

mark a view as stopped as it is, and allow adding new children events (errors, resources), until a start view is detected or only after a specific period of time.

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
Make sure you discussed the feature or bugfix with the maintaining team in an Issue
Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

plousada · 2023-08-17T13:59:05Z

...oid-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/scope/RumViewManagerScope.kt

@@ -59,6 +61,15 @@ internal class RumViewManagerScope(

        if (event is RumRawEvent.StartView && !stopped) {
            startForegroundView(event, writer)
+            lastStoppedViewTime?.let {


Won't this be triggered every time there is a gap between a stopView and startView on a manual instrumentation?
That would hinder our ability to properly assess if this gap is closed in automatic tracking strategies.

Seems like this telemetry should be moved closer to the automatic strategies so it's not influenced by manual calls to the API.

This would indeed, but the tracking strategy and view scope are completely independent from one another. If we were to move this detection in the view tracking strategy itself, we'd have to duplicate the logic three times.
We do have the information of the view tracking strategy in the configuration telemetry event, meaning we can exclude orgs using manual tracking if need be to know where the gap is coming from.

Makes sense.
It would be better if we could do it before sending the telemetry in the first place but detecting if we have an automated view tracking strategy in place here would create unwanted dependencies.

codecov-commenter · 2023-08-21T08:33:47Z

Codecov Report

Merging #1578 (48a80c3) into develop (1298b9e) will increase coverage by 0.08%.
Report is 3 commits behind head on develop.
The diff coverage is 95.65%.

@@             Coverage Diff             @@
##           develop    #1578      +/-   ##
===========================================
+ Coverage    83.42%   83.50%   +0.08%     
===========================================
  Files          437      437              
  Lines        14910    14945      +35     
  Branches      2230     2233       +3     
===========================================
+ Hits         12438    12479      +41     
+ Misses        1885     1877       -8     
- Partials       587      589       +2

Files Changed	Coverage Δ
...droid/rum/tracking/ActivityViewTrackingStrategy.kt	`90.32% <90.91%> (+3.37%)`	⬆️
...d/rum/internal/domain/scope/RumViewManagerScope.kt	`96.53% <92.31%> (-0.44%)`	⬇️
...nal/tracking/AndroidXFragmentLifecycleCallbacks.kt	`94.44% <100.00%> (+1.59%)`	⬆️
...nternal/tracking/OreoFragmentLifecycleCallbacks.kt	`89.74% <100.00%> (-0.88%)`	⬇️

... and 15 files with indirect coverage changes

...oid-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/scope/RumViewManagerScope.kt

.../main/kotlin/com/datadog/android/rum/internal/tracking/AndroidXFragmentLifecycleCallbacks.kt

xgouchet · 2023-08-21T12:18:49Z

...ation/src/androidTest/kotlin/com/datadog/android/sdk/integration/rum/FragmentTrackingTest.kt

-                currentFragmentExtras(activity)
-            )
-        )
+//        expectedEvents.add(


This expected events is removed. Apparently the JUnit Runner's callActivityOnStop doesn't properly forward the fragment's onStop() to the fragment lifecycle listeners.

mariusc83

LGTM

jonathanmos · 2023-08-22T07:32:00Z

.../main/kotlin/com/datadog/android/rum/internal/tracking/AndroidXFragmentLifecycleCallbacks.kt

@@ -94,4 +112,8 @@ internal open class AndroidXFragmentLifecycleCallbacks(
    }

    // endregion
+
+    companion object {
+        private const val STOP_VIEW_DELAY_MS = 200L


I see that this constant is defined here but also in other places. Should we unify to a SSOT incase we want to modify the delay later on?

jonathanmos · 2023-08-22T07:32:14Z

.../src/main/kotlin/com/datadog/android/rum/internal/tracking/OreoFragmentLifecycleCallbacks.kt

@@ -104,6 +117,7 @@ internal class OreoFragmentLifecycleCallbacks(

    private companion object {
        private const val REPORT_FRAGMENT_NAME = "androidx.lifecycle.ReportFragment"
+        private const val STOP_VIEW_DELAY_MS = 200L


see previous

jonathanmos · 2023-08-22T07:33:27Z

...android-rum/src/main/kotlin/com/datadog/android/rum/tracking/ActivityViewTrackingStrategy.kt

@@ -84,4 +99,8 @@ class ActivityViewTrackingStrategy @JvmOverloads constructor(
    }

    // endregion
+
+    internal companion object {
+        private const val STOP_VIEW_DELAY_MS = 200L


see previous

xgouchet requested a review from a team as a code owner August 16, 2023 14:16

xgouchet force-pushed the xgouchet/RUMM-3500/view_tracking_gap branch from ef3a127 to 27a7901 Compare August 17, 2023 12:43

plousada reviewed Aug 17, 2023

View reviewed changes

xgouchet force-pushed the xgouchet/RUMM-3500/view_tracking_gap branch from 27a7901 to 07777fb Compare August 17, 2023 15:15

mariusc83 reviewed Aug 21, 2023

View reviewed changes

xgouchet commented Aug 21, 2023

View reviewed changes

xgouchet force-pushed the xgouchet/RUMM-3500/view_tracking_gap branch from 426e347 to 48a80c3 Compare August 21, 2023 12:33

mariusc83 approved these changes Aug 21, 2023

View reviewed changes

xgouchet added 3 commits August 21, 2023 15:28

RUMM-3500 add telemetry to record gaps between views

836b5e9

RUMM-3500 Delay the stop view to prevent gap between fragments/activity

dbe85c7

RUMM-3500 Update integration tests

e6890ed

xgouchet force-pushed the xgouchet/RUMM-3500/view_tracking_gap branch from 48a80c3 to e6890ed Compare August 21, 2023 13:28

xgouchet requested a review from plousada August 21, 2023 13:28

plousada approved these changes Aug 21, 2023

View reviewed changes

xgouchet merged commit 46a9423 into develop Aug 22, 2023

xgouchet deleted the xgouchet/RUMM-3500/view_tracking_gap branch August 22, 2023 07:33

jonathanmos approved these changes Aug 22, 2023

View reviewed changes

xgouchet mentioned this pull request Aug 25, 2023

Events between Activities considered as Background #1565

Closed

xgouchet added this to the 2.1.0 milestone Dec 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUMM-3500 View tracking gap #1578

RUMM-3500 View tracking gap #1578

xgouchet commented Aug 16, 2023

plousada Aug 17, 2023 •

edited

Loading

xgouchet Aug 21, 2023

plousada Aug 21, 2023

codecov-commenter commented Aug 21, 2023 •

edited

Loading

xgouchet Aug 21, 2023

mariusc83 left a comment

jonathanmos Aug 22, 2023

jonathanmos Aug 22, 2023

jonathanmos Aug 22, 2023

RUMM-3500 View tracking gap #1578

RUMM-3500 View tracking gap #1578

Conversation

xgouchet commented Aug 16, 2023

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

plousada Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

xgouchet Aug 21, 2023

Choose a reason for hiding this comment

plousada Aug 21, 2023

Choose a reason for hiding this comment

codecov-commenter commented Aug 21, 2023 • edited Loading

Codecov Report

xgouchet Aug 21, 2023

Choose a reason for hiding this comment

mariusc83 left a comment

Choose a reason for hiding this comment

jonathanmos Aug 22, 2023

Choose a reason for hiding this comment

jonathanmos Aug 22, 2023

Choose a reason for hiding this comment

jonathanmos Aug 22, 2023

Choose a reason for hiding this comment

plousada Aug 17, 2023 •

edited

Loading

codecov-commenter commented Aug 21, 2023 •

edited

Loading