Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Log errors for messages that exceed the maximum size #143

Merged
merged 4 commits into from
Jan 11, 2018

Conversation

rohitpaulk
Copy link
Collaborator

@rohitpaulk rohitpaulk commented Jan 5, 2018

@rohitpaulk rohitpaulk changed the title [WIP] Log out errors for messages that exceed the maximum size [WIP] Log errors for messages that exceed the maximum size Jan 5, 2018
@codecov-io
Copy link

codecov-io commented Jan 5, 2018

Codecov Report

Merging #143 into master will increase coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #143      +/-   ##
=========================================
+ Coverage   98.38%   98.5%   +0.11%     
=========================================
  Files           9      11       +2     
  Lines         371     400      +29     
=========================================
+ Hits          365     394      +29     
  Misses          6       6
Impacted Files Coverage Δ
lib/segment/analytics/message.rb 100% <100%> (ø)
lib/segment/analytics/worker.rb 100% <100%> (ø) ⬆️
lib/segment/analytics/defaults.rb 100% <100%> (ø) ⬆️
lib/segment/analytics/message_batch.rb 100% <100%> (ø)

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 779cc77...874c417. Read the comment docs.

@rohitpaulk rohitpaulk changed the title [WIP] Log errors for messages that exceed the maximum size Log errors for messages that exceed the maximum size Jan 6, 2018
@rohitpaulk
Copy link
Collaborator Author

Tested this out in the console:

screen shot 2018-01-06 at 2 56 23 pm

@rohitpaulk
Copy link
Collaborator Author

Note: This doesn't handle the case where a whole batch exceeds the allowed batch size:

screen shot 2018-01-06 at 2 57 55 pm

The error logged out in that case is pretty ugly. This is an error the user shouldn't have to know about - as it could be entirely handled by the library. We could split one batch into multiple that each fall under the limited size. I'll handle this in a separate PR.

@rohitpaulk
Copy link
Collaborator Author

Ready for review.

@rohitpaulk rohitpaulk requested a review from f2prateek January 6, 2018 10:47
@f2prateek f2prateek merged commit 42b2861 into master Jan 11, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants