Fix statsd Logging in Python 3 #1010

Merged
merged 1 commit into from Apr 30, 2015

Projects

None yet

3 participants

@krallin
Contributor
krallin commented Apr 20, 2015

statsd logging appears to be (silently) broken in Python 3, because it passes it passes unicode (i.e. not bytes) to the sock.send method.

This PR:

  • Ensures bytes are used
  • Adds tests that actually go through an actual (UNIX) socket to ensure those errors can be caught in uni testing in the future.
  • Logs statsd errors as warnings (the exceptions being silently swallowed made them difficult to troubleshoot)
@berkerpeksag berkerpeksag commented on the diff Apr 21, 2015
gunicorn/glogging.py
@@ -224,8 +224,8 @@ def info(self, msg, *args, **kwargs):
def debug(self, msg, *args, **kwargs):
self.error_log.debug(msg, *args, **kwargs)
- def exception(self, msg, *args):
- self.error_log.exception(msg, *args)
+ def exception(self, msg, *args, **kwargs):
@berkerpeksag
berkerpeksag Apr 21, 2015 Collaborator

Good catch!

@berkerpeksag berkerpeksag commented on an outdated diff Apr 21, 2015
gunicorn/instrument/statsd.py
def histogram(self, name, value):
+ self._sock_send("{0}{1}:{2}|ms".format(self.prefix, name, value))
+
+ # Internal methods
@berkerpeksag
berkerpeksag Apr 21, 2015 Collaborator

I'd remove this comment since _spam names are considered private/internal.

@berkerpeksag berkerpeksag commented on an outdated diff Apr 21, 2015
tests/test_010-statsd.py
- self.msgs.append(msg)
+
+ sock_dir = tempfile.mkdtemp()
+ sock_file = os.path.join(sock_dir, "test.sock")
+
+ server = socket.socket(socket.AF_UNIX, socket.SOCK_DGRAM)
+ server.bind(sock_file)
+
+ client = socket.socket(socket.AF_UNIX, socket.SOCK_DGRAM)
+ client.connect(sock_file)
+
+ client.send(msg)
+
+ self.msgs.append(server.recv(1024))
+
+ client.close()
@berkerpeksag
berkerpeksag Apr 21, 2015 Collaborator

Can you wrap these three cleanup lines with a try...finally clause?

@krallin
Contributor
krallin commented Apr 21, 2015

@berkerpeksag ,

Fixed the two issues you pointed out. Note that if server.bind(sock_file) or client.connect(sock_file) fail, the tests might end up closing a socket that was never open, but that shouldn't raise an exception anyway.

Cheers,

@berkerpeksag
Collaborator

LGTM (thanks for squashing commits by the way!).

My only question is that if we test the statsd logging against a real socket, shouldn't we use a different class since the class' name is "MockSocket"? In theory, we should be able to test whether the input is str or not by tweaking the MockSocket.send() method (without creating a real socket).

Ping @benoitc @tilgovi.

@krallin
Contributor
krallin commented Apr 21, 2015

@berkerpeksag

The idea behind using an actual socket is to catch similar errors in unit tests in the future (if they can even exist).

This time, it was a text vs bytes issue, but maybe another issue will crop up some day (I have no idea what that issue might be, but after all that's why we write tests!).

I'm fine with renaming the class, though — let me know!

Cheers,

@benoitc benoitc commented on the diff Apr 30, 2015
gunicorn/instrument/statsd.py
try:
+ if isinstance(msg, six.text_type):
+ msg = msg.encode("ascii")
@benoitc
benoitc Apr 30, 2015 Owner

why not reusing the bytes_to_str function from the _compat module instead?

@krallin
krallin Apr 30, 2015 Contributor

@benoitc

I think we'd need str_to_bytes instead, wouldn't we?

Looking at the definition for bytes_to_str, I'm a bit unclear on what it's supposed to returns. It looks like it gives you bytes on Py2 and text Py3? I'm probably misreading though.

Cheers,

@benoitc
benoitc Apr 30, 2015 Owner

mmm you're right should probably be the reverse in your case. anyway could be useful to have it in a _compat module, so we don't have to do such things on python 2.x. thoughts?

@krallin
krallin Apr 30, 2015 Contributor

Makes sense. Can you just confirm what those methods should return to clear up the confusion above?

@berkerpeksag
berkerpeksag Apr 30, 2015 Collaborator

If we are going to add a function like this, we need to make it more general(e.g. you can pass different encodings and error handlers). util.to_bytestring is another alternative, but it uses the UTF-8 encoding by default. I'd say merge this now and create a new issue or PR to discuss details.

@benoitc
benoitc Apr 30, 2015 Owner

+1 let's merge now and handle the rest separately.
On Thu 30 Apr 2015 at 12:44 Berker Peksag notifications@github.com wrote:

In gunicorn/instrument/statsd.py
#1010 (comment):

     try:
  •        if isinstance(msg, six.text_type):
    
  •            msg = msg.encode("ascii")
    

If we are going to add a function like this, we need to make it more
general(e.g. you can pass different encodings and error handlers).
util.to_bytestring is another alternative, but it uses the UTF-8 encoding
by default. I'd say merge this now and create a new issue or PR to discuss
details.


Reply to this email directly or view it on GitHub
https://github.com/benoitc/gunicorn/pull/1010/files#r29419336.

@krallin
Contributor
krallin commented Apr 30, 2015

Do you guys need me to rebase or squash further or anything? Just let me know : )

@berkerpeksag
Collaborator

@krallin if you have time, please squash the commits. If not, I can do it today :) Thanks!

@krallin krallin Fix statsd logging to work on Python 3
Bytes must be passed to socket.send.

Update tests to ensure an actual socket is used, so that errors like
this can be caught in unit tests in the future.
410bcfa
@krallin
Contributor
krallin commented Apr 30, 2015

@berkerpeksag Sure — done

@berkerpeksag berkerpeksag merged commit a7354cf into benoitc:master Apr 30, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@berkerpeksag
Collaborator

Thanks!

@krallin krallin deleted the krallin:fix-statsd-py3 branch May 1, 2015
@urbaniak urbaniak referenced this pull request Jun 29, 2015
Closed

release now? #1057

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment