Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RUMM-2767 Handle SR requests through SessionReplayRequestsFactory #1176

Merged
merged 1 commit into from
Dec 12, 2022
Merged

RUMM-2767 Handle SR requests through SessionReplayRequestsFactory #1176

merged 1 commit into from
Dec 12, 2022

Conversation

mariusc83
Copy link
Member

@mariusc83 mariusc83 commented Dec 7, 2022 •
edited
Loading

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)

@mariusc83 mariusc83 self-assigned this Dec 7, 2022
@mariusc83 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 mariusc83 marked this pull request as ready for review December 7, 2022 13:17
@mariusc83 mariusc83 requested a review from a team as a code owner December 7, 2022 13:17
@mariusc83 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 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
@codecov-commenter
Copy link

codecov-commenter commented Dec 8, 2022 •
edited
Loading

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
0xnm requested changes Dec 8, 2022
View reviewed changes
...main/kotlin/com/datadog/android/sessionreplay/internal/domain/SessionReplayRequestFactory.kt
// 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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mariusc83 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 mariusc83 requested a review from 0xnm December 8, 2022 14:31
@mariusc83 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
0xnm reviewed Dec 9, 2022
View reviewed changes
...main/kotlin/com/datadog/android/sessionreplay/internal/domain/SessionReplayRequestFactory.kt
// 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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

...sion-replay/src/main/kotlin/com/datadog/android/sessionreplay/net/BatchesToSegmentsMapper.kt Outdated

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mariusc83 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 mariusc83 requested a review from 0xnm December 9, 2022 13:28
0xnm
0xnm approved these changes Dec 12, 2022
View reviewed changes
Copy link
Member

@0xnm 0xnm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

detekt.yml Outdated
@@ -1177,9 +1179,15 @@ datadog:
- "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)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually that's a duplicate from a rebase :)

...sion-replay/src/main/kotlin/com/datadog/android/sessionreplay/net/BatchesToSegmentsMapper.kt Outdated
Comment on lines 125 to 139
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

...android-session-replay/src/test/kotlin/com/datadog/android/sessionreplay/gson/GsonExtTest.kt Outdated
import org.mockito.quality.Strictness

@Extensions(
ExtendWith(MockitoExtension::class),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

...play/src/test/kotlin/com/datadog/android/sessionreplay/utils/GsonJsonObjectForgeryFactory.kt Outdated
override fun getForgery(forge: Forge): JsonObject {
return JsonObject().apply {
val fieldCount = forge.aTinyInt()
for (i in 0..fieldCount) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (i in 0..fieldCount) {
repeat(fieldCount + 1) {

@mariusc83 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
@mariusc83 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 mariusc83 merged commit fb6de14 into feature/sdkv2 Dec 12, 2022
@mariusc83 mariusc83 deleted the mconstantin/rumm-2767/handle-sr-requests-from-requests-factory branch December 12, 2022 12:10
@xgouchet xgouchet added this to the 1.17.0 milestone Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xnm 0xnm 0xnm approved these changes

Assignees

@mariusc83 mariusc83

Labels
None yet
Projects
None yet
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.
4 participants
@mariusc83 @codecov-commenter @0xnm @xgouchet