Monitor command

[RoeyPrat]

This is a rebase of #194 by @dougk7.
He implemented the MONITOR command in a similar way to PubSub, using a generator to get the messages.
I also fixed some PEP8 and formatting issues and added the command to README.
This issue was also mentioned in #805.
Thanks!

[itamarhaber]

LGTM. @RoeyPrat question: does it make sense to have a close()/exit thing here to release the connection?

[andymccurdy]

• There's no connection cleanup or flow control here. Presumably someone isn't going to monitor all commands forever. There should be some way for a user to say "OK, I'm done monitoring." Such a method would likely call self.connection.disconnect and return self.connection to the connection_pool. Other parts of the library have a reset() method that does this.

• Is there a benefit in separating the Monitor object init from sending the MONITOR command? What's the reasoning that I have to start this by calling client.monitor().monitor() rather than just client.monitor()?

• This might be a great use case for a context manager. The connection can be cleaned up automatically in __exit__. Maybe something like this:

with client.monitor() as m:
    for command in m.listen():
        ...
        if <some behavior indicates that we want to stop>:
            m.stop()  # maybe this trips a flag that controls the while loop in listen()

• The unit tests need to be written in py.test format like the rest of the test suite. Your tests weren't run and I suspect that if they did, they'd fail, because...

• In parse_response() you're comparing a potential bytes response to a string. That will fail in Python 3 unless decode_responses=True. Instead, use the bool_ok response handler (you can invoke it directly) to get a boolean.

• Also in parse_response() you're splitting a potential bytes string (the command) with a string (' '). That will fail in Python 3 unless decode_responses=True. We should split the command on ' ' or b(' ') depending on the value of decode_responses.

• Should we also parse out the client section ([0 127.0.0.1:47832]) of the response into a separate field in the dict returned to the user?

• It might also be nice to have another field in the response called command_name. This would always be a native string that's the command name that was executed, e.g., 'ZADD' or 'CONFIG GET'. Note that this could be two words. If we did this, it shouldn't remove the command name from the command field. I don't see this as critical and am happy to merge without this. But it could be quite nice for a user.

[RoeyPrat]

@andymccurdy I changed monitor to use context manager. also, the message is parsed.

[andymccurdy]

I'd suggest reading the response off the connection and checking it with bool_ok here rather than at the top of next_command.

If the response is not OK, I think we should raise a RedisError (or a subclass) rather than an AssertionError.

[RoeyPrat]

ok. I'm setting error to RedisError('MONITOR failed: %s' % response)

[andymccurdy]

This is going to fail on Python3 with decode_responses=False. I'm looking at another solution for this now...

[RoeyPrat]

for now, I'm setting "If decode_responses=False, return monitor output without parsing".

[RoeyPrat]

I still need to create tests for this feature...

[andymccurdy]

@RoeyPrat I took a look at this a bit yesterday and found some issues.

1. When inspecting the response, it looks like the command key is set to just the first word of the command name. For instance, when another client calls GET foo, the command key within the monitor dict is simply ('GET',). command should either contain the full command, including all the args, or we should have a separate field called command_name that is just GET. And whatever we choose, this should just be a string, not a tuple.

2. As mentioned in a comment above, some command names are two words. The current regex only parses the first word and leaves the rest behind.

3. I think the db field should be an int rather than a string. We represent it as an int everywhere else in the client.

4. We're not taking any steps to verify that the response we're getting back from next_command corresponds to the sample command being issued in the test (in your test case, PING). In fact, the response from next_command() in your test actually describes a SELECT command which the connection is running automatically just after the socket is connected. If the test database is shared with other users or processes, we could actually see activity from those as well.

5. I discovered the above issue while trying to write a test that shows how the current implementation fails with certain encoding issues. I'm trying to get around the test limitations now and will update this when I do.

[andymccurdy]

@RoeyPrat I took your PR and made some changes attempting to fix the problems I outlined above. The branch is here: https://github.com/andymccurdy/redis-py/tree/monitor

I think everything works as expected with this. Can you review and let me know what you think? Thanks!

[RoeyPrat]

@andymccurdy - looks great! I see no problem in your branch.

[andymccurdy]

Merged. Thanks @RoeyPrat