Skip to content

RUM-2704: Add a request builder for resources#1827

Merged
jonathanmos merged 12 commits into
developfrom
jmoskovich/rum-2704/resource-request-builder
Feb 21, 2024
Merged

RUM-2704: Add a request builder for resources#1827
jonathanmos merged 12 commits into
developfrom
jmoskovich/rum-2704/resource-request-builder

Conversation

@jonathanmos

@jonathanmos jonathanmos commented Jan 16, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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)

  • 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)

@jonathanmos
jonathanmos force-pushed the jmoskovich/rum-2704/resource-request-builder branch 5 times, most recently from 39fd668 to 1cf9ca6 Compare January 17, 2024 09:39
@jonathanmos
jonathanmos marked this pull request as ready for review January 17, 2024 11:37
@jonathanmos
jonathanmos requested review from a team as code owners January 17, 2024 11:37
@codecov-commenter

codecov-commenter commented Jan 17, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1827 (23467e0) into develop (1e97c29) will decrease coverage by 0.32%.
Report is 51 commits behind head on develop.
The diff coverage is 64.50%.

❗ Current head 23467e0 differs from pull request most recent head cbd637a. Consider uploading reports for the commit cbd637a to get more accurate results

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
Files Coverage Δ
...tlin/com/datadog/android/api/net/RequestFactory.kt 100.00% <ø> (ø)
...dog/android/log/internal/net/LogsRequestFactory.kt 100.00% <ø> (ø)
...adog/android/rum/internal/net/RumRequestFactory.kt 97.92% <ø> (ø)
...oid/sessionreplay/internal/SessionReplayFeature.kt 99.03% <100.00%> (ø)
...onreplay/internal/net/SegmentRequestBodyFactory.kt 100.00% <100.00%> (ø)
...essionreplay/internal/net/SegmentRequestFactory.kt 100.00% <100.00%> (ø)
...android/trace/internal/net/TracesRequestFactory.kt 100.00% <ø> (ø)
...id/core/internal/data/upload/DataOkHttpUploader.kt 90.48% <0.00%> (-2.29%) ⬇️
...ndroid/sessionreplay/internal/net/ResourceEvent.kt 85.71% <85.71%> (ø)
...ssionreplay/internal/net/ResourceRequestFactory.kt 70.00% <70.00%> (ø)
... and 2 more

... and 29 files with indirect coverage changes

0xnm
0xnm reviewed Jan 17, 2024
View reviewed changes

@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.

I've reviewed production files (didn't review tests yet) and added few questions.

Comment thread .../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt Outdated
Comment thread .../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt Outdated
Comment thread .../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt Outdated
Comment thread .../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt Outdated
Comment thread .../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt Outdated
Comment thread ...src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/SessionReplayResource.kt Outdated
Comment thread .../src/main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestFactory.kt Outdated
Comment thread .../src/main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestFactory.kt Outdated
Comment thread .../src/main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestFactory.kt Outdated
Comment thread .../src/main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestFactory.kt Outdated
@jonathanmos
jonathanmos requested review from 0xnm and mariusc83 January 22, 2024 08:24
0xnm
0xnm requested changes Jan 22, 2024
View reviewed changes
Comment thread .../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt Outdated
Comment thread .../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt Outdated
Comment thread .../src/main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestFactory.kt Outdated
Comment thread ...t/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactoryTest.kt Outdated
Comment thread .../test/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestFactoryTest.kt Outdated
@jonathanmos
jonathanmos force-pushed the jmoskovich/rum-2704/resource-request-builder branch 3 times, most recently from 76f582d to ef5a186 Compare January 23, 2024 11:20
@jonathanmos
jonathanmos force-pushed the jmoskovich/rum-2704/resource-request-builder branch from ae3f64c to 9347900 Compare January 29, 2024 09:22
@jonathanmos
jonathanmos requested a review from 0xnm January 29, 2024 10:50
0xnm
0xnm reviewed Feb 5, 2024
View reviewed changes
Comment thread .../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt Outdated
Comment thread .../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt Outdated
Comment thread .../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt Outdated
Comment thread .../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt Outdated
Comment thread .../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt Outdated
Comment thread ...session-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/utils/MiscUtils.kt
Comment thread ...session-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/utils/MiscUtils.kt Outdated
Comment thread ...t/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactoryTest.kt Outdated
Comment thread ...t/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactoryTest.kt Outdated
Comment thread .../test/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestFactoryTest.kt Outdated
@jonathanmos
jonathanmos force-pushed the jmoskovich/rum-2704/resource-request-builder branch from 2d46084 to 416cb00 Compare February 6, 2024 13:06
@jonathanmos
jonathanmos requested a review from 0xnm February 6, 2024 15:47
0xnm
0xnm reviewed Feb 7, 2024
View reviewed changes

