RUMM-2076 Add the Configuration#setVitalsMonitorUpdateFrequency API #926

mariusc83 · 2022-05-05T13:36:45Z

What does this PR do?

In this PR we are introduce a new API : ConfigurationBuilder.setVitalsUpdateFrequency(frequency) where the frequency is an enum :

/** Every 100 milliseconds. */
FREQUENT(100L),

/** Every 500 milliseconds. This is the default frequency. */
AVERAGE(500L),

/** Every 1000 milliseconds. */
RARE(1000L),

/** No data will be sent for mobile vitals. */
NEVER(0)

This will allow the client to customise the update frequency for mobile vitals reader or disable this feature.

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)

xgouchet · 2022-05-06T06:29:56Z

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

+ * Defines the frequency at which mobile vitals monitor updates the data.
+ */
+enum class VitalsUpdateFrequency(
+    internal val frequencyInMs: Long


This should be renamed periodInMs.

Frequency is measured in Hertz. The value in ms is the period (the inverse of frequency). E.g. for a period of 100ms, the frequency is 10 Hz.

xgouchet · 2022-05-06T06:35:00Z

dd-sdk-android/src/main/kotlin/com/datadog/android/rum/internal/RumFeature.kt

+        if (configuration.vitalsMonitorUpdateFrequency != VitalsUpdateFrequency.NEVER) {
+            initializeVitalMonitors(configuration.vitalsMonitorUpdateFrequency.frequencyInMs)
+        }


Two quick things, first of all, it'd make the onInitialize method easier to read if you'd call just

initializeVitalMonitors(configuration.vitalsMonitorUpdateFrequency)

and did the check just there. Also let's only extract the periodInMs when it's actually needed

I had it like that in the first place, honestly was not sure which way would be better as doing the check in the method means a premature return, sometimes I like to avoid this. But yeah, I think it looks cleaner like that.

xgouchet · 2022-05-06T06:39:55Z

...k-android/src/test/kotlin/com/datadog/android/core/configuration/ConfigurationBuilderTest.kt

+    fun `M use the given frequency W setVitalsMonitorUpdateFrequency`(forge: Forge) {
+        // Given
+        val fakeFrequency = forge.aValueFrom(VitalsUpdateFrequency::class.java)
+


Suggested change

fun `M use the given frequency W setVitalsMonitorUpdateFrequency`(forge: Forge) {

// Given

val fakeFrequency = forge.aValueFrom(VitalsUpdateFrequency::class.java)

fun `M use the given frequency W setVitalsMonitorUpdateFrequency`(

@Forgery fakeFrequency: VitalsUpdateFrequency

) {

xgouchet · 2022-05-06T06:43:10Z

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

@@ -217,9 +220,17 @@ internal class RumFeatureTest : SdkFeatureTest<Any, Configuration.Feature.RUM, R
    }

    @Test
-    fun `𝕄 setup vital monitors 𝕎 initialize()`() {
+    fun `𝕄 setup vital monitors 𝕎 initialize { frequency != NEVER }`(forge: Forge) {


@ParameterizedTest @EnumSource(VitalsUpdateFrequency::class, names = ["NEVER"], mode = EnumSource.Mode.EXCLUDE) fun `𝕄 setup vital monitors 𝕎 initialize { frequency != NEVER }`(fakeFrequency: VitalsUpdateFrequency) { … )

Yeah...make sense to have a parameterized test here instead.

xgouchet · 2022-05-06T06:44:09Z

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

@@ -311,15 +371,38 @@ internal class RumFeatureTest : SdkFeatureTest<Any, Configuration.Feature.RUM, R
    }

    @Test
-    fun `𝕄 initialize vital executor 𝕎 initialize()`() {
+    fun `𝕄 initialize vital executor 𝕎 initialize { frequency != NEVER }()`(forge: Forge) {


@ParameterizedTest @EnumSource(VitalsUpdateFrequency::class, names = ["NEVER"], mode = EnumSource.Mode.EXCLUDE) fun `𝕄 initialize vital executor 𝕎 initialize { frequency != NEVER }`(fakeFrequency: VitalsUpdateFrequency) { … )

0xnm · 2022-05-06T07:45:07Z

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

@@ -737,7 +749,8 @@ internal constructor(
            viewTrackingStrategy = ActivityViewTrackingStrategy(false),
            longTaskTrackingStrategy = MainLooperLongTaskStrategy(DEFAULT_LONG_TASK_THRESHOLD_MS),
            rumEventMapper = NoOpEventMapper(),
-            backgroundEventTracking = false
+            backgroundEventTracking = false,
+            vitalsMonitorUpdateFrequency = VitalsUpdateFrequency.AVERAGE


so we are moving from 100 ms to 500 ms by default?

Yeah, that was the agreement :)

dd-sdk-android/src/main/kotlin/com/datadog/android/rum/internal/RumFeature.kt

codecov-commenter · 2022-05-06T09:30:29Z

Codecov Report

Merging #926 (d0feae5) into develop (0b30629) will decrease coverage by 0.10%.
The diff coverage is 62.16%.

@@             Coverage Diff             @@
##           develop     #926      +/-   ##
===========================================
- Coverage    82.86%   82.76%   -0.10%     
===========================================
  Files          263      265       +2     
  Lines         8977     9006      +29     
  Branches      1449     1449              
===========================================
+ Hits          7438     7453      +15     
- Misses        1144     1159      +15     
+ Partials       395      394       -1

Impacted Files	Coverage Δ
...re/internal/thread/NoOpScheduledExecutorService.kt	`12.50% <12.50%> (ø)`
...atadog/android/core/configuration/Configuration.kt	`98.87% <100.00%> (+0.02%)`	⬆️
...ndroid/core/configuration/VitalsUpdateFrequency.kt	`100.00% <100.00%> (ø)`
...lin/com/datadog/android/rum/internal/RumFeature.kt	`92.93% <100.00%> (+1.26%)`	⬆️
.../android/rum/internal/monitor/DatadogRumMonitor.kt	`94.48% <0.00%> (-0.61%)`	⬇️
...android/rum/internal/ndk/DatadogNdkCrashHandler.kt	`87.93% <0.00%> (ø)`
...ndroid/core/internal/persistence/file/EventMeta.kt	`80.00% <0.00%> (ø)`

0xnm · 2022-05-06T09:58:42Z

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

@@ -81,6 +87,11 @@ internal class RumFeatureTest : SdkFeatureTest<Any, Configuration.Feature.RUM, R
        mockChoreographerInstance(mockChoreographer)
    }

+    @AfterEach
+    fun `stop RUM`() {
+        RumFeature.stop()


I think it is not needed, because parent class SdkFeatureTest will do a call to the testedFeature#stop

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

Line 113 in 0b30629

testedFeature.stop()

0xnm

Friday: I approved, but then I realized that we have no E2E test added for this new API.

0xnm · 2022-05-06T14:58:51Z

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

+         * @param frequency as [VitalsUpdateFrequency]
+         * @see [VitalsUpdateFrequency]
+         */
+        fun setVitalsUpdateFrequency(frequency: VitalsUpdateFrequency): Builder {


We need to add the E2E test for this one.

Good point, I will add a task for this.

xgouchet · 2022-05-09T08:10:46Z

dd-sdk-android/src/main/kotlin/com/datadog/android/rum/internal/RumFeature.kt

@@ -199,17 +210,18 @@ internal object RumFeature : SdkFeature<Any, Configuration.Feature.RUM>() {

    private fun initializeVitalMonitor(
        vitalReader: VitalReader,
-        vitalObserver: VitalObserver
+        vitalObserver: VitalObserver,
+        updateFrequencyInMs: Long


This should be named periodInMs too

mariusc83 requested a review from a team as a code owner May 5, 2022 13:36

mariusc83 self-assigned this May 5, 2022

xgouchet added the size-medium This PR is medium sized label May 5, 2022

mariusc83 force-pushed the mconstantin/rumm-2076/mobile-vitals-uptate-frequency-option branch from c559098 to 7720585 Compare May 5, 2022 14:43

xgouchet requested changes May 6, 2022

View reviewed changes

0xnm reviewed May 6, 2022

View reviewed changes

mariusc83 force-pushed the mconstantin/rumm-2076/mobile-vitals-uptate-frequency-option branch from 7720585 to a835ed6 Compare May 6, 2022 09:12

0xnm approved these changes May 6, 2022

View reviewed changes

0xnm reviewed May 6, 2022

View reviewed changes

mariusc83 force-pushed the mconstantin/rumm-2076/mobile-vitals-uptate-frequency-option branch 2 times, most recently from 9983485 to d0feae5 Compare May 9, 2022 07:34

mariusc83 requested a review from xgouchet May 9, 2022 07:47

xgouchet requested changes May 9, 2022

View reviewed changes

RUMM-2076 Add the Configuration#setVitalsMonitorUpdateFrequency API

b63545e

mariusc83 force-pushed the mconstantin/rumm-2076/mobile-vitals-uptate-frequency-option branch from d0feae5 to b63545e Compare May 10, 2022 09:00

mariusc83 requested a review from xgouchet May 10, 2022 09:01

xgouchet approved these changes May 11, 2022

View reviewed changes

mariusc83 merged commit d11f74e into develop May 12, 2022

mariusc83 deleted the mconstantin/rumm-2076/mobile-vitals-uptate-frequency-option branch May 12, 2022 13:13

xgouchet added this to the 1.14.0 milestone Dec 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUMM-2076 Add the Configuration#setVitalsMonitorUpdateFrequency API #926

RUMM-2076 Add the Configuration#setVitalsMonitorUpdateFrequency API #926

mariusc83 commented May 5, 2022

xgouchet May 6, 2022

xgouchet May 6, 2022

mariusc83 May 6, 2022

xgouchet May 6, 2022

xgouchet May 6, 2022

mariusc83 May 6, 2022

xgouchet May 6, 2022

0xnm May 6, 2022

mariusc83 May 6, 2022

codecov-commenter commented May 6, 2022 •

edited

Loading

0xnm May 6, 2022

0xnm left a comment •

edited

Loading

0xnm May 6, 2022

mariusc83 May 9, 2022

xgouchet May 9, 2022

RUMM-2076 Add the Configuration#setVitalsMonitorUpdateFrequency API #926

RUMM-2076 Add the Configuration#setVitalsMonitorUpdateFrequency API #926

Conversation

mariusc83 commented May 5, 2022

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

codecov-commenter commented May 6, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

0xnm left a comment • edited Loading

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 May 6, 2022 •

edited

Loading

0xnm left a comment •

edited

Loading