REPLAY-1887 Provide the uploadFrequency per feature by mariusc83 · Pull Request #1533 · DataDog/dd-sdk-android

mariusc83 · 2023-07-10T07:25:25Z

What does this PR do?

This PR is follow - up on the ios PR where we are providing a custom (batchSize, uploadFrequency) configuration for Session Replay feature to decrease the number of sessions with corrupted/not-enough records.

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

It works, my main concern is making the core configuration's upload frequency optional.

It would be better to keep the core configuration mandatory, and make individual features override optional. If you flip the resolveUploadFrequency logic to the following, it would work better (that way people can just override the one feature they want a different upload frequency)

    private fun resolveUploadFrequency(
        coreFeature: CoreFeature,
        storageConfiguration: FeatureStorageConfiguration
    ): UploadFrequency {
        return storageConfiguration.uploadFrequency ?: coreFeature.uploadFrequency
    }

mariusc83 · 2023-07-10T07:52:28Z

It works, my main concern is making the core configuration's upload frequency optional.

It would be better to keep the core configuration mandatory, and make individual features override optional. If you flip the resolveUploadFrequency logic to the following, it would work better (that way people can just override the one feature they want a different upload frequency)
    private fun resolveUploadFrequency(
        coreFeature: CoreFeature,
        storageConfiguration: FeatureStorageConfiguration
    ): UploadFrequency {
        return storageConfiguration.uploadFrequency ?: coreFeature.uploadFrequency
    }

Tks @xgouchet, I think that's even better for now. My idea was that at some point we will drop this API at the core level as it doesn't make sense to have it anymore there. We could simplify it by having this only in the StorageConfiguration. But to keep a backward compatibility we could go this way indeed. I would like to hear also @0xnm thoughts on that.

ncreated

The override logic looks good 👌

codecov-commenter · 2023-07-10T11:47:24Z

Codecov Report

Merging #1533 (702f13d) into develop (ee5007f) will decrease coverage by 0.26%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #1533      +/-   ##
===========================================
- Coverage    83.62%   83.36%   -0.26%     
===========================================
  Files          438      440       +2     
  Lines        14854    14871      +17     
  Branches      2237     2239       +2     
===========================================
- Hits         12421    12396      -25     
- Misses        1868     1884      +16     
- Partials       565      591      +26

Impacted Files	Coverage Δ
...android/api/storage/FeatureStorageConfiguration.kt	`100.00% <100.00%> (ø)`
...dog/android/core/internal/BatchingDelayResolver.kt	`100.00% <100.00%> (ø)`
...in/com/datadog/android/core/internal/SdkFeature.kt	`91.00% <100.00%> (+0.47%)`	⬆️
...g/android/core/internal/UploadFrequencyResolver.kt	`100.00% <100.00%> (ø)`
...core/internal/data/upload/v2/DataUploadRunnable.kt	`100.00% <100.00%> (ø)`
...ore/internal/data/upload/v2/DataUploadScheduler.kt	`100.00% <100.00%> (ø)`
...d/core/internal/persistence/ConsentAwareStorage.kt	`98.75% <100.00%> (ø)`
...oid/sessionreplay/internal/SessionReplayFeature.kt	`100.00% <100.00%> (ø)`

... and 17 files with indirect coverage changes

0xnm · 2023-07-10T14:04:59Z

+    val uploadFrequency: UploadFrequency? = null,
+    val batchSize: BatchSize? = null


it makes more sense to put default values into the DEFAULT object maybe?

actually not...I prefer NULL, it's easier when I resolve them (if it is set here I take this one if not I take the core one)

Yes, what I mean is to keep nullable type here, but put default null value to the DEFAULT object.

hmm let me look into that, not sure I understand :)

oh yeah, I see your point, done ;)

0xnm · 2023-07-10T14:06:36Z

