-
Notifications
You must be signed in to change notification settings - Fork 704
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
Feature COMMANDLOG to record slow execution and large request/reply #1294
Conversation
Signed-off-by: zhaozhao.zz <[email protected]>
Signed-off-by: zhaozhao.zz <[email protected]>
Signed-off-by: zhaozhao.zz <[email protected]>
Signed-off-by: zhaozhao.zz <[email protected]>
Signed-off-by: zhaozhao.zz <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #1294 +/- ##
============================================
+ Coverage 70.81% 70.93% +0.12%
============================================
Files 121 121
Lines 65132 65162 +30
============================================
+ Hits 46121 46225 +104
+ Misses 19011 18937 -74
|
Signed-off-by: zhaozhao.zz <[email protected]>
Signed-off-by: zhaozhao.zz <[email protected]>
Signed-off-by: zhaozhao.zz <[email protected]>
Signed-off-by: zhaozhao.zz <[email protected]>
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) |
From my understanding, for the command: commandlog get should be: commandlog len should be: commandlog reset should be: Can you describe the terms heavytraffic-input and heavytraffic-output in the json file (argument part) because I can only know And I think the existing slowlog commands should be deprecated? If yes, I think you should update the related json files as well. |
Signed-off-by: zhaozhao.zz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, approved
Signed-off-by: zhaozhao.zz <[email protected]>
Signed-off-by: zhaozhao.zz <[email protected]>
@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 👎. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM, still agree with high level direction.
Signed-off-by: zhaozhao.zz <[email protected]>
Co-authored-by: Madelyn Olson <[email protected]> Signed-off-by: zhaozhao.zz <[email protected]>
Signed-off-by: zhaozhao.zz <[email protected]>
Signed-off-by: zhaozhao.zz <[email protected]>
Signed-off-by: zhaozhao.zz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall LGTM, didn't review the code carefully in depth though.
Signed-off-by: zhaozhao.zz <[email protected]>
Signed-off-by: zhaozhao.zz <[email protected]>
Co-authored-by: Ping Xie <[email protected]> Signed-off-by: zhaozhao.zz <[email protected]>
Co-authored-by: Ping Xie <[email protected]> Signed-off-by: zhaozhao.zz <[email protected]>
Co-authored-by: Ping Xie <[email protected]> Signed-off-by: zhaozhao.zz <[email protected]>
Co-authored-by: Ping Xie <[email protected]> Signed-off-by: zhaozhao.zz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @soloestoy!
Merged! Thanks every one, the next work is adding "not-recommended" for command's spec. |
…alkey-io#1294) As discussed in PR valkey-io#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. --------- Signed-off-by: zhaozhao.zz <[email protected]> Co-authored-by: Madelyn Olson <[email protected]> Co-authored-by: Ping Xie <[email protected]>
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: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 usingCOMMANDLOG get <count> large-request
andCOMMANDLOG get <count> large-reply
, all subcommands forCOMMANDLOG
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
andcommandlog-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
andcommandlog-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
andcommandlog-slow-execution-max-len
represent the threshold for slow executions(the unit is microseconds) and the maximum number of commands that can be recorded.slowlog-log-slower-than
andslowlog-max-len
are now set as aliases for these two new configs.