[RUM-8061] RUM View Ended Telemetry now includes TNS and INV by satween · Pull Request #2495 · DataDog/dd-sdk-android

satween · 2025-01-28T18:17:22Z

What does this PR do?

Adds mandatory telemetry events for TTNS and ITNV

Spec

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)

codecov-commenter · 2025-01-28T18:43:14Z

Codecov Report

Attention: Patch coverage is 96.31579% with 7 lines in your changes missing coverage. Please review.

Project coverage is 70.11%. Comparing base (6c54695) to head (2f6ed12).
Report is 10 commits behind head on develop.

Files with missing lines	Patch %	Lines
...d/rum/internal/metric/ViewEndedMetricDispatcher.kt	94.83%	1 Missing and 2 partials ⚠️
...ntonextview/InteractionToNextViewMetricResolver.kt	95.12%	1 Missing and 1 partial ⚠️
...ric/networksettled/NetworkSettledMetricResolver.kt	92.86%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2495      +/-   ##
===========================================
+ Coverage    69.95%   70.11%   +0.16%     
===========================================
  Files          788      791       +3     
  Lines        29647    29795     +148     
  Branches      4962     4987      +25     
===========================================
+ Hits         20739    20889     +150     
+ Misses        7536     7526      -10     
- Partials      1372     1380       +8

Files with missing lines	Coverage Δ
.../datadog/android/rum/internal/domain/RumContext.kt	`100.00% <100.00%> (ø)`
...d/rum/internal/domain/scope/RumViewManagerScope.kt	`91.75% <100.00%> (+1.09%)`	⬆️
.../android/rum/internal/domain/scope/RumViewScope.kt	`94.61% <100.00%> (-0.09%)`	⬇️
...g/android/rum/internal/domain/scope/RumViewType.kt	`100.00% <100.00%> (ø)`
.../internal/metric/ViewInitializationMetricsState.kt	`100.00% <100.00%> (ø)`
...android/rum/internal/vitals/VitalReaderRunnable.kt	`100.00% <100.00%> (ø)`
...actiontonextview/TimeBasedInteractionIdentifier.kt	`91.67% <100.00%> (+0.76%)`	⬆️
...tworksettled/TimeBasedInitialResourceIdentifier.kt	`91.67% <100.00%> (+0.76%)`	⬆️
...ntonextview/InteractionToNextViewMetricResolver.kt	`94.52% <95.12%> (-1.63%)`	⬇️
...ric/networksettled/NetworkSettledMetricResolver.kt	`94.20% <92.86%> (+1.52%)`	⬆️
... and 1 more

... and 23 files with indirect coverage changes

0xnm · 2025-01-29T10:35:07Z

            networkSettledMetricResolver.viewWasStopped()
