-
Notifications
You must be signed in to change notification settings - Fork 5
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
fix: batch event payloads into chunks of 100 to reduce errors on event processing #193
Conversation
would be good to add some tests for this |
1580127
to
62bd86c
Compare
3e883f3
to
87833aa
Compare
87833aa
to
ba86750
Compare
f3f8605
to
dfb60ed
Compare
dfb60ed
to
3f1dc8d
Compare
abfa668
to
f5ada6a
Compare
f5ada6a
to
3acb4ff
Compare
let requestBody: [String: Any] = [ | ||
"events": batchEvents, | ||
"user": userBody | ||
] | ||
|
||
let jsonBody = try? JSONSerialization.data(withJSONObject: requestBody, options: .prettyPrinted) | ||
Log.debug("Post Events Payload: \(String(data: jsonBody!, encoding: .utf8) ?? "")") | ||
eventsRequest.httpBody = jsonBody | ||
|
||
self.makeRequest(request: eventsRequest) { data, response, error in | ||
if error != nil || data == nil { | ||
return completion((data, response, error)) | ||
} | ||
// Continue with next batch | ||
startIndex = endIndex | ||
endIndex = min(endIndex + self.maxBatchSize, totalEventsCount) | ||
|
||
if startIndex >= totalEventsCount { | ||
return completion((data, response, nil)) | ||
} | ||
} |
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.
You could move this request code to its own method to clean this up a bit.
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.
updated to split the request generation into its own private function
let data = "{\"config\":\"key}".data(using: .utf8) | ||
let config = processConfig(data) | ||
XCTAssertNil(config) | ||
} | ||
|
||
func testFlushingEvents() { |
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.
Can you make sure there is a test that if there are less than 100 events it still gets eventually flushed.
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.
yup, this test is only generating 20 events :) the next one batches them with 205 events
macos-13-large