@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.

nice work, I left some final comments to address. Nothing blocking, but I think it is better to handle them before the merge.

Comment thread .../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt Outdated
Comment thread .../main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactory.kt Outdated
Comment thread .../src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/base64/ResourceEvent.kt Outdated
Comment thread ...eplay/src/test/kotlin/com/datadog/android/sessionreplay/forge/ResourceEventForgeryFactory.kt Outdated
Comment thread ...t/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestBodyFactoryTest.kt Outdated
Comment thread .../src/main/kotlin/com/datadog/android/sessionreplay/internal/domain/ResourceRequestFactory.kt Outdated
xgouchet
xgouchet suggested changes Feb 7, 2024
View reviewed changes

@xgouchet xgouchet left a comment

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.

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.

Comment thread ...src/main/kotlin/com/datadog/android/sessionreplay/internal/net/ResourceRequestBodyFactory.kt
Comment thread ...src/main/kotlin/com/datadog/android/sessionreplay/internal/net/ResourceRequestBodyFactory.kt Outdated
Comment thread ...src/main/kotlin/com/datadog/android/sessionreplay/internal/net/ResourceRequestBodyFactory.kt Outdated
Comment thread ...src/main/kotlin/com/datadog/android/sessionreplay/internal/net/ResourceRequestBodyFactory.kt Outdated
Comment thread ...src/main/kotlin/com/datadog/android/sessionreplay/internal/net/ResourceRequestBodyFactory.kt Outdated
Comment thread ...session-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/utils/MiscUtils.kt Outdated
@jonathanmos
jonathanmos force-pushed the jmoskovich/rum-2704/resource-request-builder branch 2 times, most recently from 39be041 to 67c70a6 Compare February 8, 2024 14:08
@jonathanmos
jonathanmos requested review from 0xnm and xgouchet February 8, 2024 15:09
0xnm
0xnm approved these changes Feb 9, 2024
View reviewed changes

@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.

lgtm!

Comment thread ...test/kotlin/com/datadog/android/sessionreplay/internal/net/ResourceRequestBodyFactoryTest.kt Outdated
xgouchet
xgouchet approved these changes Feb 13, 2024
View reviewed changes

@xgouchet xgouchet left a comment

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.

Nice work, just some nitpicking in the tests ;)

Comment thread ...-android-logs/src/test/kotlin/com/datadog/android/log/internal/net/LogsRequestFactoryTest.kt Outdated
Comment thread ...dk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/net/RumRequestFactoryTest.kt Outdated
@jonathanmos
jonathanmos requested review from 0xnm and xgouchet February 13, 2024 12:07
@jonathanmos
jonathanmos force-pushed the jmoskovich/rum-2704/resource-request-builder branch from 23467e0 to cbd637a Compare February 21, 2024 12:20
@jonathanmos
jonathanmos merged commit 3b933cf into develop Feb 21, 2024
@jonathanmos
jonathanmos deleted the jmoskovich/rum-2704/resource-request-builder branch February 21, 2024 12:43
@xgouchet xgouchet added this to the 2.7.0 milestone Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@0xnm 0xnm 0xnm approved these changes
@mariusc83 mariusc83 Awaiting requested review from mariusc83
+1 more reviewer
@xgouchet xgouchet xgouchet approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.7.x

Development

Successfully merging this pull request may close these issues.

5 participants

@jonathanmos @codecov-commenter @xgouchet @mariusc83 @0xnm