Add user/client name to MONITOR and SLOWLOG

[guybe7]

No description provided.

[guybe7]

pushed -f to support module commands calling RM_Call

[guybe7]

@antirez if we want to see the real user and connection for modules (like we do for lua), specifically for threads and async module clients, we need to add a module_caller in the server struct (or maybe in the client struct? due to threading). do you want me to write such code?

other than that, this PR invalidates #2877

[antirez]

Hi @guybe7, during the previous meeting on Slack we said we want to design this with more efforts before making breaking changes. Thanks.

[oranagra]

@antirez i was under the impression that you didn't care about breaking the monitor format. if we want we can easily avoid it by adding an argument like MINITOR WITH-NAMES or something alike.

anyway, what design concerns do you think are uncovered by this change? let's discuss them, and make the design and changes before we ship the final 6.0.

[hradilf]

Hello, please, is there any estimation when this can be clarified and/or merged? Thanks.

[yossigo]

@oranagra I think it would make sense to avoid breaking MONITOR and make it optional, although having replicationFeedMonitors() adapt the output per client might not be ideal.

[oranagra]

@yossigo i completely agree, but i got a different impression from Salvatore (saying he wants to have the new info as default and doesn't care much about monitor backwards compatibility).
Re-reading this conversation i think i may have missed the final word, and it seems he eventually agreed to make it optional. @guybe7 do you want to handle that?

[guybe7]

@antirez this feature is now optional with MONITOR CLIENTINFO
if and when merged i will update redis.io

[antirez]

Folks, this is too hardly under-designed. It does not make sense to have an option to just enable a single additional field. This should be part of a serious redesign. I think we can still do that during the Redis 6 development, but we need higher standards, a design document, and so forth, and understand now if we want to change format completely. IMHO we should have something like a "FULL" option or alike that will output filed="value" or alike so that the design is flexible and no longer positional.

[oranagra]

@antirez i would like to propose that we add 3 new arguments to MONITOR:

1. FORMAT [HUMAN / MACHINE] - human is just space separated list of lines. and machine can be either RESP map, or a field value list like the one in CLIENT LIST. (default is HUMAN, i.e. the old pre-redis 6.0 format)
2. LOG [COMMANDS / ERRORS] - by default it'll log just COMMANDS, but once the user supplies this argument, he can ask for only ERRORS, or both commands and errors (by repeating the LOG argument twice). we don't have to implement this yet, but let's make it part of the design.
3. SHOW [DEFAULT / FULL / USERNAME / CLIENTNAME] - the DEFAULT will be the old info, FULL will include whatever we add in the future (each version can change it), but i think we may also want to let users specifically choose what to show, this way we can add something insane (like full info string) which most users don't want there (we won't even include it in FULL).

please let me know if you like that direction, or which parts of it you don't like, and we can progress from there.
Regarding the FORMAT argument, do you prefer RESP map or field=value lines?

p.s. one more unrelated feature that can be useful (mainly for developers debugging redis under fuzzers) is to add is a way (CONFIG) that makes monitors be flushed by the signal handler (so on crush, if you had a monitor you have all the commands leading to the crash), and even an option to flush monitors in call (maybe some real use cases with low traffic can find that useful too).

[antirez]

Thanks, postponed to Redis 7, no room for Redis 6 that is due in 1 month. I think this should not be a "last minute" change.

[itamarhaber]

Also related #4195 and #3592

[soloestoy]

moreover, I'd like to have more arguments:

1. COMMANDS [GET|SET...]
2. DB <dbid>
3. KEYS [key1|key2...]

then users can user monitor as a temporary audit method.

[madolson]

@soloestoy Not sure about commands and keys, since they can be inferred from the command (except lua, would we do declared keys or the actually touched keys?) DB is a great idea though.

[oranagra]

if / when we'll ever redesign the MONITOR output, here's another one to consider in the new design:
#7629 (Denote elapsed time and response size)

[soloestoy]

Hi @madolson

Not sure about commands and keys, since they can be inferred from the command

Sometimes users wanna find who(ip:port) modify specific keys via specific commands, I think if we can support monitor filter it would be very useful in this scenario.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}