RUMM-924 Introduce the new InternalLogger#metric API #1581

mariusc83 · 2023-08-22T07:39:12Z

What does this PR do?

A brief description of the change being made with this pull request.

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)

0xnm · 2023-08-22T08:13:35Z

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

@@ -96,6 +96,13 @@ interface InternalLogger {
        additionalProperties: Map<String, Any?>? = null
    )

+    /**
+     * Logs a metric from the internal implementation.


what is the meaning of internal implementation here? Also maybe we need to add the statement that metric will be sent to the telemetry (and say how), because from the method arguments it is not evident where is the metric here.

0xnm · 2023-08-22T08:13:56Z

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

@@ -188,6 +192,21 @@ internal class SdkInternalLogger(
        }
        rumFeature.sendEvent(telemetryEvent)
    }
+    private fun logMetric(


Suggested change

private fun logMetric(

private fun logMetric(

0xnm · 2023-08-22T08:14:55Z

dd-sdk-android-core/src/test/kotlin/com/datadog/android/core/SdkInternalLoggerTest.kt

+        testedInternalLogger.metric(
+            mockLambda,
+            fakeAdditionalProperties
+        )


What is the point of the test without verification part?

I cannot verify anything there, I wanted to make sure is not throwing anything...maybe there is a method to assert that isNotThrowing

you can wrap the invocation in assertNotThrows.

yup, that's what I also have in mind ;)

0xnm · 2023-08-22T08:18:00Z

features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/telemetry/internal/Telemetry.kt

@@ -39,4 +39,8 @@ internal class Telemetry(
        (GlobalRumMonitor.get(sdkCore) as? AdvancedRumMonitor)
            ?.sendDebugTelemetryEvent(message, additionalProperties)
    }
+    fun metric(message: String, additionalProperties: Map<String, Any?>? = null) {


Suggested change

fun metric(message: String, additionalProperties: Map<String, Any?>? = null) {

fun metric(message: String, additionalProperties: Map<String, Any?>? = null) {

0xnm · 2023-08-22T08:19:11Z

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

@@ -113,7 +113,7 @@ internal class TelemetryEventHandler(

        val eventIdentity = event.identity

-        if (seenInCurrentSession.contains(eventIdentity)) {
+        if (!event.isMetric && seenInCurrentSession.contains(eventIdentity)) {


do we have a risk of sending too much metrics by disabling this check and thus adding a lot of pressure on the storage and also on the Datadog servers?

I was following what was done in iOS here. I guess the risk was assessed already there, in the end we are going to have an extra sampler in the MetricsDispatcher which will come in the next PR. We have a 15% extra sampler there.

0xnm · 2023-08-22T08:19:52Z

features/dd-sdk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/RumFeatureTest.kt

+    }
+
+    @Test
+    fun `𝕄 handle metric event 𝕎 onReceive(){no additionalProperties}`(


probably test with missing message is missing

jonathanmos · 2023-08-22T11:18:34Z

features/dd-sdk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/RumFeatureTest.kt

+        val fakeAdditionalProperties = forge.exhaustiveAttributes()
+        testedFeature.onInitialize(appContext.mockInstance)
+        val event = mapOf(
+            "type" to "mobile_metric",


minor: "mobile_metric" is hardcoded in the tests in several places - perhaps it could be taken from a constant so as to only change in one place in future if the value changed?

xgouchet · 2023-08-22T13:06:33Z

dd-sdk-android-core/src/test/kotlin/com/datadog/android/core/SdkInternalLoggerTest.kt

@@ -444,6 +446,53 @@ internal class SdkInternalLoggerTest {
            )
    }

+    @Test
+    fun `𝕄 send metric 𝕎 metric`(


Suggested change

fun `𝕄 send metric 𝕎 metric`(

fun `𝕄 send metric 𝕎 metric()`(

xgouchet · 2023-08-22T13:08:59Z

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

+     * @param messageBuilder the lambda building the metric message
+     * @param additionalProperties additional properties to add to the metric
+     */
+    fun metric(messageBuilder: () -> String, additionalProperties: Map<String, Any?>)


Function name should be action verbs. We should consider logMetric. Alternatively, we could instead use an InternalLogger.Level.METRIC instead, and reuse the existing top level methods. WDYT?

i'd rather keep a new method here to match what iOS added in the InternalLogger but will change it to logMetric

0xnm

lgtm. I left some comments, but they are not blocking.

0xnm · 2023-08-22T14:29:46Z

dd-sdk-android-core/api/dd-sdk-android-core.api

@@ -77,6 +77,7 @@ public abstract interface class com/datadog/android/api/InternalLogger {
 	public static final field Companion Lcom/datadog/android/api/InternalLogger$Companion;
 	public abstract fun log (Lcom/datadog/android/api/InternalLogger$Level;Lcom/datadog/android/api/InternalLogger$Target;Lkotlin/jvm/functions/Function0;Ljava/lang/Throwable;ZLjava/util/Map;)V
 	public abstract fun log (Lcom/datadog/android/api/InternalLogger$Level;Ljava/util/List;Lkotlin/jvm/functions/Function0;Ljava/lang/Throwable;ZLjava/util/Map;)V
+	public abstract fun metric (Lkotlin/jvm/functions/Function0;Ljava/util/Map;)V


this has to be updated

yeah it is something wrong with the api generation as I already ran that method 🤔

0xnm · 2023-08-22T14:33:01Z

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

+     * Logs a specific metric from the internal implementation. The metric will leverage the new
+     * added additionalProperties in the


I'm still not quite clear with the description. Does internal implementation mean from SDK-related code only? Also I guess there is a typo in new added, and not quite clear how additionalProperties will be leveraged. Maybe we can say that metric data (type, values, etc.) will be taken from additionalProperties? This way it is more clear imo.

the internal implementation is present in all the other methods description so I just followed the pattern. And yes for me it means that is meant for internal usage inside the SDK code. I will try to update the last part there ;)

