Skip to content
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

Merged
merged 12 commits into from
Feb 21, 2024

Conversation

jonathanmos
Copy link
Member

@jonathanmos jonathanmos commented Jan 16, 2024

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
Copy link

codecov-commenter commented Jan 17, 2024

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

Copy link
Member

@0xnm 0xnm left a 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.

@jonathanmos jonathanmos requested review from mariusc83 and 0xnm January 22, 2024 08:24
@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
@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
Copy link
Member

@0xnm 0xnm left a 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.

Copy link
Member

@xgouchet xgouchet left a 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.

@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
Copy link
Member

@0xnm 0xnm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

Copy link
Member

@xgouchet xgouchet left a 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 ;)

@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
23 checks passed
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants