-
Notifications
You must be signed in to change notification settings - Fork 64
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-2704: Add a request builder for resources #1827
RUM-2704: Add a request builder for resources #1827
Conversation
39fd668
to
1cf9ca6
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #1827 +/- ##
===========================================
- Coverage 83.53% 83.21% -0.32%
===========================================
Files 467 470 +3
Lines 16468 16627 +159
Branches 2482 2497 +15
===========================================
+ Hits 13756 13836 +80
- Misses 2034 2105 +71
- Partials 678 686 +8
|
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've reviewed production files (didn't review tests yet) and added few questions.
.../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt
Outdated
Show resolved
Hide resolved
.../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt
Outdated
Show resolved
Hide resolved
.../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt
Outdated
Show resolved
Hide resolved
.../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt
Outdated
Show resolved
Hide resolved
.../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt
Outdated
Show resolved
Hide resolved
...src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/SessionReplayResource.kt
Outdated
Show resolved
Hide resolved
.../src/main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestFactory.kt
Outdated
Show resolved
Hide resolved
.../src/main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestFactory.kt
Outdated
Show resolved
Hide resolved
.../src/main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestFactory.kt
Outdated
Show resolved
Hide resolved
.../src/main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestFactory.kt
Outdated
Show resolved
Hide resolved
...src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/SessionReplayResource.kt
Outdated
Show resolved
Hide resolved
.../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt
Outdated
Show resolved
Hide resolved
.../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt
Outdated
Show resolved
Hide resolved
.../src/main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestFactory.kt
Outdated
Show resolved
Hide resolved
...t/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactoryTest.kt
Outdated
Show resolved
Hide resolved
.../test/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestFactoryTest.kt
Outdated
Show resolved
Hide resolved
76f582d
to
ef5a186
Compare
.../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt
Outdated
Show resolved
Hide resolved
ae3f64c
to
9347900
Compare
.../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt
Outdated
Show resolved
Hide resolved
.../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt
Outdated
Show resolved
Hide resolved
.../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt
Outdated
Show resolved
Hide resolved
.../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt
Outdated
Show resolved
Hide resolved
.../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt
Outdated
Show resolved
Hide resolved
...session-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/utils/MiscUtils.kt
Show resolved
Hide resolved
...session-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/utils/MiscUtils.kt
Outdated
Show resolved
Hide resolved
...t/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactoryTest.kt
Outdated
Show resolved
Hide resolved
...t/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactoryTest.kt
Outdated
Show resolved
Hide resolved
.../test/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestFactoryTest.kt
Outdated
Show resolved
Hide resolved
2d46084
to
416cb00
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.
nice work, I left some final comments to address. Nothing blocking, but I think it is better to handle them before the merge.
.../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt
Outdated
Show resolved
Hide resolved
.../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt
Outdated
Show resolved
Hide resolved
.../src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/base64/ResourceEvent.kt
Outdated
Show resolved
Hide resolved
...eplay/src/test/kotlin/com/datadog/android/sessionreplay/forge/ResourceEventForgeryFactory.kt
Outdated
Show resolved
Hide resolved
...t/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactoryTest.kt
Outdated
Show resolved
Hide resolved
.../src/main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestFactory.kt
Outdated
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.
Please let's make sure we don't overuse the @Supress
annotation.
Those checks were put in place for a reason: making sure that we never crash the host application. We can use some of those but we need to make sure that it is always in a 100% safe way.
...src/main/kotlin/com/datadog/android/sessionreplay/internal/net/ResourceRequestBodyFactory.kt
Show resolved
Hide resolved
...src/main/kotlin/com/datadog/android/sessionreplay/internal/net/ResourceRequestBodyFactory.kt
Outdated
Show resolved
Hide resolved
...src/main/kotlin/com/datadog/android/sessionreplay/internal/net/ResourceRequestBodyFactory.kt
Outdated
Show resolved
Hide resolved
...src/main/kotlin/com/datadog/android/sessionreplay/internal/net/ResourceRequestBodyFactory.kt
Outdated
Show resolved
Hide resolved
...src/main/kotlin/com/datadog/android/sessionreplay/internal/net/ResourceRequestBodyFactory.kt
Outdated
Show resolved
Hide resolved
...session-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/utils/MiscUtils.kt
Outdated
Show resolved
Hide resolved
39be041
to
67c70a6
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.
lgtm!
...test/kotlin/com/datadog/android/sessionreplay/internal/net/ResourceRequestBodyFactoryTest.kt
Outdated
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.
Nice work, just some nitpicking in the tests ;)
...-android-logs/src/test/kotlin/com/datadog/android/log/internal/net/LogsRequestFactoryTest.kt
Outdated
Show resolved
Hide resolved
...dk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/net/RumRequestFactoryTest.kt
Outdated
Show resolved
Hide resolved
23467e0
to
cbd637a
Compare
What does this PR do?
Adds a request builder for resources, and renames the existing request builder factories to make it clear that they handle segments.
This is the first of several prs and at the moment is detached code.
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)