RUMM-2076 Add the Configuration#setVitalsMonitorUpdateFrequency API by mariusc83 · Pull Request #926 · DataDog/dd-sdk-android

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

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

+        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

+    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


    @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


    @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

            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 :)

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


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

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

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

Uh oh!

Conversation

mariusc83 commented May 5, 2022

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!

Uh oh!

codecov-commenter commented May 6, 2022 • 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!

0xnm left a comment • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented May 6, 2022 •

edited

Loading

0xnm left a comment •

edited

Loading