+
+import com.datadog.android.api.storage.FeatureStorageConfiguration
+
+internal class BatchingDelayResolver {


imo it is a bit too much of abstractions, let's maybe just mock values returned by the wrappedFeature in the SdkFeature? Same for another resolver.

I get your point, in the same time I prefer to have them separated and do not mix responsibilities. It's way easier for unit testing. I also suspect that this logic will change later so in that case SdkFeature will not change anymore and we will just change the resolver. What do you think ?

not a fan of the overhead introduced by the resolvers either, it would be cleaner to not use them IMO

I will try to add these as private methods instead, just need to find a better way to test it without too much noise.

0xnm · 2023-07-10T14:07:46Z

    private val networkInfoProvider: NetworkInfoProvider,
    private val systemInfoProvider: SystemInfoProvider,
-    uploadFrequency: UploadFrequency,
+    internal val uploadFrequency: UploadFrequency,


why this change is needed?

0xnm · 2023-07-11T07:04:51Z

+    }
+
+    @Test
+    fun `M provide the core frequency W resolveUploadFrequency{not provide in storage config}`() {


Suggested change

fun `M provide the core frequency W resolveUploadFrequency{not provide in storage config}`() {

fun `M provide the core frequency W resolveUploadFrequency{not provided in storage config}`() {

0xnm

lgtm, added few comments

0xnm · 2023-07-11T09:36:29Z

+ * @property uploadFrequency the desired upload frequency policy
+ * @property batchSize the desired batch size policy


minor: maybe we should say, that if not explicitly provided, then value will be taken from the core configuration.

0xnm · 2023-07-11T09:37:28Z

+        whenever(mockWrappedFeature.storageConfiguration)
+            .thenReturn(fakeStorageConfig)
+        // When
+
+        testedFeature.initialize(appContext.mockInstance)


Suggested change

whenever(mockWrappedFeature.storageConfiguration)

.thenReturn(fakeStorageConfig)

// When

testedFeature.initialize(appContext.mockInstance)

whenever(mockWrappedFeature.storageConfiguration)

.thenReturn(fakeStorageConfig)

// When

testedFeature.initialize(appContext.mockInstance)

0xnm · 2023-07-11T09:38:05Z

+    }
+
+    @Test
+    fun `𝕄 initialize the storage 𝕎 initialize()`() {


with core batch size probably is missing in the name?

I am considering this as default initialization so wanted to keep it as that in the test description

mariusc83 self-assigned this Jul 10, 2023

xgouchet suggested changes Jul 10, 2023

View reviewed changes

mariusc83 closed this Jul 10, 2023

mariusc83 reopened this Jul 10, 2023

mariusc83 force-pushed the mconstantin/replay-1887/make-replay-data-uploader-more-eager branch 2 times, most recently from 32f490b to 5555a4a Compare July 10, 2023 11:00

mariusc83 marked this pull request as ready for review July 10, 2023 11:05

mariusc83 requested a review from a team as a code owner July 10, 2023 11:05

ncreated reviewed Jul 10, 2023

View reviewed changes

mariusc83 force-pushed the mconstantin/replay-1887/make-replay-data-uploader-more-eager branch from 5555a4a to fb29a38 Compare July 10, 2023 11:30

mariusc83 requested a review from xgouchet July 10, 2023 12:00

0xnm reviewed Jul 10, 2023

View reviewed changes

0xnm approved these changes Jul 11, 2023

View reviewed changes

mariusc83 force-pushed the mconstantin/replay-1887/make-replay-data-uploader-more-eager branch 3 times, most recently from cd35303 to 702f13d Compare July 11, 2023 08:59

xgouchet approved these changes Jul 11, 2023

View reviewed changes

mariusc83 force-pushed the mconstantin/replay-1887/make-replay-data-uploader-more-eager branch from 702f13d to 81fa32a Compare July 11, 2023 09:29

mariusc83 requested review from 0xnm and xgouchet July 11, 2023 09:30

0xnm approved these changes Jul 11, 2023

View reviewed changes

REPLAY-1887 Provide the uploadFrequency per feature

6118490

mariusc83 force-pushed the mconstantin/replay-1887/make-replay-data-uploader-more-eager branch from 81fa32a to 6118490 Compare July 11, 2023 10:40

mariusc83 merged commit ccc5562 into develop Jul 11, 2023

mariusc83 deleted the mconstantin/replay-1887/make-replay-data-uploader-more-eager branch July 11, 2023 13:55

xgouchet added this to the 2.0.0 milestone Dec 13, 2023

		val uploadFrequency: UploadFrequency? = null,
		val batchSize: BatchSize? = null


		import com.datadog.android.api.storage.FeatureStorageConfiguration

		internal class BatchingDelayResolver {

	fun `M provide the core frequency W resolveUploadFrequency{not provide in storage config}`() {
	fun `M provide the core frequency W resolveUploadFrequency{not provided in storage config}`() {

		* @property uploadFrequency the desired upload frequency policy
		* @property batchSize the desired batch size policy

Uh oh!

Conversation

mariusc83 commented Jul 10, 2023 • 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!

xgouchet left a comment

Choose a reason for hiding this comment

Uh oh!

mariusc83 commented Jul 10, 2023

Uh oh!

ncreated left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jul 10, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

0xnm Jul 10, 2023 • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mariusc83 commented Jul 10, 2023 •

edited

Loading

codecov-commenter commented Jul 10, 2023 •

edited

Loading

0xnm Jul 10, 2023 •

edited

Loading