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

Input output traffic stats and command process count for each client. #327

Merged
merged 8 commits into from May 6, 2024

Conversation

CharlesChen888
Copy link
Member

@CharlesChen888 CharlesChen888 commented Apr 16, 2024

We already have global stats for input traffic, output traffic and how many commands have been executed.

However, some users have the difficulty of locating the IP(s) which have heavy network traffic. So here some stats for single client are introduced.

tot-net-in   // Total network input bytes read from the client
tot-net-out  // Total network output bytes sent to the client
tot-cmds     // Total commands the client has executed     

These three stats are shown in CLIENT LIST and CLIENT INFO.

Though the metrics are handled in hot paths of the code, personally I don't think it will slow down the server. Considering all other complex operations handled nearby, this is only a small and simple operation.

However we do need to be cautious when adding more and more metrics, as discussed in redis/redis#12640, we may need to find a way to tell whether this has obvious performance degradation.

Signed-off-by: Chen Tianjie <TJ_Chen@outlook.com>
@madolson madolson added the major-decision-pending Needs decision by core team label Apr 17, 2024
Copy link
Member

@madolson madolson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still standing behind my original comment in the Redis issue, which is that I want us to build a more comprehensive observability story figured out for Valkey 8. I believe this will be part of it though.

src/networking.c Outdated Show resolved Hide resolved
src/server.h Outdated Show resolved Hide resolved
src/networking.c Outdated Show resolved Hide resolved
@madolson
Copy link
Member

@valkey-io/core-team Please 👍 and 👎 for this since it's a major issue.

Signed-off-by: Chen Tianjie <TJ_Chen@outlook.com>
Signed-off-by: Chen Tianjie <TJ_Chen@outlook.com>
@madolson madolson added major-decision-approved Approved by core team and removed major-decision-pending Needs decision by core team labels Apr 19, 2024
src/server.c Outdated Show resolved Hide resolved
@madolson madolson added the needs-doc-pr This change needs to update a documentation page label Apr 19, 2024
Signed-off-by: Chen Tianjie <TJ_Chen@outlook.com>
Copy link

codecov bot commented Apr 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (unstable@96d14fe). Click here to learn what that means.

Additional details and impacted files
@@             Coverage Diff             @@
##             unstable     #327   +/-   ##
===========================================
  Coverage            ?   68.40%           
===========================================
  Files               ?      108           
  Lines               ?    61571           
  Branches            ?        0           
===========================================
  Hits                ?    42118           
  Misses              ?    19453           
  Partials            ?        0           
Files Coverage Δ
src/blocked.c 91.86% <100.00%> (ø)
src/networking.c 84.94% <100.00%> (ø)
src/server.c 88.13% <100.00%> (ø)

@madolson
Copy link
Member

madolson commented Apr 29, 2024

OK, besides the one comment about tot-cmds, the change LGTM.

Signed-off-by: Chen Tianjie <TJ_Chen@outlook.com>
Signed-off-by: Chen Tianjie <TJ_Chen@outlook.com>
@CharlesChen888
Copy link
Member Author

@madolson Any more comments? I think this PR is ready to merge.

@madolson madolson merged commit cc703aa into valkey-io:unstable May 6, 2024
17 checks passed
@madolson madolson added the release-notes This issue should get a line item in the release notes label May 6, 2024
madolson pushed a commit to valkey-io/valkey-doc that referenced this pull request May 6, 2024
Corresponding doc PR to valkey-io/valkey#327.

* `tot-net-in`: Total network input bytes read from this client. Added
in Valkey 8.0
* `tot-net-out`: Total network output bytes sent to this client. Added
in Valkey 8.0
* `tot-cmds`: Total count of commands this client executed. Added in
Valkey 8.0

---------

Signed-off-by: Chen Tianjie <TJ_Chen@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-decision-approved Approved by core team needs-doc-pr This change needs to update a documentation page release-notes This issue should get a line item in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants