-
Notifications
You must be signed in to change notification settings - Fork 57
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.
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
RUMM-2356 Add session replay recorder basic logic #1007
RUMM-2356 Add session replay recorder basic logic #1007
Conversation
504bb2c
to
a18cfe5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good, I can easily understand what's happening in there 👍. Left one arch suggestion, but non-blocking.
import com.datadog.android.sessionreplay.model.MobileSegment | ||
|
||
internal data class Node( | ||
val id: Long, | ||
val wireframes: List<MobileSegment.Wireframe>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: I think it might be healthier to not depend directly on (HTTP) transport details in the core SR logic (the MobileSegment.Wireframe
is what we serialize and send in payload). Introducing intermediate (DTO) and doing Node
→ indermediate → Wireframe
transformation could IMO help on a long run by clearly separating concerns. Otherwise, the Recorder code will have to be changed if transport details change, which seems to be against SoC as both layers clearly overlap here. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I thought about it but then I think I prefer it this way even though maybe at some point I need to change the wireframes a bit. Main reason why is:
- I don't want to introduce another layer which will require a transformer. If there's any change on the Wireframe I will still have to apply it at the transformer level from the intermediate DTO to Wireframe
- Most important I want to avoid creating too many objects. With this approach the wireframes instances created here will be re - used at the Processor level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very clear 👌, was just checking what's the reasoning behind it 👍. I'll perhaps introduce a layer in between on iOS (have my reasons, mainly around threading), but I think it might be iOS-specific.
Codecov Report
@@ Coverage Diff @@
## feature/session-replay-vo #1007 +/- ##
=============================================================
+ Coverage 82.99% 83.03% +0.04%
=============================================================
Files 307 314 +7
Lines 9980 10129 +149
Branches 1628 1669 +41
=============================================================
+ Hits 8282 8410 +128
- Misses 1193 1195 +2
- Partials 505 524 +19
|
49fb615
to
8a5db0e
Compare
a18cfe5
to
c5bbbb6
Compare
c5bbbb6
to
d9065f3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No docs impact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the SnapshotProducer
class seems quite complex as you have many distinct usecases (TextView, ImageView, ButtonView, …).
I think it might be better to have a WireframeAdapter
interface (or converter, or whatever name suits best), looking like
interface WireframeAdapter <T: View> {
fun adapt(input: T): Node
}
and then have independent implementations for each type we want to support.
And then, if you really need shared extension functions, then place them in a separate file with documentation
...android-session-replay/src/main/kotlin/com/datadog/android/sessionreplay/recorder/LongExt.kt
Show resolved
Hide resolved
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES | ||
.LOLLIPOP | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit pick but the formatting here seems really weird
} | ||
|
||
@Suppress("ComplexMethod") | ||
private fun View.toNode(pixelsDensity: Float): Node? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is an extension method really relevant here? This would be equally efficient as a fun convertViewToNode(view: View, pixelsDensity: Float): Node?
and less ambiguous.
|
||
@Suppress("ComplexMethod") | ||
private fun View.toNode(pixelsDensity: Float): Node? { | ||
if (isNotValid()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, using extension makes things ambiguous here. Does isNotValid
refer to the implicit this: View
, or to the parent this: SnapshotProducer
?
return null | ||
} | ||
|
||
if (isSystemNoise()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
return null | ||
} | ||
|
||
if (isToolbar()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
return toNode(childNodes, pixelsDensity) | ||
} | ||
|
||
private fun View.toNode(childNodes: LinkedList<Node>, pixelsDensity: Float) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid using block function when the implementation is this long, as it hides the return type and make things harder to read
private fun Drawable.resolveShapeStyleAndBorder(): Pair<MobileSegment | ||
.ShapeStyle?, MobileSegment.ShapeBorder?>? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we pick a consistent name for all those conversion functions? You have at least three in this class: toSomething
, asSomething
and resolveSomething
.
I do agree that I might not need all these extension functions, will try to get rid of at least part of them. |
d9065f3
to
29b0cdd
Compare
) | ||
} | ||
|
||
private fun View.isNotValid(): Boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does valid
means here? Do we mean trackable, visible on screen, …?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will change it to trackable
// skip adding the children and just take a screenshot of the toolbar. | ||
// It is too complex to de - structure this in multiple wireframes | ||
// and we cannot actually get all the details here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a TODO/Task in the backlog to handle this in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will already be handled in v1 once we introduce the logic in the screenshotWireframeMapper
WireframeMapper<T, S> { | ||
|
||
protected fun resolveViewId(view: View): Long { | ||
return if (view.id != View.NO_ID) view.id.toLong() else view.hashCode().toLong() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't you afraid that there would be collision here? What would be the consequences if collisions happened?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm you are right...it might be a problem due the the view.hashcode
maybe I should use the reference address there instead ? What do you think ? It can create problems if it happens as it might override other view properties in the mutation computation.
import android.widget.TextView | ||
import com.datadog.android.sessionreplay.model.MobileSegment | ||
|
||
internal class RecorderWireframeMapper( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this called RecorderWireframeMapper
? The naming doesn't seem consistent with the TextWireframeMapper
or ButtonWireframeMapper
.
It seems to map Views to Wireframe, so what's the difference between this class and the ViewWireframeMapper
? Same question goes for the ScreenshotWireframeMapper
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes...I think I need to find better names for these 🤔
25061da
to
c1c8cba
Compare
What does this PR do?
This PR introduces the basic logic in our
SnapshotProducer
as a part of theScreenRecorder
component where we are actually going to capture the application screens and resolve them asNode
tree equivalents.Please note that for now the
SnapshotProducer
is quite dummy and can only handle basic application screens mostly from our Android Sample App. The way we will work on this component is through multiple iterations by running it with different sample applications and improving the output.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)