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

Add Sender.queue_size limits #230

Merged
merged 10 commits into from
Feb 1, 2022
Merged

Add Sender.queue_size limits #230

merged 10 commits into from
Feb 1, 2022

Conversation

djmitche
Copy link
Contributor

@djmitche djmitche commented Dec 3, 2021

This adds a queue_size limit to the Sender class, with messages beyond that limit dropped. We support Ruby-2.1.0, which doesn't have a SizedQueue that supports "attempted" sends (it always blocks the sender if the queue is full), so we need to limit the queue size out-of-band. Also, we want to limit the size based on bytes, not message count. Because the @message_queue_bytesize is accessed concurrently in a read-modify-write pattern, it requires a mutex.

Things I'd like feedback on:

  • Ideas on other ways to do this that don't involve two mutex syncs on each message
  • The tests - is there something similar to Waiter already defined? Or another more rspec-y way to test this kind of concurrency-related behavior?
  • Logging on drops - this will log every drop, which is probably exactly the wrong thing to do. Other options:
    • Log only the first drop
    • Log edges (not dropping -> dropping / dropping -> not dropping)
    • Log the first and then every N messages dropped

This isn't quite done yet. Still to do:

  • Thread queue_size through Statsd constructor
  • More tests, at least covering resuming sending
  • Docs
  • Changelog

@djmitche djmitche requested a review from remeh December 3, 2021 19:16
@remeh
Copy link
Contributor

remeh commented Dec 23, 2021

Small feedback @djmitche

  • I don't think Ruby is supporting any sort of atomic integer whatsoever so I would probably have done the same...
  • Can't help here sorry
  • I think we want to use the internal telemetry of the client to report the amount of drops. However about the logs, that's a good question, we should check what's the behavior in our other implementations (i.e. datadog-go).

You have been with the byte size and not the messages count, I would probably have done the same, but it may make sense to validate it has been done the same way in other implementations and to discuss about it with the team otherwise.

@djmitche
Copy link
Contributor Author

datadog-go doesn't log anything, but does count it in datadog.dogstatsd.client.packets_dropped_queue. I'll do something similar in this client.

@djmitche
Copy link
Contributor Author

This is based on the Go client, which uses WithSenderQueueSize:

WithSenderQueueSize sets the size of the sender queue in number of buffers.

After data has been serialized in a buffer they're pushed to a queue that the sender will consume and then each one ot the agent.

The default value 0 will set the option to the optimal size for the transport protocol used: 2048 for UDP and named pipe and 512 for UDS.

So that should be easy to emulate with the ruby version.

@djmitche
Copy link
Contributor Author

Also, somehow I convinced myself that Ruby doesn't have a GIL. It does. So access to an integer is automatically serialized by that GIL.

@djmitche
Copy link
Contributor Author

I've asked for some advice on how best to test this. I don't think Waiter is a good idea.

@djmitche
Copy link
Contributor Author

djmitche commented Jan 4, 2022

Ivo has suggested using dependency-injection to inject a fake Queue implementation here, mocking out the background thread's behavior. I had tried mocking Thread, without success, so I'll try Queue instead.

@djmitche djmitche force-pushed the dustin.mitchell/ac-1321 branch 4 times, most recently from ce4faef to bab630f Compare January 6, 2022 16:34
@djmitche
Copy link
Contributor Author

djmitche commented Jan 6, 2022

Supporting those ancient Ruby versions is a real drag.

@djmitche
Copy link
Contributor Author

djmitche commented Jan 6, 2022

Hm, it seems webrick might have dropped support for ruby-2.0??

@djmitche djmitche closed this Jan 6, 2022
@djmitche djmitche reopened this Jan 6, 2022
@djmitche djmitche marked this pull request as ready for review January 6, 2022 17:12
@djmitche
Copy link
Contributor Author

djmitche commented Jan 6, 2022

I think the Ruby-2.0 failures are unrelated to this, so maybe we can handle that with #229 ?

@djmitche
Copy link
Contributor Author

Merged from main -- looks like I changed all the same places as @abicky :)

Copy link
Contributor

@remeh remeh left a comment

Choose a reason for hiding this comment

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

Nice!

lib/datadog/statsd/sender.rb Outdated Show resolved Hide resolved
spec/statsd/telemetry_spec.rb Outdated Show resolved Hide resolved
@djmitche djmitche merged commit 83c0fbc into master Feb 1, 2022
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