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

Limit batch size to 500kb #147

Merged
merged 10 commits into from
Jan 12, 2018
Merged

Limit batch size to 500kb #147

merged 10 commits into from
Jan 12, 2018

Conversation

rohitpaulk
Copy link
Collaborator

@rohitpaulk rohitpaulk commented Jan 9, 2018

#144

Includes changes from #143.

@rohitpaulk rohitpaulk changed the title [WIP] Limit batch size [WIP] Limit batch size to 500kb Jan 9, 2018
@codecov-io
Copy link

codecov-io commented Jan 9, 2018

Codecov Report

Merging #147 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #147      +/-   ##
==========================================
+ Coverage   98.76%   98.81%   +0.04%     
==========================================
  Files          11       11              
  Lines         405      422      +17     
==========================================
+ Hits          400      417      +17     
  Misses          5        5
Impacted Files Coverage Δ
lib/segment/analytics/defaults.rb 100% <100%> (ø) ⬆️
lib/segment/analytics/worker.rb 100% <100%> (ø) ⬆️
lib/segment/analytics/message.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 204d5a2...ffbe680. Read the comment docs.

Since builds run in parallel, the e2e tests sometimes fail
because their runscope messages are pushed down by other builds.

We currently test against 7 ruby versions, and a maximum of 2 builds are
triggered per PR (push + PR build). Checking 20 items should be
sufficient to handle this workload.
The assertion in the thread wouldn't trigger if the test
exited first.
@rohitpaulk
Copy link
Collaborator Author

Tested with the following script:

require 'segment/analytics'

analytics = Segment::Analytics.new({write_key: 'testkey'})

large_message = {
  properties: {a: 'b' * 30000},
  user_id: 'abcd',
  event: 'abcd'
}

50.times { analytics.track(large_message) }
analytics.flush

(Run locally using bundle exec ruby test_script.rb)

On master, I get this error:

E, [2018-01-10T14:28:09.443559 #15243] ERROR -- Segment::Analytics: 784: unexpected token at 'Bad Request                                                                         '
E, [2018-01-10T14:28:09.443652 #15243] ERROR -- Segment::Analytics: /Users/rohitpaulk/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/json-1.8.6/lib/json/common.rb:155:in `parse' E, [2018-01-10T14:28:09.443709 #15243] ERROR -- Segment::Analytics: /Users/rohitpaulk/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/json-1.8.6/lib/json/common.rb:155:in `parse'
E, [2018-01-10T14:28:09.443805 #15243] ERROR -- Segment::Analytics: /Users/rohitpaulk/experiments/productions/segment/analytics-ruby/lib/segment/analytics/request.rb:43:in `block in post'
E, [2018-01-10T14:28:09.443846 #15243] ERROR -- Segment::Analytics: /Users/rohitpaulk/experiments/productions/segment/analytics-ruby/lib/segment/analytics/request.rb:85:in `retry_with_backoff'
E, [2018-01-10T14:28:09.443872 #15243] ERROR -- Segment::Analytics: /Users/rohitpaulk/experiments/productions/segment/analytics-ruby/lib/segment/analytics/request.rb:94:in `retry_with_backoff'
E, [2018-01-10T14:28:09.443894 #15243] ERROR -- Segment::Analytics: /Users/rohitpaulk/experiments/productions/segment/analytics-ruby/lib/segment/analytics/request.rb:94:in `retry_with_backoff'
E, [2018-01-10T14:28:09.443916 #15243] ERROR -- Segment::Analytics: /Users/rohitpaulk/experiments/productions/segment/analytics-ruby/lib/segment/analytics/request.rb:94:in `retry_with_backoff'
E, [2018-01-10T14:28:09.443937 #15243] ERROR -- Segment::Analytics: /Users/rohitpaulk/experiments/productions/segment/analytics-ruby/lib/segment/analytics/request.rb:94:in `retry_with_backoff'
E, [2018-01-10T14:28:09.443958 #15243] ERROR -- Segment::Analytics: /Users/rohitpaulk/experiments/productions/segment/analytics-ruby/lib/segment/analytics/request.rb:94:in `retry_with_backoff'
E, [2018-01-10T14:28:09.443980 #15243] ERROR -- Segment::Analytics: /Users/rohitpaulk/experiments/productions/segment/analytics-ruby/lib/segment/analytics/request.rb:94:in `retry_with_backoff'
E, [2018-01-10T14:28:09.444001 #15243] ERROR -- Segment::Analytics: /Users/rohitpaulk/experiments/productions/segment/analytics-ruby/lib/segment/analytics/request.rb:94:in `retry_with_backoff'
E, [2018-01-10T14:28:09.444021 #15243] ERROR -- Segment::Analytics: /Users/rohitpaulk/experiments/productions/segment/analytics-ruby/lib/segment/analytics/request.rb:94:in `retry_with_backoff'
E, [2018-01-10T14:28:09.444042 #15243] ERROR -- Segment::Analytics: /Users/rohitpaulk/experiments/productions/segment/analytics-ruby/lib/segment/analytics/request.rb:94:in `retry_with_backoff'
E, [2018-01-10T14:28:09.444062 #15243] ERROR -- Segment::Analytics: /Users/rohitpaulk/experiments/productions/segment/analytics-ruby/lib/segment/analytics/request.rb:41:in `post'E, [2018-01-10T14:28:09.444082 #15243] ERROR -- Segment::Analytics: /Users/rohitpaulk/experiments/productions/segment/analytics-ruby/lib/segment/analytics/worker.rb:45:in `run'
E, [2018-01-10T14:28:09.444102 #15243] ERROR -- Segment::Analytics: /Users/rohitpaulk/experiments/productions/segment/analytics-ruby/lib/segment/analytics/client.rb:371:in `block
 (2 levels) in ensure_worker_running'

On this branch, the code executes successfully with no error. With some debug logs, here's how the result looks:

Batch count: 2
Batch count: 16
Batch count: 16
Batch count: 16
bundle exec ruby test_script.rb  0.71s user 0.27s system 7% cpu 13.075 total

@rohitpaulk
Copy link
Collaborator Author

Ready for review. Please merge #143 first though, this diff includes changes from that PR.

@rohitpaulk rohitpaulk requested a review from f2prateek January 10, 2018 09:02
@rohitpaulk rohitpaulk changed the title [WIP] Limit batch size to 500kb Limit batch size to 500kb Jan 10, 2018
Copy link
Contributor

@f2prateek f2prateek left a comment

Choose a reason for hiding this comment

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

Nice!

@rohitpaulk rohitpaulk merged commit 04beda6 into master Jan 12, 2018
@rohitpaulk rohitpaulk deleted the limit-batch-size branch January 16, 2018 05:46
# 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