-
Notifications
You must be signed in to change notification settings - Fork 133
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
RUM-2816 Session Replay Browser Record Receiver #1720
RUM-2816 Session Replay Browser Record Receiver #1720
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 2880 Passed, 0 Skipped, 32m 50.65s Wall Time 🔻 Code Coverage Decreases vs Default Branch (8)
|
2ae4f21
to
08e8f24
Compare
d95ca54
to
594b1e9
Compare
594b1e9
to
0242681
Compare
if let viewID = self.viewCache.lastView(before: date, hasReplay: true) { | ||
if let viewID = self.viewCache.lastView(before: correctedDate, hasReplay: true) { |
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.
We should actually use the corrected time so the browser view is associated to the right native view.
private enum Constants { | ||
enum Constants { |
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.
for accessibility in tests.
internal struct RUMContext: Decodable, Equatable { | ||
internal struct RUMContext: Codable, Equatable { |
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.
Adding Encodable
for tests.
static let totalWireframeMutationRecords = 7 | ||
static let totalWireframeMutationRecords = 5 |
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.
Reflecting removal of the WKWebView
from the unsupported views.
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.
LGTM
@@ -40,18 +40,6 @@ internal final class WebViewEventReceiver: FeatureMessageReceiver { | |||
return false | |||
} | |||
|
|||
write(event: event, to: core) |
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 can see, that change joins two functions. Does it make sense to keep it broken down to two functions? Just for the readability sake.
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.
Lets keep it as one for now, we can revisit if it grow in complexity!
return offset | ||
} | ||
offsets.insert((id, offset), at: 0) | ||
// only retain 3 offsets |
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 3 only?
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.
idk.. this is from the original imp of RUM web-view integration #683
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.
Well done 👌. Glad to see integrat-unit added 🚀
What and why?
Forward Browser records to Session Replay pipeline.
How?
The
WebViewRecordReceiver
will get Browser records from the message-bus, apply the NTP offset of the container RUM view and write it to Session Replay storage.Review checklist
Custom CI job configuration (optional)
tools/