[dogstatsd] Handle EAGAIN socket error when dropping packets #515
Conversation
mrknmc
force-pushed
the
blocking-io-error-catch
branch
from
75a4278
to
f22e0f5
Compare
mrknmc
force-pushed
the
blocking-io-error-catch
branch
from
f22e0f5
to
9fef0b2
Compare
mrknmc
changed the title
datadog/dogstatsd/base.py
Outdated
| @@ -399,8 +399,11 @@ def _send_to_server(self, packet): | |||
| # dogstatsd is overflowing, drop the packets (mimicks the UDP behaviour) | |||
| pass | |||
| except (socket.error, socket.herror, socket.gaierror) as se: | |||
| log.warning("Error submitting packet: {}, dropping the packet and closing the socket".format(se)) | |||
| self.close_socket() | |||
| if se.errno == 11: | |||
There was a problem hiding this comment.
This should be errno.EAGAIN.
The number is different depending on the platform.
I'd also prefer to see that under a specific socket.error except statement to be clearer. Catching everything like this is done is not a good thing, we should make clear things a bit.
There was a problem hiding this comment.
Good point on errno.EAGAIN.
What do you mean by a specific socket.error except statement? I can create a separate statement but if we want to keep the logic the same for non-EAGAIN errors, I'd have to add the close_socket logic there as well.
There was a problem hiding this comment.
I mean:
except socket.error:
if not EAGAIN:
close()
except (socket.herror, socket.gaierror):
close()in summary :)
tests/unit/dogstatsd/test_statsd.py
Outdated
|
|
||
| def send(self, payload): | ||
| error = socker.error("Socker error") | ||
| error.errno = 11 |
|
/azp run DataDog.datadogpy.integration |
|
Azure Pipelines successfully started running 1 pipeline(s). |
@mrknmc Many thanks for following up on our comments! |
No problem, thanks for looking over it! :) |
|
@mrknmc Sorry almost ready to go, but the branch is now out of sync with the master branch. Can you update it and we should be good to go. |
Are you ok with a rebase or do you want me to merge |
I would say the latter, merge |
|
/azp run DataDog.datadogpy.integration |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@dabcoder we should be good to go now, correct? |
|
Yes all good thanks |
jirikuncar
added
changelog/Fixed
zippolyte
changed the title
What does this PR do?
Fixes #514 by catching
socket.errorand checkingerrno == 11(EAGAIN) and logs a warning without closing the socket.Description of the Change
As mentioned in the issue,
socket.sendraises aBlockingIOError (socket.error on py2)exception when the socket is "full" rather than asocket.timeoutexception. Hence, we create a new branch in the try-raise where we catchsocket.error, check theerrnoand log a warning without closing the socket. This works across both python 2 and 3.I left the
socket.timeoutbranch in place because I wasn't sure if there is another way to set up DogStatsd wheresocket.timeoutis thrown when the socket is full. This way, the change shouldn't break compatibility for people who rely on that behaviour.Alternate Designs
I considered not logging a message when a
socket.errorwitherrno == 11occurs but thought that people might want to be informed about this happening.Possible Drawbacks
N/A
Verification Process
I created a unit test and checked that it passes.
Additional Notes
N/A
Release Notes
N/A
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.