codecov-commenter · 2023-08-23T07:33:16Z

Codecov Report

Merging #1581 (71a10da) into develop (3bcb7ae) will decrease coverage by 0.04%.
Report is 4 commits behind head on develop.
The diff coverage is 88.37%.

@@             Coverage Diff             @@
##           develop    #1581      +/-   ##
===========================================
- Coverage    83.37%   83.34%   -0.04%     
===========================================
  Files          437      437              
  Lines        14910    14942      +32     
  Branches      2230     2237       +7     
===========================================
+ Hits         12431    12452      +21     
- Misses        1885     1894       +9     
- Partials       594      596       +2

Files Changed	Coverage Δ
...n/kotlin/com/datadog/android/api/InternalLogger.kt	`100.00% <ø> (ø)`
...om/datadog/android/telemetry/internal/Telemetry.kt	`12.50% <0.00%> (-2.88%)`	⬇️
...tlin/com/datadog/android/core/SdkInternalLogger.kt	`89.01% <87.50%> (-0.15%)`	⬇️
...lin/com/datadog/android/rum/internal/RumFeature.kt	`92.72% <94.74%> (+0.25%)`	⬆️
...g/android/rum/internal/domain/scope/RumRawEvent.kt	`100.00% <100.00%> (ø)`
.../android/rum/internal/monitor/DatadogRumMonitor.kt	`88.58% <100.00%> (+1.24%)`	⬆️
...ndroid/telemetry/internal/TelemetryEventHandler.kt	`75.64% <100.00%> (-1.28%)`	⬇️

... and 15 files with indirect coverage changes

mariusc83 self-assigned this Aug 22, 2023

mariusc83 marked this pull request as ready for review August 22, 2023 07:41

mariusc83 requested a review from a team as a code owner August 22, 2023 07:41

0xnm reviewed Aug 22, 2023

View reviewed changes

jonathanmos previously approved these changes Aug 22, 2023

View reviewed changes

mariusc83 dismissed jonathanmos’s stale review via 35e8143 August 22, 2023 12:51

mariusc83 force-pushed the mconstantin/rumm-924/add-the-internalloggger-metrics-method branch from cfde8e0 to 35e8143 Compare August 22, 2023 12:51

mariusc83 requested review from 0xnm and jonathanmos August 22, 2023 12:59

xgouchet reviewed Aug 22, 2023

View reviewed changes

mariusc83 force-pushed the mconstantin/rumm-924/add-the-internalloggger-metrics-method branch 4 times, most recently from b2af158 to c2f62d2 Compare August 22, 2023 14:30

0xnm previously approved these changes Aug 22, 2023

View reviewed changes

mariusc83 dismissed 0xnm’s stale review via ac0ffa6 August 23, 2023 07:22

mariusc83 force-pushed the mconstantin/rumm-924/add-the-internalloggger-metrics-method branch from c2f62d2 to ac0ffa6 Compare August 23, 2023 07:22

mariusc83 force-pushed the mconstantin/rumm-924/add-the-internalloggger-metrics-method branch from ac0ffa6 to 71a10da Compare August 23, 2023 07:33

mariusc83 requested a review from 0xnm August 23, 2023 07:34

xgouchet approved these changes Aug 23, 2023

View reviewed changes

jonathanmos approved these changes Aug 23, 2023

View reviewed changes

Introduce the new InternalLogger#metric API

409b4e0

mariusc83 force-pushed the mconstantin/rumm-924/add-the-internalloggger-metrics-method branch from 71a10da to 409b4e0 Compare August 23, 2023 09:31

mariusc83 merged commit 2b41ee8 into develop Aug 23, 2023

mariusc83 deleted the mconstantin/rumm-924/add-the-internalloggger-metrics-method branch August 23, 2023 11:07

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-924 Introduce the new InternalLogger#metric API #1581

RUMM-924 Introduce the new InternalLogger#metric API #1581

mariusc83 commented Aug 22, 2023

0xnm Aug 22, 2023

0xnm Aug 22, 2023

0xnm Aug 22, 2023

mariusc83 Aug 22, 2023

0xnm Aug 22, 2023

mariusc83 Aug 22, 2023

0xnm Aug 22, 2023

0xnm Aug 22, 2023

mariusc83 Aug 22, 2023

0xnm Aug 22, 2023

jonathanmos Aug 22, 2023

xgouchet Aug 22, 2023

xgouchet Aug 22, 2023

mariusc83 Aug 22, 2023

0xnm left a comment

0xnm Aug 22, 2023

mariusc83 Aug 22, 2023

0xnm Aug 22, 2023

mariusc83 Aug 22, 2023

codecov-commenter commented Aug 23, 2023 •

edited

Loading

	fun metric(message: String, additionalProperties: Map<String, Any?>? = null) {

	fun metric(message: String, additionalProperties: Map<String, Any?>? = null) {

	fun `𝕄 send metric 𝕎 metric`(
	fun `𝕄 send metric 𝕎 metric()`(

		* Logs a specific metric from the internal implementation. The metric will leverage the new
		* added additionalProperties in the

RUMM-924 Introduce the new InternalLogger#metric API #1581

RUMM-924 Introduce the new InternalLogger#metric API #1581

Conversation

mariusc83 commented Aug 22, 2023

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xnm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 23, 2023 • edited Loading

Codecov Report

codecov-commenter commented Aug 23, 2023 •

edited

Loading