RUM-6129 Introduce the API usage telemetry event and API by mariusc83 · Pull Request #2258 · DataDog/dd-sdk-android

mariusc83 · 2024-09-11T12:00:07Z

What does this PR do?

In this PR we are introducing the new InternalLogger#logApisUsage method which will be used to report usage of different APIs as Telemetry from our sdk. In order to achieve this we had to refactor and simplify the whole Telemetry related logic in the SDK to make it more scalable. For this we introduced the new InternalTelemetryEvent which will be shared through out all the module from the dd-sdk-android-internal module.

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)

codecov-commenter · 2024-09-13T09:32:35Z

Codecov Report

Attention: Patch coverage is 58.08824% with 57 lines in your changes missing coverage. Please review.

Project coverage is 69.90%. Comparing base (febadea) to head (531b346).
Report is 5 commits behind head on develop.

Files with missing lines	Patch %	Lines
...ndroid/telemetry/internal/TelemetryEventHandler.kt	40.32%	34 Missing and 3 partials ⚠️
...og/android/telemetry/internal/TelemetryEventExt.kt	0.00%	9 Missing ⚠️
.../main/kotlin/com/datadog/android/_InternalProxy.kt	76.92%	0 Missing and 3 partials ⚠️
...dog/android/telemetry/internal/TelemetryEventId.kt	72.73%	1 Missing and 2 partials ⚠️
...n/com/datadog/android/core/internal/DatadogCore.kt	75.00%	0 Missing and 2 partials ⚠️
.../android/core/internal/logger/SdkInternalLogger.kt	92.86%	0 Missing and 1 partial ⚠️
...lin/com/datadog/android/rum/internal/RumFeature.kt	90.00%	0 Missing and 1 partial ⚠️
.../android/rum/internal/monitor/DatadogRumMonitor.kt	66.67%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2258      +/-   ##
===========================================
- Coverage    70.06%   69.90%   -0.16%     
===========================================
  Files          730      727       -3     
  Lines        27181    27112      -69     
  Branches      4593     4572      -21     
===========================================
- Hits         19043    18950      -93     
- Misses        6845     6878      +33     
+ Partials      1293     1284       -9

Files with missing lines	Coverage Δ
...n/kotlin/com/datadog/android/api/InternalLogger.kt	`100.00% <ø> (ø)`
...ndroid/rum/internal/domain/event/RumEventMapper.kt	`100.00% <100.00%> (ø)`
...g/android/rum/internal/domain/scope/RumRawEvent.kt	`100.00% <100.00%> (ø)`
...atadog/android/telemetry/internal/TelemetryType.kt	`100.00% <100.00%> (ø)`
.../android/core/internal/logger/SdkInternalLogger.kt	`88.17% <92.86%> (-4.76%)`	⬇️
...lin/com/datadog/android/rum/internal/RumFeature.kt	`93.52% <90.00%> (-0.08%)`	⬇️
.../android/rum/internal/monitor/DatadogRumMonitor.kt	`85.28% <66.67%> (-1.75%)`	⬇️
...n/com/datadog/android/core/internal/DatadogCore.kt	`82.19% <75.00%> (+0.29%)`	⬆️
.../main/kotlin/com/datadog/android/_InternalProxy.kt	`65.22% <76.92%> (+1.58%)`	⬆️
...dog/android/telemetry/internal/TelemetryEventId.kt	`80.00% <72.73%> (-20.00%)`	⬇️
... and 2 more

... and 24 files with indirect coverage changes

xgouchet · 2024-09-13T12:53:29Z

+                message = message,
+                additionalProperties = null
+            )
+            rumFeature?.sendEvent(bundleEventIntoTelemetry(telemetryEvent))


Note
Since the event itself can be of type Any, you could avoid the Map<…> altogether and send the InternalTelemetryEvent directly. Same for all the methods in this class.

yes but then I will need an extra case in the onReceive of the RumFeature only for this so I would rather bundle it here.

xgouchet · 2024-09-13T12:56:56Z

+                batchUploadFrequency = configuration.coreConfig.uploadFrequency.baseStepMs,
+                batchProcessingLevel = configuration.coreConfig.batchProcessingLevel.maxBatchesPerUploadJob
+            )
+            rumFeature.sendEvent(


Warning
This is another argument to send the event InternalTelemetryEvent directly. Here you're not reusing the bundleEventIntoTelemetry() function you made, meaning that any further change around the telemetry would need to happen in two places, which is error prone.

hmmm actually I see your point also regarding the first comment. I don't know why I kept using the Map...we could just send the InternalTelemetryEvent everywhere

0xnm · 2024-09-16T08:45:31Z

+        apiUsageEvent: InternalTelemetryEvent.ApiUsage,
+        samplingRate: Float
+    ) {
+        println("AU [T]: ${apiUsageEvent::class.simpleName} | $samplingRate%")


Is it useful to log apiUsageEvent::class.simpleName if AU already stands for ApiUsage?

0xnm · 2024-09-16T08:47:07Z

+     */
+    @InternalApi
+    fun logApiUsage(
+        apiUsageEvent: InternalTelemetryEvent.ApiUsage,


won't it cause any issues if we are adding the class from implementation dependency to the class which is visible on the customer side via api dependency type? any compilation/IDE issues?

I didn't see any but will try to test it from the sample app

0xnm · 2024-09-16T08:56:20Z

+            overwrite = forge.aBool(),
+            noView = forge.aBool(),
+            noActiveView = forge.aBool(),
+            additionalProperties = forge.aMap { forge.aString() to forge.aString() }.toMutableMap()


minor: calls like forge.aMap, etc. have a Forge as lambda receiver, so it can be written as forge.aMap { aString() to aString() }

0xnm · 2024-09-16T08:58:47Z

+	public static final field Companion Lcom/datadog/android/telemetry/model/CommonTelemetryProperties$Companion;
+	public fun <init> (Lcom/datadog/android/telemetry/model/CommonTelemetryProperties$Dd;JLjava/lang/String;Lcom/datadog/android/telemetry/model/CommonTelemetryProperties$Source;Ljava/lang/String;Lcom/datadog/android/telemetry/model/CommonTelemetryProperties$Application;Lcom/datadog/android/telemetry/model/CommonTelemetryProperties$Session;Lcom/datadog/android/telemetry/model/CommonTelemetryProperties$View;Lcom/datadog/android/telemetry/model/CommonTelemetryProperties$Action;Ljava/util/List;Lcom/datadog/android/telemetry/model/CommonTelemetryProperties$Telemetry;)V
+	public synthetic fun <init> (Lcom/datadog/android/telemetry/model/CommonTelemetryProperties$Dd;JLjava/lang/String;Lcom/datadog/android/telemetry/model/CommonTelemetryProperties$Source;Ljava/lang/String;Lcom/datadog/android/telemetry/model/CommonTelemetryProperties$Application;Lcom/datadog/android/telemetry/model/CommonTelemetryProperties$Session;Lcom/datadog/android/telemetry/model/CommonTelemetryProperties$View;Lcom/datadog/android/telemetry/model/CommonTelemetryProperties$Action;Ljava/util/List;Lcom/datadog/android/telemetry/model/CommonTelemetryProperties$Telemetry;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
+	public final fun component1 ()Lcom/datadog/android/telemetry/model/CommonTelemetryProperties$Dd;


What bothers me is that by adding new data classes in our schema which have many params, we add so many useless destructuring methods which just grow binary footprint for nothing. Maybe there is a way to prevent them from being generated using some 3rd party plugins.

I am going to have a look for something but not going to spend too much on this

yes, it is a side thought, for sure shouldn't be a part of this PR.

0xnm · 2024-09-16T08:59:57Z

    targetPackageName = "com.datadog.android.telemetry.model"
-    ignoredFiles = arrayOf(
-        "_common-schema.json",
-        "usage-schema.json"


blocking: you need to remove only usage-schema.json from here. _common-schema is already referenced and being used by individual event schema files.

oh ok...thought it will need also the _common-schema as usage-schema is referencing it

0xnm · 2024-09-16T09:05:17Z


    // region Internal

+    private fun handleTelemetryEvent(event: Map<*, *>) {


probably with that change we can send class directly instead of using Map? It will require a change of the check right at the beginning of onReceive, but it is a minor thing.

yup exactly what @xgouchet was pointing out also, I am going to apply the changes now.

0xnm · 2024-09-16T09:06:19Z

-        val onlyOnce: Boolean = false,
-        override val eventTime: Time = Time(),
-        val isMetric: Boolean = false
+    internal data class TelemetryEventWrapper(


we probably can keep the same name for the class - SendTelemetry. but not a big deal

0xnm · 2024-09-16T09:08:12Z

                }
-            }

+                else -> null


shouldn't we log here?

actually that's a sealed class so we don't really need the else here anyway. In case we do not treat a case we will be warned by the editor.

0xnm · 2024-09-16T09:09:43Z

+                    )
+                )
+            }
+            else -> null


we don't need else here I think, because we are using sealed class. So if we remove else here and add one more sub-type to InternalTelemetryEvent.ApiUsage, without adding additional branch in when, we will get a compilation error, which is for good.

yup it is exactly the case in line 117 also so you are right

0xnm

I added few comments/suggestions, but nothing blocking.

0xnm · 2024-09-16T10:00:15Z

+            !is Map<*, *> -> {
                sdkCore.internalLogger.log(
                    InternalLogger.Level.WARN,
                    InternalLogger.Target.USER,
-                    { UNKNOWN_EVENT_TYPE_PROPERTY_VALUE.format(Locale.US, event["type"]) }
+                    { UNSUPPORTED_EVENT_TYPE.format(Locale.US, event::class.java.canonicalName) }
                )
+                return
+            }
+
+            else -> when (event["type"]) {


I think it is better to swap these branches, and just do:

is Map<*, *> -> else ->

reason: !is is harder to read and then in else it it takes some time to understand that type of the object there is Map.

0xnm · 2024-09-16T10:00:48Z

                " where mandatory message field is either missing or has a wrong type."
-        internal const val TELEMETRY_MISSING_MESSAGE_FIELD = "RUM feature received a telemetry" +
-            " event, but mandatory message field is either missing or has a wrong type."
+        internal const val TELEMETRY_MISSING_EVENT_FIELD_WARNING_MESSAGE = "RUM feature received a telemetry" +


is it still needed?

0xnm · 2024-09-16T10:02:29Z

-            val telemetryEvent: Any? = when (event.type) {
-                TelemetryType.DEBUG -> {
+            val timestamp = wrappedEvent.eventTime.timestamp + datadogContext.time.serverTimeOffsetMs
+            val telemetryEvent: Any? = when (event) {


does it become exhaustive when? The reason why I'm asking it because there are 2 sealed classes involved here, not only one. Will it be a compilation error if we add say InternalTelemetry.Log.XXX into classes definition?

I think the way it behaves is that whenever you add a new type of the sealed class it will warn you that is not handled but will double check it.

yes, but you now have 2 sealed base classes in this hierarchy, not one, that is why I'm curious.

Yeah, I confirm whenever you add a new type to that sealed class it is only to crash at compile time saying that you need to be exhaustive.

mariusc83 self-assigned this Sep 11, 2024

mariusc83 force-pushed the mconstantin/rum-6129/introduce-the-usagetelemetry-event branch 7 times, most recently from 9b196a9 to cefb3c2 Compare September 13, 2024 08:49

mariusc83 marked this pull request as ready for review September 13, 2024 08:51

mariusc83 requested review from a team as code owners September 13, 2024 08:51

mariusc83 force-pushed the mconstantin/rum-6129/introduce-the-usagetelemetry-event branch from cefb3c2 to 94d452d Compare September 13, 2024 09:13

mariusc83 force-pushed the mconstantin/rum-6129/introduce-the-usagetelemetry-event branch from 94d452d to e2bd3f2 Compare September 13, 2024 09:37

xgouchet suggested changes Sep 13, 2024

View reviewed changes

0xnm reviewed Sep 16, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-6129/introduce-the-usagetelemetry-event branch from b76ea8a to 9475ffd Compare September 16, 2024 09:48

mariusc83 requested review from 0xnm and xgouchet September 16, 2024 09:48

mariusc83 added 2 commits September 16, 2024 11:54

RUM-6129 Introduce the API usage telemetry event and API

804969f

Rename classes

40283db

0xnm approved these changes Sep 16, 2024

View reviewed changes

Apply code review changes

c17f90a

mariusc83 force-pushed the mconstantin/rum-6129/introduce-the-usagetelemetry-event branch from 9475ffd to db9a4f1 Compare September 16, 2024 12:52

mariusc83 requested a review from 0xnm September 16, 2024 12:53

0xnm approved these changes Sep 16, 2024

View reviewed changes

xgouchet approved these changes Sep 17, 2024

View reviewed changes

Handle TelemetryUsageEvent in RumEventMapper

531b346

mariusc83 force-pushed the mconstantin/rum-6129/introduce-the-usagetelemetry-event branch from db9a4f1 to 531b346 Compare September 17, 2024 07:41

mariusc83 merged commit 531effd into develop Sep 17, 2024

mariusc83 deleted the mconstantin/rum-6129/introduce-the-usagetelemetry-event branch September 17, 2024 09:37


		// region Internal

		private fun handleTelemetryEvent(event: Map<, >) {

Uh oh!

Conversation

mariusc83 commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Uh oh!

codecov-commenter commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xnm Sep 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xnm Sep 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xnm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mariusc83 commented Sep 11, 2024 •

edited

Loading

codecov-commenter commented Sep 13, 2024 •

edited

Loading

0xnm Sep 16, 2024 •

edited

Loading

0xnm Sep 16, 2024 •

edited

Loading