Upload module improvements #135
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…um process / [WIP] fixing retryCache
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -54,25 +63,46 @@ public class EmbraceUpload: EmbraceLogUploader { | |||
| /// Attempts to upload all the available cached data. | |||
| public func retryCachedData() { | |||
| queue.async { [weak self] in | |||
| guard let self = self else { return } | |||
There was a problem hiding this comment.
⚠️ Conditional statements should always return on the next line (conditional_returns_on_newline)
|
|
||
| // clear data from cache that shouldn't be retried as it's stale | ||
| self.clearCacheFromStaleData() | ||
|
|
There was a problem hiding this comment.
⚠️ Lines should not have trailing whitespace (trailing_whitespace)
| // retry for all other non-handled cases with errors | ||
| return error != nil | ||
| } | ||
|
|
There was a problem hiding this comment.
⚠️ Lines should not have trailing whitespace (trailing_whitespace)
| maxDelay: Double = 32.0 | ||
| ) { | ||
| self.baseDelay = baseDelay | ||
| self.maxDelay = max(maxDelay, baseDelay) // prevent somebody from adding a baseDelay larger than the maxDelay |
There was a problem hiding this comment.
⚠️ Line should be 120 characters or less; currently it has 121 characters (line_length)
| @@ -44,7 +44,7 @@ final class SessionControllerTests: XCTestCase { | |||
| ) | |||
|
|
|||
| self.queue = DispatchQueue(label: "com.test.embrace.upload.queue", attributes: .concurrent) | |||
| upload = try EmbraceUpload(options: uploadTestOptions, logger: MockLogger(), queue: queue) | |||
| upload = try EmbraceUpload(options: uploadTestOptions, logger: MockLogger(), queue: queue, semaphore: .init(value: .max)) | |||
There was a problem hiding this comment.
⚠️ Line should be 120 characters or less; currently it has 129 characters (line_length)
| @@ -729,7 +729,7 @@ | |||
| let storage = try EmbraceStorage.createInMemoryDb() | |||
| defer { try? storage.teardown() } | |||
|
|
|||
| let upload = try EmbraceUpload(options: uploadOptions, logger: logger, queue: queue) | |||
| let upload = try EmbraceUpload(options: uploadOptions, logger: logger, queue: queue, semaphore: .init(value: .max)) | |||
There was a problem hiding this comment.
⚠️ Line should be 120 characters or less; currently it has 123 characters (line_length)
| } | ||
|
|
There was a problem hiding this comment.
⚠️ Files should have a single trailing newline (trailing_newline)
| @@ -33,7 +33,7 @@ class EmbraceUploadTests: XCTestCase { | |||
| ) | |||
|
|
|||
| self.queue = DispatchQueue(label: "com.test.embrace.queue", attributes: .concurrent) | |||
| module = try EmbraceUpload(options: testOptions, logger: MockLogger(), queue: queue) | |||
| module = try EmbraceUpload(options: testOptions, logger: MockLogger(), queue: queue, semaphore: .init(value: .max)) | |||
There was a problem hiding this comment.
⚠️ Line should be 120 characters or less; currently it has 123 characters (line_length)
| EmbraceHTTPMock.mock(url: testSpansUrl()) | ||
| EmbraceHTTPMock.mock(url: testLogsUrl()) | ||
|
|
||
|
|
There was a problem hiding this comment.
⚠️ Limit vertical whitespace to a single empty line; currently 2 (vertical_whitespace)
| @@ -131,28 +151,39 @@ public struct MockResponse { | |||
| private(set) var data: Data? | |||
| private(set) var error: Error? | |||
| private(set) var statusCode: Int = -1 | |||
| private(set) var headers: [String: String]? = nil | |||
There was a problem hiding this comment.
⚠️ Initializing an optional variable with nil is redundant (redundant_optional_initialization)
Generated by 🚫 Danger Swift against afa7cc5 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #135 +/- ##
==========================================
- Coverage 91.94% 90.51% -1.44%
==========================================
Files 410 418 +8
Lines 26631 27768 +1137
==========================================
+ Hits 24487 25135 +648
- Misses 2144 2633 +489
|
NachoEmbrace
approved these changes
Dec 4, 2024
|
All warnings are from Tests. We should remove that :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This PR addresses some issues and also does some improvements on the way the
UploadModuleworks.Details
EmbraceUploadCacheto clean/purge it only based on date.ExponentialBackoffstruct).attemptCountto not be properly included in the headers of retried requests429,500-599) and errors. . Some requests (like the ones with statusCode400: Bad Requestor the ones with specific errors likeURLError.badURL) are going to be dropped.maximumAmountOfRetrieswas added toRedundancyOptionsto prevent requests from retrying indefinitely (which fixes a bug that would use an unnecessary amount of memory and data).Retry-Afterheader in combination with the Expontential Backoff logic.