-
Notifications
You must be signed in to change notification settings - Fork 113
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-2154 Enable resource endpoint #1592
RUM-2154 Enable resource endpoint #1592
Conversation
To be rebased on develop after #1585 is merged and prod backend is ready |
cb1d464
to
7fb41f7
Compare
Datadog ReportBranch report: ❌ 1 Failed (0 Known Flaky), 2722 Passed, 0 Skipped, 6m 49.62s Wall Time ❌ Failed Tests (1)
🔻 Code Coverage Decreases vs Default Branch (8)
|
da778d5
to
16e5208
Compare
Awaiting for backend to be deployed. |
4f97709
to
ba44d2f
Compare
DatadogSessionReplay/Sources/Recorder/Utilities/UIImage+SessionReplay.swift
Outdated
Show resolved
Hide resolved
DatadogSessionReplay/Sources/Processor/ResourcesProcessor.swift
Outdated
Show resolved
Hide resolved
DatadogSessionReplay/Sources/Recorder/Utilities/UIImage+SessionReplay.swift
Show resolved
Hide resolved
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 haven't looked SR product changes, but modifications in sr-snapshots
CLI and SRSnapshotTests
project look fine with one remark 👌. Glad we extended it with the support to SR Resources - nice 💪.
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 good 👍 I have left a couple of blocking points
DatadogSessionReplay/Sources/Processor/ResourcesProcessor.swift
Outdated
Show resolved
Hide resolved
DatadogSessionReplay/Sources/Processor/ResourcesProcessor.swift
Outdated
Show resolved
Hide resolved
DatadogSessionReplay/Sources/Recorder/Utilities/UIImage+SessionReplay.swift
Outdated
Show resolved
Hide resolved
DatadogSessionReplay/Sources/Recorder/Utilities/UIImage+SessionReplay.swift
Outdated
Show resolved
Hide resolved
...urces/Recorder/ViewTreeSnapshotProducer/ViewTreeSnapshot/NodeRecorders/UIImageResource.swift
Outdated
Show resolved
Hide resolved
...urces/Recorder/ViewTreeSnapshotProducer/ViewTreeSnapshot/NodeRecorders/UIImageResource.swift
Outdated
Show resolved
Hide resolved
Thanks for the review @maxep I think I managed to resolve all the comments. Also simplified threading by reusing the same queue in both processors. |
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 ok 👌, I left suggestions on adding tests coverage
DatadogSessionReplay/Sources/Recorder/Utilities/UIImage+SessionReplay.swift
Show resolved
Hide resolved
...urces/Recorder/ViewTreeSnapshotProducer/ViewTreeSnapshot/NodeRecorders/UIImageResource.swift
Show resolved
Hide resolved
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 great 👍
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 have this PR properly described, not leaving blank PR template? We won't know what was the scope of this change.
What and why?
Enables resource recording using resource endpoint and deprecates and disables base64 solution.
Along with this, we conducted some E2E tests and fixed some minor bugs that we spotted.
Also, there were a few updates to snapshot tests, that cover new ways of getting and rendering image data.
How?
Code was prepared in past PRs. It's mostly achieved by injecting
ResourceProcessor
.Review checklist
Custom CI job configuration (optional)
tools/