RUM-6033 Add telemetry and logs related with RumMonitor#addViewLoadigTime API #2267

mariusc83 · 2024-09-18T06:22:32Z

What does this PR do?

Following the RFC in this PR we are adding the logs and telemetry related with usage of the RumMonitor#addViewLoadingTime newly introduced API. The telemetry and log configuration will be configured accordingly with the current RUM context state and will help us understand what is the pattern for this API usage on the client side.

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

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)

ganeshnj

I don't see rest of the API usage events such as TrackingConsent?

Are we generating them or not?

ganeshnj · 2024-09-18T08:51:29Z

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

@@ -144,7 +145,24 @@ internal class RumViewManagerScope(
        val importanceForeground = ActivityManager.RunningAppProcessInfo.IMPORTANCE_FOREGROUND
        val isForegroundProcess = processFlag == importanceForeground

-        if (applicationDisplayed || !isForegroundProcess) {
+        if (event is RumRawEvent.AddViewLoadingTime) {


/question

do you need to add checks like this in future if you any other usage telemetry like setTrackingConsent ?

if yes, then we should change it to more generic else these if-else will explode.

not anticipating that but this whole part will need re - factory soon but not planned for now.

a backlog item would be fine here so we don't forget.

...dk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/scope/RumViewScope.kt

...-android-rum/src/main/kotlin/com/datadog/android/telemetry/internal/TelemetryEventHandler.kt

mariusc83 · 2024-09-19T09:00:32Z

I don't see rest of the API usage events such as TrackingConsent?

Are we generating them or not?

This PR is not about the TrackingConsent, that is a different topic and it is the first time I hear about this. Let's talk about this during the daily.

xgouchet · 2024-09-19T09:10:09Z

dd-sdk-android-core/src/main/kotlin/com/datadog/android/api/InternalLogger.kt

     */
    @InternalApi
    fun logApiUsage(
        apiUsageEvent: InternalTelemetryEvent.ApiUsage,
-        samplingRate: Float
+        samplingRate: Float = 15f


Note
Nitpick, this should be a constant and not a hardcoded Magic Number

ganeshnj · 2024-09-19T12:54:08Z

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

@@ -144,7 +145,24 @@ internal class RumViewManagerScope(
        val importanceForeground = ActivityManager.RunningAppProcessInfo.IMPORTANCE_FOREGROUND
        val isForegroundProcess = processFlag == importanceForeground

-        if (applicationDisplayed || !isForegroundProcess) {
+        if (event is RumRawEvent.AddViewLoadingTime) {


a backlog item would be fine here so we don't forget.

…gTime API

ambushwork · 2024-09-19T13:09:23Z

...-rum/src/test/kotlin/com/datadog/android/rum/internal/domain/event/RumEventSerializerTest.kt

@@ -748,6 +750,110 @@ internal class RumEventSerializerTest {
        }
    }

+    @RepeatedTest(8)


we should use @Test to reduce CI load after making repeating test in local.

in this case we need a repeated test as this model has multiple variables variation. Is good to have repetitive tests in order to make sure we cover more cases in this variation.

codecov-commenter · 2024-09-19T13:11:24Z

Codecov Report

Attention: Patch coverage is 96.72131% with 2 lines in your changes missing coverage. Please review.

Project coverage is 70.10%. Comparing base (531effd) to head (23e11c2).
Report is 10 commits behind head on develop.

Files with missing lines	Patch %	Lines
...n/kotlin/com/datadog/android/api/InternalLogger.kt	0.00%	1 Missing ⚠️
.../android/rum/internal/domain/scope/RumViewScope.kt	97.67%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2267      +/-   ##
===========================================
+ Coverage    69.91%   70.10%   +0.19%     
===========================================
  Files          727      727              
  Lines        27112    27134      +22     
  Branches      4572     4570       -2     
===========================================
+ Hits         18954    19021      +67     
+ Misses        6872     6831      -41     
+ Partials      1286     1282       -4

Files with missing lines	Coverage Δ
...id/rum/internal/domain/event/RumEventSerializer.kt	`100.00% <100.00%> (ø)`
...d/rum/internal/domain/scope/RumViewManagerScope.kt	`91.30% <100.00%> (+1.30%)`	⬆️
...ndroid/telemetry/internal/TelemetryEventHandler.kt	`86.49% <100.00%> (+12.07%)`	⬆️
...n/kotlin/com/datadog/android/api/InternalLogger.kt	`93.75% <0.00%> (-6.25%)`	⬇️
.../android/rum/internal/domain/scope/RumViewScope.kt	`94.50% <97.67%> (+0.15%)`	⬆️

... and 32 files with indirect coverage changes

Handle the TelemetryUsageEvent sampling correctly

c820a40

mariusc83 self-assigned this Sep 18, 2024

mariusc83 force-pushed the mconstantin/rum-6033/add-viewloadingtime-logs branch 4 times, most recently from 72483f6 to cd72d44 Compare September 18, 2024 08:30

mariusc83 marked this pull request as ready for review September 18, 2024 08:31

mariusc83 requested review from a team as code owners September 18, 2024 08:31

ganeshnj requested changes Sep 18, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-6033/add-viewloadingtime-logs branch from cd72d44 to e70c3ea Compare September 18, 2024 11:11

xgouchet reviewed Sep 19, 2024

View reviewed changes

ganeshnj approved these changes Sep 19, 2024

View reviewed changes

RUM-6033 Add telemetry and logs related with RumMonitor#addViewLoadin…

23e11c2

…gTime API

mariusc83 force-pushed the mconstantin/rum-6033/add-viewloadingtime-logs branch from e70c3ea to 23e11c2 Compare September 19, 2024 12:58

mariusc83 requested a review from xgouchet September 19, 2024 13:06

ambushwork reviewed Sep 19, 2024

View reviewed changes

mariusc83 merged commit acd33c7 into develop Sep 19, 2024
23 checks passed

mariusc83 deleted the mconstantin/rum-6033/add-viewloadingtime-logs branch September 19, 2024 14:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUM-6033 Add telemetry and logs related with RumMonitor#addViewLoadigTime API #2267

RUM-6033 Add telemetry and logs related with RumMonitor#addViewLoadigTime API #2267

mariusc83 commented Sep 18, 2024 •

edited

Loading

ganeshnj left a comment

ganeshnj Sep 18, 2024

mariusc83 Sep 19, 2024

ganeshnj Sep 19, 2024

mariusc83 commented Sep 19, 2024

xgouchet Sep 19, 2024

ganeshnj Sep 19, 2024

ambushwork Sep 19, 2024

mariusc83 Sep 19, 2024

codecov-commenter commented Sep 19, 2024 •

edited

Loading

RUM-6033 Add telemetry and logs related with RumMonitor#addViewLoadigTime API #2267

RUM-6033 Add telemetry and logs related with RumMonitor#addViewLoadigTime API #2267

Conversation

mariusc83 commented Sep 18, 2024 • edited Loading

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

ganeshnj left a comment

Choose a reason for hiding this comment

ganeshnj Sep 18, 2024

Choose a reason for hiding this comment

mariusc83 Sep 19, 2024

Choose a reason for hiding this comment

ganeshnj Sep 19, 2024

Choose a reason for hiding this comment

mariusc83 commented Sep 19, 2024

xgouchet Sep 19, 2024

Choose a reason for hiding this comment

ganeshnj Sep 19, 2024

Choose a reason for hiding this comment

ambushwork Sep 19, 2024

Choose a reason for hiding this comment

mariusc83 Sep 19, 2024

Choose a reason for hiding this comment

codecov-commenter commented Sep 19, 2024 • edited Loading

Codecov Report

mariusc83 commented Sep 18, 2024 •

edited

Loading

codecov-commenter commented Sep 19, 2024 •

edited

Loading