RUMM-924 Introduce the new InternalLogger#metric API by mariusc83 · Pull Request #1581 · DataDog/dd-sdk-android

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

    )

+    /**
+     * 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

        }
        rumFeature.sendEvent(telemetryEvent)
    }
+    private fun logMetric(


Suggested change

private fun logMetric(

private fun logMetric(

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

+        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

        (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

        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

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


probably test with missing message is missing

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

+        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

    }

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


Suggested change

fun `𝕄 send metric 𝕎 metric`(

fun `𝕄 send metric 𝕎 metric()`(

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

+     * @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

 	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

+     * 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

	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

Uh oh!

Conversation

mariusc83 commented Aug 22, 2023

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

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 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!

codecov-commenter commented Aug 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Aug 23, 2023 •

edited

Loading