Feature COMMANDLOG to record slow execution and large request/reply

[soloestoy]

As discussed in PR #336.

We have different types of resources like CPU, memory, network, etc. The slowlog can only record commands eat lots of CPU during the processing phase (doesn't include read/write network time), but can not record commands eat too many memory and network. For example:

1. run "SET key value(10 megabytes)" command would not be recored in slowlog, since when processing it the SET command only insert the value's pointer into db dict. But that command eats huge memory in query buffer and bandwidth from network. In this case, just 1000 tps can cause 10GB/s network flow.
2. run "GET key" command and the key's value length is 10 megabytes. The get command can eat huge memory in output buffer and bandwidth to network.

This PR introduces a new command COMMANDLOG, to log commands that consume significant network bandwidth, including both input and output. Users can retrieve the results using COMMANDLOG get <count> large-request and COMMANDLOG get <count> large-reply, all subcommands for COMMANDLOG are:

• COMMANDLOG HELP
• COMMANDLOG GET <count> <slow|large-request|large-reply>
• COMMANDLOG LEN <slow|large-request|large-reply>
• COMMANDLOG RESET <slow|large-request|large-reply>

And the slowlog is also incorporated into the commandlog.

For each of these three types, additional configs have been added for control:

• commandlog-request-larger-than and commandlog-large-request-max-len represent the threshold for large requests(the unit is Bytes) and the maximum number of commands that can be recorded.
• commandlog-reply-larger-than and commandlog-large-reply-max-len represent the threshold for large replies(the unit is Bytes) and the maximum number of commands that can be recorded.
• commandlog-execution-slower-than and commandlog-slow-execution-max-len represent the threshold for slow executions(the unit is microseconds) and the maximum number of commands that can be recorded.
  • Additionally, slowlog-log-slower-than and slowlog-max-len are now set as aliases for these two new configs.

[codecov]

Codecov Report

Attention: Patch coverage is 92.06349% with 10 lines in your changes missing coverage. Please review.

Project coverage is 70.95%. Comparing base (dc9ca1b) to head (c86bc80).
Report is 6 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/commandlog.c	92.79%	8 Missing ⚠️
src/module.c	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1294      +/-   ##
============================================
- Coverage     70.98%   70.95%   -0.04%     
============================================
  Files           120      120              
  Lines         65061    65126      +65     
============================================
+ Hits          46185    46208      +23     
- Misses        18876    18918      +42     

Files with missing lines	Coverage Δ
src/blocked.c	91.38% <100.00%> (ø)
src/commands.def	100.00% <ø> (ø)
src/config.c	78.39% <ø> (ø)
src/latency.c	80.92% <100.00%> (ø)
src/multi.c	97.30% <ø> (ø)
src/networking.c	88.78% <ø> (+0.34%)	⬆️
src/server.c	87.65% <100.00%> (+0.07%)	⬆️
src/server.h	100.00% <ø> (ø)
src/module.c	9.59% <0.00%> (+0.02%)	⬆️
src/commandlog.c	92.79% <92.79%> (ø)

... and 16 files with indirect coverage changes

[hwware]

Just for Note: Let's first refresh the memory in pr #336, the last comment conclusion is:

After a core team meeting, we decided adding a new command COMMANDLOG with subcommands HEAVYTRAFFIC and SLOW, and then slowlog.c can be renamed to a common commandlog.c. #336 (comment)

[hwware]

From my understanding, for the command: commandlog get should be:
COMMANDLOG get count slow | heavytraffic-input | heavytraffic-output.

commandlog len should be:
COMMANDLOG len slow | heavytraffic-input | heavytraffic-output.

commandlog reset should be:
COMMANDLOG reset slow | heavytraffic-input | heavytraffic-output.

Can you describe the terms heavytraffic-input and heavytraffic-output in the json file (argument part) because I can only know
them from the source codes and valkey.conf so far?

And I think the existing slowlog commands should be deprecated? If yes, I think you should update the related json files as well.

[hwware]

LGTM, approved

[zuiderkwast]

For compatibility with other databases, I think we should be extremely careful about deprecating commands. Some users are very careful to not use deprecated commands. I remember when some product team were working on upgrading to Valkey and reading all the release notes of the 7.x versions, they noticed that CLUSTER SLOTS is deprecated, it almost caused them a lot of extra work, until I explained to them that it is no longer deprecated.

We can document that it is an alias of COMMANDLOG, but we don't have to mark it as deprecated.

[soloestoy]

seems we need a "not-recommended" field.

[zuiderkwast]

We can just describe it in the docs in text?

Just curious, why mark as not recommended? Is there a reason to make users move away from the slowlog commands?

[hwware]

From my understanding, the new command 'COMMANDLOG get count slow | heavytraffic-input | heavytraffic-output.' is encouraged in new version, thus the command 'Slowlog' is not suggested to be used anymore.

I think it makes no sense that 2 commands with the same weight implement the same function in a system, they are duplicated.

[zuiderkwast]

OK, so there are pros and cons:

• Avoid duplicate/alias commands. Of course this is good to avoid.
• Keep compatibility with other systems (forks and managed databases) and tools written for them. This is a good thing too.

You say keep compatibility with other forks and tools makes no sense? Or you say we keep SLOWLOG for backward compatibility but it's recommended to use COMMANDLOG instead?

This is fine. I'm just concerned that if we say "deprecated", users will think that we want to remove it and diverge from redis and other forks. This is a bad communication to the community IMO.

I have seen some comments that people are worried that Valkey will diverge from open source Redis versions. If Redis and Valkey diverges, I hope we can safely say it is Redis fault, not our fault. Did you see the recent discussion about the redis-rs Rust library?

[hwware]

OK, so there are pros and cons:

• Avoid duplicate/alias commands. Of course this is good to avoid.
• Keep compatibility with other systems (forks and managed databases) and tools written for them. This is a good thing too.

You say keep compatibility with other forks and tools makes no sense? Or you say we keep SLOWLOG for backward compatibility but it's recommended to use COMMANDLOG instead?

First all, I do not express making no sense to compatible with other forks and tools. I think Valkey should try the best compatible with others. And I would suggest in the new version, user had better choose the COMMANDLOG command instead of SLOWLOG command.

This is fine. I'm just concerned that if we say "deprecated", users will think that we want to remove it and diverge from redis and other forks. This is a bad communication to the community IMO.

Of course, from my personal experience, if I see API noted deprecated, I would like to avoid to use them. Maybe we can find the other words to encourage to use COMMANDLOG command

I have seen some comments that people are worried that Valkey will diverge from open source Redis versions. If Redis and Valkey diverges, I hope we can safely say it is Redis fault, not our fault. Did you see the recent discussion about the redis-rs Rust library?

Yes, I just saw the lots of comments from redis-rs discussion yesterday. But for deprecating an existing command in Redis OSS, I think we could find a better solution to let our user know to deal with it.

[zuiderkwast]

First all, I do not express making no sense to compatible with other forks and tools.

Sorry! I know you don't.

I think we want the same thing. I just wanted to highlight that there are pros and cons. Maybe I'm overrreacting. Maybe deprecated is right here.

Whatever we call it, deprecated or not recommended, we can describe in the text that there is no plan to remove the slowlog command. It can make worried people less worried maybe.

[hwware]

First all, I do not express making no sense to compatible with other forks and tools.

Sorry! I know you don't.

Do not worry, I know you ^_^

I think we want the same thing. I just wanted to highlight that there are pros and cons. Maybe I'm overrreacting. Maybe deprecated is right here.

Whatever we call it, deprecated or not recommended, we can describe in the text that there is no plan to remove the slowlog command. It can make worried people less worried maybe.

In fact, 'not recommended' is much properly than deprecated if we can add it in json file. @soloestoy can we? haha,
I never did it before

[zuiderkwast]

Ok, "not-recommended" or "not-recommended-since" field is not bad, haha. We can keep replaced-by too.

It has no effect in any tools, but we can use it for the website. We just need to implement some code for it in website repo.

[soloestoy]

I'm ok with "not-recommended", I think we can open another PR to implement and discuss which commands should be changed from "deprecated" to "no-recommended".

[zuiderkwast]

@valkey-io/core-team Let's make a TSC decision on the command syntax and configs as described in the top comment.

Approve or vote 👍 or 👎.

[madolson]

Mostly LGTM, still agree with high level direction.

[enjoy-binbin]

overall LGTM, didn't review the code carefully in depth though.