+
+            viewEndedMetricDispatcher.sendViewEnded(


Is it intentional that we are using a different lingvo: ended vs stopped? Shouldn't we align?

View may be stopped, but still be in the list of the scopes (but not active ones), waiting for the child scopes to finish.

Once all childs scopes finish, the view is considered as complete (see

dd-sdk-android/features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/scope/RumViewScope.kt

Lines 1314 to 1322 in 6c54695

private fun isViewComplete(): Boolean {

val pending = pendingActionCount +

pendingResourceCount +

pendingErrorCount +

pendingLongTaskCount

// we use <= 0 for pending counter as a safety measure to make sure this ViewScope will

// be closed.

return stopped && activeResourceScopes.isEmpty() && (pending <= 0L)

}

).

Is ended here an equivalent of stopped or complete?

Nice catch here - changes followed from this note lead me to found potential issue.
The ended here comes from metric name rather than android sdk notation so I prefer to keep it as it is ( to be aligned with SessionEndedMetric), but entire method call now checks for child scopes

0xnm · 2025-01-29T10:36:11Z

+            )
+
+            val viewEndedMetricDispatcher = ViewEndedMetricDispatcher(
+                viewType = ViewMetricDispatcher.ViewType.CUSTOM,


shouldn't we inherit it as an argument of fromEvent call and it is up to the caller to provide a relevant value?

I found that there is RumViewType enum that does actually the same so removed duplication and reusing RumViewType instead. There is no reason to get the value from method parameters because RumViewType is not provided at this moment, but thanks for spotting - it helped to reduce code duplication

ncreated

I only validated the spec values, finding a problem around loading_time key - requesting a change. Other that this, it looks very good, well done 🙂 💪.

ncreated

Looks good to me 👍

0xnm · 2025-02-03T10:28:59Z

+        // Second, if there is no previous view information, we are either on the very first screen of the app or
+        // lost the previous view information for some reason
+        val previousViewId = resolvePreviousViewId(viewId)
+            ?: return NoValueReason.InteractionToNextView.NO_PREVIOUS_VIEW
+
+        // Third - the case where there is no interaction from the previous view.
+        if (lastInteractions[previousViewId] == null) {
+            return NoValueReason.InteractionToNextView.NO_ACTION
        }
-        return null
+
+        // Finally - checking that the previous view interaction is eligible
+        resolveLastInteraction(previousViewId, currentViewCreatedTimestamp)
+            ?: return NoValueReason.InteractionToNextView.NO_ELIGIBLE_ACTION


There is a corner case when view ID may change if session ID has changed

dd-sdk-android/features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/scope/RumViewScope.kt

Lines 226 to 229 in 7387cc9

if (parentContext.sessionId != sessionId) {

sessionId = parentContext.sessionId

viewId = UUID.randomUUID().toString()

}

, but it is hard to address it anyway.

satween requested review from a team as code owners January 28, 2025 18:17

xgouchet reviewed Jan 29, 2025

View reviewed changes

Comment thread ...um/src/main/kotlin/com/datadog/android/rum/internal/metric/ViewInitializationMetricsState.kt

xgouchet previously approved these changes Jan 29, 2025

View reviewed changes

Comment thread ...-android-rum/src/main/kotlin/com/datadog/android/rum/internal/metric/ViewMetricDispatcher.kt Outdated

0xnm reviewed Jan 29, 2025

View reviewed changes

jonathanmos previously approved these changes Jan 30, 2025

View reviewed changes

Comment thread ...otlin/com/datadog/android/rum/internal/metric/networksettled/NetworkSettledMetricResolver.kt Outdated

ncreated requested changes Jan 30, 2025

View reviewed changes

Comment thread ...oid-rum/src/main/kotlin/com/datadog/android/rum/internal/metric/ViewEndedMetricDispatcher.kt Outdated

satween added 6 commits January 30, 2025 17:10

RUM-8061: View ended telemetry support

ae22aed

RUM-8061: ViewEndedMetricDispatcher callback integrations tests

5957bef

RUM-8061: tests support

af1d0fe

RUM-8061: fix tabulation, adding docs

e6fb8fb

RUM-8061: fix api surface

bd1f4ca

RUM-8061: fix review notes

20c91fe

satween dismissed stale reviews from jonathanmos and xgouchet via fae6587 January 30, 2025 17:11

satween force-pushed the tvaleev/rum-8061 branch from f8fbc88 to fae6587 Compare January 30, 2025 17:11

satween requested review from 0xnm and ncreated January 30, 2025 17:32

RUM-8061: fix tests

2f6ed12

satween force-pushed the tvaleev/rum-8061 branch from 733fdbd to 2f6ed12 Compare January 31, 2025 15:30

ncreated approved these changes Jan 31, 2025

View reviewed changes

xgouchet approved these changes Feb 3, 2025

View reviewed changes

satween merged commit 056dc01 into develop Feb 3, 2025

satween deleted the tvaleev/rum-8061 branch February 3, 2025 10:25

0xnm reviewed Feb 3, 2025

View reviewed changes

plousada changed the title ~~RUM View Ended" Telemetry~~ [RUM-8098] RUM View Ended" Telemetry Feb 3, 2025

plousada changed the title ~~[RUM-8098] RUM View Ended" Telemetry~~ RUM View Ended" Telemetry Feb 3, 2025

plousada changed the title ~~RUM View Ended" Telemetry~~ [RUM-8061] RUM View Ended Telemetry now includes TNS and INV Feb 3, 2025

satween mentioned this pull request Feb 6, 2025

[RUM-8449] Fixing sampling rate #2503

Merged

3 tasks

satween mentioned this pull request Feb 10, 2025

[RUM-8375] View ended instrumentation type attribute support #2504

Merged

3 tasks

		networkSettledMetricResolver.viewWasStopped()

		viewEndedMetricDispatcher.sendViewEnded(

	private fun isViewComplete(): Boolean {
	val pending = pendingActionCount +
	pendingResourceCount +
	pendingErrorCount +
	pendingLongTaskCount
	// we use <= 0 for pending counter as a safety measure to make sure this ViewScope will
	// be closed.
	return stopped && activeResourceScopes.isEmpty() && (pending <= 0L)
	}

	if (parentContext.sessionId != sessionId) {
	sessionId = parentContext.sessionId
	viewId = UUID.randomUUID().toString()
	}

Uh oh!

Conversation

satween commented Jan 28, 2025

What does this PR do?

Review checklist (to be filled by reviewers)

Uh oh!

codecov-commenter commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

0xnm Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

satween Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

0xnm Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

satween Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ncreated left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ncreated left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

0xnm Feb 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-commenter commented Jan 28, 2025 •

edited

Loading