RUMM-2767 Add the 'forceNewBatch' option into the FeatureScope by mariusc83 · Pull Request #1174 · DataDog/dd-sdk-android

mariusc83 · 2022-12-06T16:05:16Z

What does this PR do?

As a follow up task for the https://datadoghq.atlassian.net/wiki/spaces/RUMP/pages/2714764346/RFC+-+Handle+SR+data+storage+upload, we are adding a new option to the FeatureScope#withWriterContext method to allow us to force the orchestrator to start a new batch before writing the new data.

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-12-07T07:59:23Z

        return reusableFile ?: createNewFile()
    }

+    override fun getNewWritableFile(): File? {


instead of making a new method that is almost identical to getWritableFile you could instead do

@WorkerThread override fun getWritableFile(forceNewFile: Boolean = false): File? { if (!isRootDirValid()) { return null } deleteObsoleteFiles() freeSpaceIfNeeded() return if (!forceNewFile){ getReusableWritableFile() ?: createNewFile() } else { createNewFile() } }

Yes, was not very sure which way to go here. Thanks for sharing your opinion, will change that :)

codecov-commenter · 2022-12-07T08:00:36Z

Codecov Report

Merging #1174 (06b450a) into feature/sdkv2 (d451d7e) will increase coverage by 0.01%.
The diff coverage is 75.00%.

@@                Coverage Diff                @@
##           feature/sdkv2    #1174      +/-   ##
=================================================
+ Coverage          82.28%   82.29%   +0.01%     
=================================================
  Files                353      354       +1     
  Lines              11796    11808      +12     
  Branches            2007     2014       +7     
=================================================
+ Hits                9706     9717      +11     
- Misses              1465     1469       +4     
+ Partials             625      622       -3

Impacted Files	Coverage Δ
...ence/file/advanced/ConsentAwareFileOrchestrator.kt	`85.71% <0.00%> (-3.17%)`	⬇️
.../kotlin/com/datadog/android/v2/api/FeatureScope.kt	`50.00% <50.00%> (ø)`
...atadog/android/v2/core/internal/storage/Storage.kt	`25.00% <50.00%> (+25.00%)`	⬆️
...id/v2/core/internal/storage/ConsentAwareStorage.kt	`97.83% <75.00%> (-1.06%)`	⬇️
...in/com/datadog/android/core/internal/SdkFeature.kt	`88.99% <100.00%> (-0.10%)`	⬇️
...al/persistence/file/batch/BatchFileOrchestrator.kt	`94.69% <100.00%> (+0.25%)`	⬆️
.../persistence/file/single/SingleFileOrchestrator.kt	`78.57% <100.00%> (+1.65%)`	⬆️
...src/main/kotlin/com/datadog/android/DatadogSite.kt	`83.33% <0.00%> (-3.33%)`	⬇️
...ndroid/telemetry/internal/TelemetryEventHandler.kt	`69.17% <0.00%> (-3.33%)`	⬇️
...d/v2/core/internal/storage/FileEventBatchWriter.kt	`95.24% <0.00%> (-2.38%)`	⬇️
... and 10 more

0xnm

Nice change! Finally we will get rid of the hack for the SR.

I've added some comments, mostly regarding the tests or typos.

0xnm · 2022-12-07T13:25:53Z


-    override fun withWriteContext(callback: (DatadogContext, EventBatchWriter) -> Unit) {
+    override fun withWriteContext(
+        forceNewBoolean: Boolean,


forceNewBatch, maybe? :)

Suggested change

forceNewBoolean: Boolean,

forceNewBatch: Boolean,

Damn...yes, miss typing

0xnm · 2022-12-07T13:27:19Z

 internal interface FileOrchestrator {

    /**
+     * @param forceNewBatch if `true` will force the orchestrator to start a new file.


Mismatch between name in the doc and name in the function signature. I suppose it should be forceNewFile here?

yup....miss typing again

0xnm · 2022-12-07T13:28:48Z

 interface FeatureScope {
    /**
     * Utility to write an event, asynchronously.
+     * @param forceNewBoolean if `true` forces the [EventBatchWriter] to write in a new file and


Suggested change

* @param forceNewBoolean if `true` forces the [EventBatchWriter] to write in a new file and

* @param forceNewBatch if `true` forces the [EventBatchWriter] to write in a new file and

0xnm · 2022-12-07T13:29:00Z

     */
-    fun withWriteContext(callback: (DatadogContext, EventBatchWriter) -> Unit)
+    fun withWriteContext(
+        forceNewBoolean: Boolean = false,


Suggested change

forceNewBoolean: Boolean = false,

forceNewBatch: Boolean = false,

0xnm · 2022-12-07T13:30:30Z

-    fun writeCurrentBatch(datadogContext: DatadogContext, callback: (EventBatchWriter) -> Unit)
+    fun writeCurrentBatch(
+        datadogContext: DatadogContext,
+        forceNewBatch: Boolean = false,


do we need to have the default value here if in all the calls (apart of the tests) we will get the value explicitly?

Good question, need to check it out

0xnm · 2022-12-07T13:43:28Z

    // region FeatureScope

-    @Test
+    @ParameterizedTest


not sure if we need a parametrized test here, given that we don't assert anything related to the forceNewBatch value.

We can simply use BoolForgery on the forceNewBatch test parameter. And coverage will be the same, because the only place where code path is different depending on the forceNewBatch parameter is BatchFileOrchestrator.

Yeah but I rather make sure we test with both true/false value.

IMO this would make sense if some assertion depends on that, which is not the case in most of the test. Anyway we already have kinda check of the proper flow by doing whenever(object.methodCall(parameter)), because if the parameter is not the one we provided in whenever, mock won't work and test will break anyway.

But I'm not pushing for it :)

0xnm · 2022-12-07T13:44:14Z


-    @Test
-    fun `𝕄 do nothing 𝕎 withWriteContext(callback) { no Datadog context }`() {
+    @ParameterizedTest


Same as above, BoolForgery is enough, no need for the parametrized test here and in other test files where ValueSource is just true or false.

0xnm · 2022-12-07T13:47:48Z

+        val end = System.currentTimeMillis()
+
+        // Then
+        checkNotNull(result)


I think it is better to use assertion method from AssertJ here instead of the Kotlin stdlib

Suggested change

checkNotNull(result)

assertThat(result).isNotNull()

hmm yes, I think I just duplicated that function so I need to change it also in the original.

Yeah I think this is everywhere in this test class and removing it will imply other changes :(. I would rather keep it like this and maybe open a new PR.

0xnm · 2022-12-07T13:50:24Z

+
+        // When
+        val start = System.currentTimeMillis()
+        val result = testedOrchestrator.getWritableFile(forceNewFile = true)


I think result with forceNewFile = false will be the same, because it is the fresh state of the orchestrator and it will always return the new file.

What we need to do is to call getWritableFile twice (one is default call, another with forceNewFile = true) and check that second result is not the same as first and was created after the first call.

0xnm

lgtm!

mariusc83 self-assigned this Dec 6, 2022

mariusc83 force-pushed the mconstantin/rumm-2767/add-forcenewbatch-option branch from de732cb to 404b92a Compare December 6, 2022 18:11

mariusc83 marked this pull request as ready for review December 6, 2022 18:35

mariusc83 requested a review from a team as a code owner December 6, 2022 18:35

mariusc83 force-pushed the mconstantin/rumm-2767/add-forcenewbatch-option branch from 404b92a to 06b450a Compare December 6, 2022 18:39

xgouchet suggested changes Dec 7, 2022

View reviewed changes

mariusc83 force-pushed the mconstantin/rumm-2767/add-forcenewbatch-option branch 2 times, most recently from d18426f to 83e6530 Compare December 7, 2022 09:46

mariusc83 requested a review from xgouchet December 7, 2022 10:42

xgouchet approved these changes Dec 7, 2022

View reviewed changes

0xnm requested changes Dec 7, 2022

View reviewed changes

mariusc83 force-pushed the mconstantin/rumm-2767/add-forcenewbatch-option branch from 83e6530 to 351eae5 Compare December 7, 2022 16:58

mariusc83 requested a review from 0xnm December 7, 2022 17:01

RUMM-2767 Add the 'forceNewBatch' option into the FeatureScope

7a0b30f

mariusc83 force-pushed the mconstantin/rumm-2767/add-forcenewbatch-option branch from 351eae5 to 7a0b30f Compare December 8, 2022 08:50

0xnm approved these changes Dec 8, 2022

View reviewed changes

mariusc83 merged commit 5b26dc9 into feature/sdkv2 Dec 8, 2022

mariusc83 deleted the mconstantin/rumm-2767/add-forcenewbatch-option branch December 8, 2022 09:51

xgouchet added this to the 1.17.0 milestone Dec 13, 2023

	* @param forceNewBoolean if `true` forces the [EventBatchWriter] to write in a new file and
	* @param forceNewBatch if `true` forces the [EventBatchWriter] to write in a new file and

	forceNewBoolean: Boolean = false,
	forceNewBatch: Boolean = false,

Uh oh!

Conversation

mariusc83 commented Dec 6, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mariusc83 commented Dec 6, 2022 •

edited

Loading

codecov-commenter commented Dec 7, 2022 •

edited

Loading