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

Deliver each event in a separate request to avoid exceeding payload size limit #424

Closed
wants to merge 5 commits into from

Conversation

fractalwrench
Copy link
Contributor

@fractalwrench fractalwrench commented Oct 11, 2019

Goal

The payload size limit can be exceeded if multiple events are batched together into one request, meaning events are dropped by our API. This changeset removes the batching functionality and sends each event in a separate request to reduce the likelihood of this occurring.

Changeset

  • Sent each event in a separate request within sendAllReportsWithCompletion.
  • Logged the filename when a request succeeds/fails
  • Passed through 1 event at a time to BugsnagApiClient for errors (sessions are still batched together in multiple payloads)
  • Changed completion callback to send the filename rather than the count of reports sent

Tests

  • Stored multiple events on disk, then launched the notifier configured to point at a local endpoint and confirmed that each request was received individually.
  • Updated mazerunner scenario assertions where they assumed requests were batched

@codecov
Copy link

codecov bot commented Oct 11, 2019

Codecov Report

Merging #424 into next will decrease coverage by 0.22%.
The diff coverage is 18.6%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next     #424      +/-   ##
==========================================
- Coverage   52.79%   52.56%   -0.23%     
==========================================
  Files          66       66              
  Lines        7594     7590       -4     
==========================================
- Hits         4009     3990      -19     
- Misses       3585     3600      +15
Impacted Files Coverage Δ
...SCrash/Recording/Tools/BSG_KSCrashCallCompletion.m 100% <100%> (ø) ⬆️
Source/BugsnagErrorReportApiClient.m 86.66% <50%> (ø) ⬆️
...rce/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.m 48.16% <50%> (-1.84%) ⬇️
Source/BugsnagSink.m 12.08% <6.25%> (-7.27%) ⬇️
Source/BugsnagFileStore.m 66.19% <0%> (-3.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af8307d...ad224a7. Read the comment docs.

@fractalwrench fractalwrench changed the title Deliver each event in a separate request to avoid exceeding payload size limit [WIP] Deliver each event in a separate request to avoid exceeding payload size limit Oct 14, 2019
@fractalwrench fractalwrench changed the title [WIP] Deliver each event in a separate request to avoid exceeding payload size limit Deliver each event in a separate request to avoid exceeding payload size limit Oct 14, 2019
Copy link
Contributor

@kattrali kattrali left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this. This changeset handles many of the cases involved in report delivery, however, I can't merge it as-is since there are a few architectural problems to address.

  1. As far as I've been able to tell, reports that are cancelled via beforeSend/beforeNotify blocks are never deleted now, unless enough new reports are added that they are removed due to file count limits. It could mean hanging around forever.
  2. The naming of the functions and parameters around delivery are often not logically consistent. Some specify plurals where they are now used for single items, others specify "completion" where the block is called many times. In addition, someof the documentation for the blocks no longer match so its not clear where the error lies for future passersby.

Neither of these are hard problems, however it looks like a good candidate for taking a step back and making a more detailed plan for the changes. Rather than going through each change inline, I'll sketch out something to work with. The tests are an excellent scaffold for ensuring that the end result is right, with some additions to ensure that cancelled files are deleted.

@kattrali kattrali changed the base branch from next to master November 4, 2019 15:50
@fractalwrench
Copy link
Contributor Author

Closing as stale.

@fractalwrench fractalwrench deleted the remove-event-batching branch May 22, 2020 09:18
@fractalwrench fractalwrench restored the remove-event-batching branch June 3, 2020 08:55
@fractalwrench fractalwrench deleted the remove-event-batching branch June 23, 2020 10:33
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.

2 participants