Set buffer size in bytes #86
Conversation
lib/datadog/statsd.rb
Outdated
| @@ -93,16 +93,19 @@ def self.VERSION | |||
| # @param [Integer] port your statsd port | |||
| # @option opts [String] :namespace set a namespace to be prepended to every metric name | |||
| # @option opts [Array<String>] :tags tags to be added to every metric | |||
| # @option opts [Integer] :max_buffer_size max messages to buffer | |||
| # @option opts [Integer] :max_buffer_bytes max messages to buffer | |||
There was a problem hiding this comment.
this option was never released, so it should be fine to change it
users that used the additional argument before will get a "ArgumentError" and then have to read the docs on how to upgrade
lib/datadog/statsd.rb
Outdated
| @buffer = Array.new | ||
| return if @buffer_bytes.zero? | ||
| send_to_socket(@buffer) | ||
| @buffer = String.new |
There was a problem hiding this comment.
not using "" since that breaks the tests on ruby 2.3+ where we use frozen strings
|
after that the only missing things is thread-safety, that can be another minor release or just added to 4.0 |
|
thread safe batching will need some rework to not be a big mess ... so prefer getting 4.0 out ... |
| @@ -432,17 +434,28 @@ def send_stats(stat, delta, type, opts=EMPTY_OPTIONS) | |||
|
|
|||
| def send_stat(message) | |||
| if @batch_nesting_depth > 0 | |||
| message_bytes = message.bytesize | |||
There was a problem hiding this comment.
I think this leaves open the (unlikely) possibility of message_bytes being greater than @max_buffer_size.
What do you think about raising an error if that ever happens?
if message_bytes >= @max_buffer_bytes
raise ArgumentError.new("Expected message bytesize to be under #{@max_buffer_bytes} but was #{message_size}")
end
lib/datadog/statsd.rb
Outdated
| @buffer = Array.new | ||
| @max_buffer_size = opts[:max_buffer_size] || 50 | ||
| # batching | ||
| @max_buffer_bytes = opts[:max_buffer_bytes] || 8192 |
There was a problem hiding this comment.
What are your thoughts on warning somebody when they set max_buffer_bytes greater than this default?
lib/datadog/statsd.rb
Outdated
| @@ -432,17 +434,28 @@ def send_stats(stat, delta, type, opts=EMPTY_OPTIONS) | |||
|
|
|||
| def send_stat(message) | |||
| if @batch_nesting_depth > 0 | |||
| message_bytes = message.bytesize | |||
| unless @buffer_bytes.zero? | |||
There was a problem hiding this comment.
IMO if @buffer_bytes != 0 would be much clearer. Admittedly however, this is subjective.
|
kinda unlikely and means 1 more check for everyone else, so would prefer
not to check
…On Tue, Jun 5, 2018 at 12:19 PM Syed Humza Shah ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/datadog/statsd.rb
<#86 (comment)>:
> @@ -432,17 +434,28 @@ def send_stats(stat, delta, type, opts=EMPTY_OPTIONS)
def send_stat(message)
if @batch_nesting_depth > 0
+ message_bytes = message.bytesize
I think this leaves open the (unlikely) possibility of message_bytes
being greater than @max_buffer_size and us ending up with the same
problem. What do you think about raising an error if that ever happens?
if message_bytes >= @max_buffer_bytes
raise ArgumentError.new("Message size is too large.")end
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#86 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAsZ3aDE1L5jBUW4CEamamlPpXIpSkaks5t5tmmgaJpZM4UNGAi>
.
|
|
I think the agent can be configured to accept higher values, so that should
be fine.
A warning for something that might be a valid usecase also means we must
allow users to opt-out of the warning,
if someone sets this option I'd trust them to know what they are doing.
…On Tue, Jun 5, 2018 at 12:22 PM Syed Humza Shah ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/datadog/statsd.rb
<#86 (comment)>:
> @@ -108,8 +108,10 @@ def initialize(host = DEFAULT_HOST, port = DEFAULT_PORT, opts = EMPTY_OPTIONS)
raise ArgumentError, 'tags must be a Array<String>' unless tags.nil? or tags.is_a? Array
@tags = (tags || []).compact.map! {|tag| escape_tag_content(tag)}
- @buffer = Array.new
- @max_buffer_size = opts[:max_buffer_size] || 50
+ # batching
+ @max_buffer_bytes = opts[:max_buffer_bytes] || 8192
What are your thoughts on warning somebody when they set max_buffer_bytes
greater than this default?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#86 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAsZ1vK9okU_F7Sm3motAgSx2GyqL-cks5t5tpdgaJpZM4UNGAi>
.
|
grosser
force-pushed
the
grosser/bytes
branch
2 times, most recently
from
0579178
to
c534ec7
Compare
|
needs #93 to be merged first |
Default buffer size matches dd-agent read buffer
|
@gmmeyer rebased and green 🎉 |
|
I agree with your assessment @grosser that the raise wouldn't be necessary for this, that is configurable on the agent size |
|
maybe good enough for a 4.0 release ... |
Default buffer size matches dd-agent read buffer
@Antti @jonmoter
as replacement for #82 and #65