RUMM-2767 Handle SR requests through SessionReplayRequestsFactory by mariusc83 · Pull Request #1176 · DataDog/dd-sdk-android

mariusc83 · 2022-12-07T12:17:51Z

What does this PR do?

As a follow up task on the RFC in this PR we are handling the SR data upload requests through the SessionReplayRequestFactory. Also we are making sure that the new events are stored in a new batch whenever the RUM context changed.

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)

codecov-commenter · 2022-12-08T09:51:16Z

Codecov Report

Merging #1176 (211a0eb) into feature/sdkv2 (16217da) will increase coverage by 1.17%.
The diff coverage is 94.71%.

@@                Coverage Diff                @@
##           feature/sdkv2    #1176      +/-   ##
=================================================
+ Coverage          82.09%   83.27%   +1.17%     
=================================================
  Files                355      355              
  Lines              11973    11957      -16     
  Branches            2049     2058       +9     
=================================================
+ Hits                9829     9956     +127     
+ Misses              1506     1359     -147     
- Partials             638      642       +4

Impacted Files	Coverage Δ
...droid/sessionreplay/net/BatchesToSegmentsMapper.kt	`86.96% <85.71%> (+1.24%)`	⬆️
...essionreplay/internal/domain/RequestBodyFactory.kt	`97.87% <97.87%> (ø)`
...lay/internal/domain/SessionReplayRequestFactory.kt	`97.06% <98.46%> (+83.73%)`	⬆️
...play/internal/storage/SessionReplayRecordWriter.kt	`100.00% <100.00%> (+83.33%)`	⬆️
.../kotlin/com/datadog/android/v2/core/DatadogCore.kt	`88.55% <100.00%> (-0.58%)`	⬇️
...droid/rum/tracking/ActivityViewTrackingStrategy.kt	`86.54% <0.00%> (-1.92%)`	⬇️
...al/persistence/file/batch/BatchFileOrchestrator.kt	`92.66% <0.00%> (-1.83%)`	⬇️
...lin/com/datadog/android/rum/internal/RumFeature.kt	`90.83% <0.00%> (-1.67%)`	⬇️
...ndroid/telemetry/internal/TelemetryEventHandler.kt	`72.50% <0.00%> (-0.83%)`	⬇️
... and 12 more

0xnm · 2022-12-08T09:15:16Z

+            // TODO: RUMM-2397 Add the proper logs here once the sdkLogger will be added
+            // the data in the batch is corrupted. We return an empty request just to make
+            // sure the batch will be deleted by the core.
+            return getEmptyRequest()


should we maybe change RequestFactory signature to allow null return value? Otherwise this case looks like a hack, which will also trigger some telemetry.

I think this will be nice if we can do so.

Yes, empty request looks like a hack and we shouldn't even make an attempt to send it.

0xnm · 2022-12-09T08:27:02Z

+            // TODO: RUMM-2397 Add the proper logs here once the sdkLogger will be added
+            // the data in the batch is corrupted. We return an empty request just to make
+            // sure the batch will be deleted by the core.
+            return getEmptyRequest()


Yes, empty request looks like a hack and we shouldn't even make an attempt to send it.

0xnm · 2022-12-09T08:37:09Z

+
+        if (startTimestamp == null || stopTimestamp == null) {
+            // this is just to avoid having kotlin warnings but the elements
+            // without timestamp property were already removed int the logic above


Suggested change

// without timestamp property were already removed int the logic above

// without timestamp property were already removed in the logic above

0xnm

LGTM. I've added few suggestions, but they are not blocking for me.

The only concern is "empty request" thing, but let's create another task to do something with it eventually.

0xnm · 2022-12-12T09:39:51Z

      - "kotlin.collections.MutableSet.map(kotlin.Function1)"
      - "kotlin.collections.MutableSet.remove(java.io.File)"
      - "kotlin.collections.MutableSet.remove(com.datadog.android.v2.core.internal.storage.ConsentAwareStorage.Batch)"
+      - "kotlin.collections.Iterable.toMap(kotlin.collections.MutableMap)"


minor: should go up, to L1058 (where Iterable is declared)

Actually that's a duplicate from a rebase :)

0xnm · 2022-12-12T09:44:34Z

+    private fun JsonObject.records(): JsonArray? {
+        return get(EnrichedRecord.RECORDS_KEY)?.asJsonArray
+    }
+
+    private fun JsonObject.timestamp(): Long? {
+        return getAsJsonPrimitive(TIMESTAMP_KEY)?.asLong
+    }
+
+    private fun JsonObject.rumContext(): SessionReplayRumContext? {
+        val applicationId = get(EnrichedRecord.APPLICATION_ID_KEY)?.asString
+        val sessionId = get(EnrichedRecord.SESSION_ID_KEY)?.asString
+        val viewId = get(EnrichedRecord.VIEW_ID_KEY)?.asString


these methods can still throw json related exceptions and they are not covered by try-catch

0xnm · 2022-12-12T09:45:28Z

+import org.mockito.quality.Strictness
+
+@Extensions(
+    ExtendWith(MockitoExtension::class),


minor: we don't really need this one (and MockitoSettings annotation as well), because we don't mock anything in this test.

0xnm · 2022-12-12T09:47:01Z

+    override fun getForgery(forge: Forge): JsonObject {
+        return JsonObject().apply {
+            val fieldCount = forge.aTinyInt()
+            for (i in 0..fieldCount) {


Suggested change

for (i in 0..fieldCount) {

repeat(fieldCount + 1) {

mariusc83 self-assigned this Dec 7, 2022

mariusc83 force-pushed the mconstantin/rumm-2767/handle-sr-requests-from-requests-factory branch 3 times, most recently from f1c7ac1 to 5a43262 Compare December 7, 2022 13:15

mariusc83 marked this pull request as ready for review December 7, 2022 13:17

mariusc83 requested a review from a team as a code owner December 7, 2022 13:17

mariusc83 force-pushed the mconstantin/rumm-2767/handle-sr-requests-from-requests-factory branch 2 times, most recently from 0b711b5 to 0fe31e3 Compare December 7, 2022 15:13

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

Base automatically changed from mconstantin/rumm-2767/add-forcenewbatch-option to feature/sdkv2 December 8, 2022 09:51

0xnm requested changes Dec 8, 2022

View reviewed changes

mariusc83 force-pushed the mconstantin/rumm-2767/handle-sr-requests-from-requests-factory branch 3 times, most recently from ed8677c to bd95086 Compare December 8, 2022 13:12

mariusc83 requested a review from 0xnm December 8, 2022 14:31

mariusc83 force-pushed the mconstantin/rumm-2767/handle-sr-requests-from-requests-factory branch from bd95086 to 211a0eb Compare December 8, 2022 14:52

0xnm reviewed Dec 9, 2022

View reviewed changes

mariusc83 force-pushed the mconstantin/rumm-2767/handle-sr-requests-from-requests-factory branch 6 times, most recently from 57491fe to e4c4d1d Compare December 9, 2022 12:23

mariusc83 requested a review from 0xnm December 9, 2022 13:28

0xnm approved these changes Dec 12, 2022

View reviewed changes

mariusc83 force-pushed the mconstantin/rumm-2767/handle-sr-requests-from-requests-factory branch 2 times, most recently from 295508b to 0549eb2 Compare December 12, 2022 11:07

RUMM-2767 Handle SR requests through SessionReplayRequestsFactory

347b709

mariusc83 force-pushed the mconstantin/rumm-2767/handle-sr-requests-from-requests-factory branch from 0549eb2 to 347b709 Compare December 12, 2022 11:45

mariusc83 merged commit fb6de14 into feature/sdkv2 Dec 12, 2022

mariusc83 deleted the mconstantin/rumm-2767/handle-sr-requests-from-requests-factory branch December 12, 2022 12:10

xgouchet added this to the 1.17.0 milestone Dec 13, 2023

	// without timestamp property were already removed int the logic above
	// without timestamp property were already removed in the logic above

Uh oh!

Conversation

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

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mariusc83 commented Dec 7, 2022 •

edited

Loading

codecov-commenter commented Dec 8, 2022 •

edited

Loading