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-5564 increase retry delay on DNS error #2135

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

xgouchet
Copy link
Member

What does this PR do?

When a DNS issue prevents the SDK from reaching our intake hosts, we increase the delay to avoid an infinite loop.

Motivation

By default our retry loop can be quite frequent, but DNS issues often take some time to be fixed. Although an actual issue in our DNS registration is unlikely, there seems to also be some issues on some networks preventing the devices from resolving the intake host address.

Additional Notes

Relates to #2131

@xgouchet xgouchet requested review from a team as code owners July 23, 2024 14:32
internal class DNSErrorStatusForgeryFactory : ForgeryFactory<UploadStatus.DNSError> {

override fun getForgery(forge: Forge): UploadStatus.DNSError {
return UploadStatus.DNSError
Copy link
Member

@mariusc83 mariusc83 Jul 23, 2024

Choose a reason for hiding this comment

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

why do we need a factory here which only returns one single type of enum ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not an enum but an object as part of a sealed class

Copy link
Member

Choose a reason for hiding this comment

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

even so in this case this factory only returns one value of that UploadStatus sealed class. Wouldn't it be more simple to just add this value in your test data without using forge ?

Copy link
Member

@mariusc83 mariusc83 left a comment

Choose a reason for hiding this comment

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

Except for that Forge factory which I don't think is needed this LGTM

@codecov-commenter
Copy link

codecov-commenter commented Jul 24, 2024

Codecov Report

Attention: Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.01%. Comparing base (54f5f12) to head (a393004).
Report is 4 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2135      +/-   ##
===========================================
+ Coverage    70.00%   70.01%   +0.01%     
===========================================
  Files          726      726              
  Lines        26970    26996      +26     
  Branches      4521     4524       +3     
===========================================
+ Hits         18879    18901      +22     
- Misses        6817     6834      +17     
+ Partials      1274     1261      -13     
Files Coverage Δ
...id/core/internal/data/upload/DataUploadRunnable.kt 96.92% <100.00%> (+0.10%) ⬆️
.../android/core/internal/data/upload/UploadStatus.kt 98.36% <100.00%> (+0.15%) ⬆️
...id/core/internal/data/upload/DataOkHttpUploader.kt 91.11% <85.71%> (-0.46%) ⬇️

... and 32 files with indirect coverage changes

@xgouchet xgouchet merged commit d1a746b into develop Jul 24, 2024
19 checks passed
@xgouchet xgouchet deleted the xgouchet/RUM-5564/dns_error_infinite_loop branch July 24, 2024 12:00
@xgouchet xgouchet added this to the 2.12.x milestone Jul 31, 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.

4 participants