make batch_size count bytes, not messages

[ccutrer]

otherwise it's impossible to properly fill a UDP packet, without risking going over, due to varying lengths of messages

[raggi]

The underlying proposal herein is certainly a useful change.

I have a few concerns as proposed, which prevent me from immediately merging:

1. Altering the public API in an incompatible way is not a good idea. It would be better to introduce a batch_size_bytes for this, and add some logic that appropriately coalesces the two values (for example, send on whichever comes first).
2. The chosen size assuming ethernet seems reasonable, but I'm not sure how much value there is in worrying about fragmentation as a boundary. ipv4 will carry 64k without a problem on a reasonably reliable network, and most networks on which statsd is deployed are reasonably reliable.
3. I don't think the method of backlog storage is ideal, that is the change to a string. I think we'd be better off keeping a count.
4. The introduction of those dups is a separate concern from this change and deserves a separate discussion. For almost all users this is not necessary.

[ccutrer]

1. Yeah, I wasn't sure what to do there. Maintaining two different batch size params and attempting to choose between them seems bug-prone and surprising to users. So I just left it using the same param, and assumed you would do a major version bump to point out the break API change. I can do whatever you would prefer, though.
2. It seems far preferable to me to allow someone with a reliable network to increase the size, rather than default to something that will possibly break (mostly silently) for anyone that doesn't fit your ideal world.
3. Sure, no problem, I can change that.
4. The dup is only in spec code, not code that end users will run. And it will go away by switching back to keeping an array of backlogged messages that is combined/serialized at send time, rather than passing the actual backlog through to the fake socket, which is then cleared by the Batch object.

[ccutrer]

I've pushed up an amended commit that addresses 3 and 4.

[raggi]

Thank you :)

re.

1. I should think if @backlog.size > @batch_size || @backlog_bytes > @batch_bytes_size should be a fairly easy to maintain condition pair. I think I'd like for us to go this route, as it saves any user surprises. The other alternative is to introduce a separate batch class, but I don't think we're at sufficient complexity to warrant that yet. We can pick an arbitrarily large value for the default @batch_bytes_size.

2. 👏 🉐 I parry that if we're not considering ideal worlds then we should do as DNS does, and pick 512 bytes, making space for swolen ip headers. ⚔️

I don't think we'll reach a perfect answer for 2, but let me know what you think.

[ccutrer]

1. okay, I went that way, but allowing you to set one or the other limiting params to nil to ignore it.
2. I cleverly sidestep your parry by leaving the old defaults intact - 10 messages, and byte size batching disabled :)

[raggi]

Thank you for your thoughtful responses, patience and contribution!

[raggi]

Released, too!