Add blocking socket mode and background sender #787
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vickenty
force-pushed
the
vickenty/timeout
branch
2 times, most recently
from
b0ff7cd
to
8cf0201
Compare
vickenty
changed the title
settimeout(0) is equivalent to setblocking(0), so need only one call. Neither UDP or nor unix datagram sockets block on connect, so handling timeout in get_socket is not necessary.
Align reported metrics with the go client.
vickenty
force-pushed
the
vickenty/timeout
branch
2 times, most recently
from
eefd108
to
a7f6b62
Compare
vickenty
force-pushed
the
vickenty/timeout
branch
from
a7f6b62
to
1ac3a88
Compare
Python2.7 does not allow other positional parameters after *expression.
vickenty
changed the title
vickenty
added
the
changelog/Added
therve
previously approved these changes
Aug 15, 2023
remeh
previously approved these changes
Aug 16, 2023
| self._xmit_packet(packet + '\n', False) | ||
| if self._queue is not None: | ||
| try: | ||
| self._queue.put(packet + '\n', self._queue_blocking, self._queue_timeout) |
There was a problem hiding this comment.
Nit: about the packet + '\n', it might be worth doing it directly at the only direct call to _send_to_server in the flush method:
self._send_to_server("\n".join(self._buffer) + '\n')
There was a problem hiding this comment.
_send_to_server is also used in a bunch of places indirectly via the _send alias when buffering is disabled.
There was a problem hiding this comment.
That's right, but it doesn't stop us from having the call to _send_to_server in the flush method to add the \n as shown above. No strong opinion here, feel free to discard 👍
| self._xmit_packet_with_telemetry(self._queue.get()) | ||
| self._queue.task_done() | ||
|
|
||
| def wait_for_pending(self): |
There was a problem hiding this comment.
Should we consider documenting the new need (when using the threaded sender) to use this method to make sure everything's flushed properly and no buffered metrics are lost on shutdown?
Co-authored-by: Rémy Mathieu <[email protected]>
Co-authored-by: Rémy Mathieu <[email protected]>
vickenty
force-pushed
the
vickenty/timeout
branch
from
5334be9
to
d2f4a60
Compare
vickenty
force-pushed
the
vickenty/timeout
branch
from
d2f4a60
to
5fdad16
Compare
remeh
approved these changes
Aug 17, 2023
therve
approved these changes
Aug 17, 2023
6 tasks
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Add options to improve dogstatsd packet deliverability.
Description of the Change
Allow dogstatsd socket to be used in a blocking mode with a timeout. This improves our chances to send the packet if the socket queue is full (e.g. after a burst in packets).
Add a background thread to handle the writes, with a queue to act as a buffer.
Together, this allows users to reduce dropped packets to zero while maintaining low latency for the user application, at the cost of higher cpu and memory usage.
Alternate Designs
Possible Drawbacks
Verification Process
Additional Notes
Release Notes
Review checklist (to be filled by reviewers)
changelog/label attached. If applicable it should have thebackward-incompatiblelabel attached.do-not-merge/label attached.kind/andseverity/labels attached at least.