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

Missing logger accessor attributes #198

Closed
aks opened this issue Nov 11, 2020 · 4 comments
Closed

Missing logger accessor attributes #198

aks opened this issue Nov 11, 2020 · 4 comments

Comments

@aks
Copy link

aks commented Nov 11, 2020

Code that uses redis.logger or redis.logger = other_logger fails when using MockRedis. The emulation should include these attributes.

@sds
Copy link
Owner

sds commented Nov 26, 2020

Happy to accept a pull request!

@jdelStrother
Copy link
Contributor

AFAICT redis-rb doesn't implement logger accessors -

irb(main):007:0> Redis.new.logger
Traceback (most recent call last):
        1: from (irb):7
Redis::CommandError (ERR unknown command `logger`, with args beginning with:)

- I assume you're just asking for the ability to log redis commands as they're executed? eg:

irb(main):011:0> Redis.new(logger: Logger.new(STDOUT)).hgetall "foo"
D, [2021-05-12T16:29:05.939290 #76845] DEBUG -- : [Redis] command=HGETALL args="foo"
D, [2021-05-12T16:29:05.940348 #76845] DEBUG -- : [Redis] call_time=0.80 ms
=> {}

I started to implement it in mock-redis here: jdelStrother@2ba1a14 - though it's kind of tricky to do it cleanly. Redis has the advantage of pushing all commands through the process method, which gives a single entrypoint for logging. In my version I just attach logging to all public methods of the Database modules. In addition, some mock-redis methods are built on top of each other - eg hmget uses hget internally, so a single call to hmget produces multiple logs like this:

D, [2021-05-12T16:35:57.356922 #77623] DEBUG -- : [MockRedis] command=HMGET args="key" "a" "b" "c"
D, [2021-05-12T16:35:57.356943 #77623] DEBUG -- : [MockRedis] command=HGET args="key" "a"
D, [2021-05-12T16:35:57.356943 #77623] DEBUG -- : [MockRedis] command=HGET args="key" "b"
D, [2021-05-12T16:35:57.356943 #77623] DEBUG -- : [MockRedis] command=HGET args="key" "c"

which would be fixable by refactoring hget into public & private parts - eg:

def hget(key, field)
  _hget(key, field)
end
def hmget(key, *fields)
  fields.flatten.map { |f| _hget(key, f) }
end

private
def _hget(key, field)
  with_hash_at(key) { |h| h[field.to_s] }
end

since only the public methods are logged, but that's starting to sound like a lot of tedious work. Any suggestions on a better implementation?

@jdelStrother
Copy link
Contributor

Actually scratch all that, jdelStrother@dde6209 seems a much saner approach. I'll submit a PR for proper review.

@sds
Copy link
Owner

sds commented May 13, 2021

Implemented in #211.

@sds sds closed this as completed May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants