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

Add approximate usec percentage per command #1620

Open
wants to merge 1 commit into
base: unstable
Choose a base branch
from

Conversation

bluayer
Copy link
Contributor

@bluayer bluayer commented Jan 27, 2025

Background

#1619

Changes

  • Add a function for calculating total command usec time
  • With total usec, caculate approximate usec percentage
    • Don't need to use floating point-level accuracy because it's just hint for identifying occupied commands.

Sample screenshot

Screenshot 2025-01-27 at 12 27 24 PM

Concern

A recursive function is used when calculating the total command time, but there may be a better way. Please give me feedback.

- Add a function for calculating total command usec time
- With total usec, caculate approximate usec percentage
  - Don't need to use floating point-level accuracy because it's just hint for identifying occupied commands.

Signed-off-by: bluayer <bluayer@gmail.com>
Copy link

codecov bot commented Jan 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.99%. Comparing base (9071a5c) to head (d1213dd).
Report is 1 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1620      +/-   ##
============================================
+ Coverage     70.85%   70.99%   +0.13%     
============================================
  Files           121      121              
  Lines         65169    65183      +14     
============================================
+ Hits          46176    46274      +98     
+ Misses        18993    18909      -84     
Files with missing lines Coverage Δ
src/server.c 87.67% <100.00%> (+0.05%) ⬆️

... and 14 files with indirect coverage changes

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.

Thanks for the contribution, but I don't think we want to accept the feature idea. This data can be pretty easily derived on the client side, and all it does is take up server side CPU computation. (and a division at that, which can be expensive) It will also add more data to the output. So unless someone else thinks it's a good idea I think we can close this.

@bluayer
Copy link
Contributor Author

bluayer commented Jan 27, 2025

I understand what direction Valkey is.
I've currently two ideas while creating this PR, and I'd like to hear your opinions.

1/ What if we calculate usec_per_call as decimal(not float)? In most use cases, more precise time than microseconds is not required.

2/ There was an idea that it would be good if stat and info also had a level, like the log level. For example, at the production level, dashboards are usually configured separately, so there is no need to provide statistics in a way that is easy for people to see.

@zuiderkwast
Copy link
Contributor

We already have complaints that INFO is a performance problem and we want to avoid making it slower and increase the size of the reply. Computing derived information can be done on the client side, or using EVAL if you want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants