Skip to content

RUMM-2423 SnapshotProcessor - process wireframe mutations#1033

Merged
mariusc83 merged 1 commit into
feature/session-replay-vofrom
mconstantin/rumm-2423/snapshot-processor-compute-mutations
Sep 8, 2022
Merged

RUMM-2423 SnapshotProcessor - process wireframe mutations#1033
mariusc83 merged 1 commit into
feature/session-replay-vofrom
mconstantin/rumm-2423/snapshot-processor-compute-mutations

Conversation

@mariusc83

Copy link
Copy Markdown
Member

What does this PR do?

A brief description of the change being made with this pull request.

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 Sep 5, 2022
@mariusc83
mariusc83 force-pushed the mconstantin/rumm-2423/snapshot-processor-compute-mutations branch from 74b77d9 to b63c394 Compare September 5, 2022 10:01
@xgouchet xgouchet added the size-large This PR is large sized label Sep 5, 2022
@mariusc83
mariusc83 force-pushed the mconstantin/rumm-2423/snapshot-processor-compute-mutations branch from b63c394 to beaf333 Compare September 5, 2022 12:12
@codecov-commenter

codecov-commenter commented Sep 5, 2022

Copy link
Copy Markdown

Codecov Report

Merging #1033 (e6ffd2a) into feature/session-replay-vo (922fd29) will increase coverage by 0.24%.
The diff coverage is 88.91%.

@@                      Coverage Diff                      @@
##           feature/session-replay-vo    #1033      +/-   ##
=============================================================
+ Coverage                      82.99%   83.22%   +0.24%     
=============================================================
  Files                            307      323      +16     
  Lines                           9980    10424     +444     
  Branches                        1628     1734     +106     
=============================================================
+ Hits                            8282     8675     +393     
- Misses                          1193     1210      +17     
- Partials                         505      539      +34     
Impacted Files Coverage Δ
...nal/tracking/AndroidXFragmentLifecycleCallbacks.kt 93.48% <ø> (ø)
...m/datadog/android/sessionreplay/recorder/IntExt.kt 100.00% <ø> (ø)
.../android/sessionreplay/processor/EnrichedRecord.kt 35.29% <35.29%> (ø)
...oid/sessionreplay/utils/SessionReplayRumContext.kt 58.33% <58.33%> (ø)
...com/datadog/android/sessionreplay/recorder/Node.kt 66.67% <66.67%> (+66.67%) ⬆️
...ionreplay/recorder/mapper/ButtonWireframeMapper.kt 66.67% <66.67%> (ø)
...android/sessionreplay/recorder/SnapshotProducer.kt 77.14% <77.14%> (+27.14%) ⬆️
...y/recorder/mapper/ViewScreenshotWireframeMapper.kt 80.00% <80.00%> (ø)
...onreplay/recorder/mapper/GenericWireframeMapper.kt 85.71% <85.71%> (ø)
...ssionreplay/recorder/mapper/ViewWireframeMapper.kt 88.46% <88.46%> (ø)
... and 21 more

@mariusc83
mariusc83 marked this pull request as ready for review September 5, 2022 12:34
@mariusc83
mariusc83 requested a review from a team as a code owner September 5, 2022 12:34

@0xnm 0xnm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Overall lgtm, I left some questions to clarify the code.

Comment on lines +38 to +37
adds = adds,
removes = removes,
updates = updates

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nitpick: I'd rename those fields: additions and removals. A field should be a noun, and verbes should be used for functions

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this is part of the JSON schemas we agreed upon. If you don't mind we can add this as an extra task later.


// TODO: RUMM-2481 Use the `diff` method int the ShapeWireframe type when available
@Suppress("ComplexMethod")
private fun resolveUpdateMutation(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nitpick: Just for clarity, I'd rename that one resolveShapeUpdateMutation and the other one resolveTextUpdateMutation.

private fun MobileSegment.Wireframe.id(): Long {
return when (this) {
is MobileSegment.Wireframe.ShapeWireframe -> this.id
is MobileSegment.Wireframe.TextWireframe -> this.id
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wouldn't this one be better in the MobileSegmentExt.kt file?

writer.write(bundleRecordInEnrichedRecord(newRumContext, records))
}

private fun checkLastFullSnapshotTime(): Boolean {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nitpick: I'd rename that isLastFullSnapshotTime

val fakeAddedWireframes = forge.aList { forge.getForgery<MobileSegment.Wireframe>() }
val fakeCurrentSnapshot = fakePrevSnapshot.toMutableList() + fakeAddedWireframes
var lastPrevWireframe = fakePrevSnapshot.last()
val expectedAdds = LinkedList<MobileSegment.Add>()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nitpick: expectedAdditions

}
val fakeRemovedSize = forge.anInt(min = 1, max = fakePrevSnapshot.size)
val fakeCurrentSnapshot = fakePrevSnapshot.drop(fakeRemovedSize)
val expectedRemoves = fakePrevSnapshot

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nitpick: expectedRemovals

Comment on lines +76 to +82
while (stack.isNotEmpty()) {
val node = stack.pop()
expectedList.addAll(node!!.wireframes)
for (i in node.children.count() - 1 downTo 0) {
stack.push(node.children[i])
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here you're rewriting the prod algorithm, meaning that your test isn't really testing anything…
Another approach for this would be to start from the expected output: a list of nodes, and build a random tree that would give this output.
A pseudo algorithm would be:

fun generateTreeFromList(nodes : List<Node>) : Node {
    val maxDepth = 9 // from your current test
    val lastSiblings = Array<Node?>(size = maxDepth) { null }
    val tree = nodes.popFirst()
    while (nodes.isNotEmpty()) {
        // get random index of non null node from lastSiblings > 0
        val index = magic() // exercise left to reader
        val parent = lastSiblings[index - 1]
        val node = nodes.popFirst()
        parent.addChild(node)
        lastSiblings[index] = node
        for (i in (index + 1) until maxDepth) {
            lastSiblings[i] = null
        }
    }

    return tree
}

@mariusc83
mariusc83 requested review from 0xnm and xgouchet September 7, 2022 11:28
@mariusc83
mariusc83 force-pushed the mconstantin/rumm-2423/snapshot-processor-compute-mutations branch from e6ffd2a to e8bd782 Compare September 7, 2022 14:20
@mariusc83
mariusc83 merged commit e4a0aef into feature/session-replay-vo Sep 8, 2022
@mariusc83
mariusc83 deleted the mconstantin/rumm-2423/snapshot-processor-compute-mutations branch September 8, 2022 07:40
@xgouchet xgouchet added this to the internal milestone Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size-large This PR is large sized

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants