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 new SCRIPT STATS subcommand #1418

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

Conversation

artikell
Copy link
Contributor

An inspiration is needed to query the execution of the current script. Solve this problem https://github.com/orgs/valkey-io/discussions/1404

Copy link

codecov bot commented Dec 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.89%. Comparing base (4f61034) to head (b103f0f).
Report is 26 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1418      +/-   ##
============================================
+ Coverage     70.85%   70.89%   +0.03%     
============================================
  Files           118      118              
  Lines         63591    63627      +36     
============================================
+ Hits          45058    45108      +50     
+ Misses        18533    18519      -14     
Files with missing lines Coverage Δ
src/commands.def 100.00% <ø> (ø)
src/eval.c 57.69% <100.00%> (+0.67%) ⬆️

... and 12 files with indirect coverage changes

@madolson madolson added the major-decision-pending Major decision pending by TSC team label Dec 17, 2024
Comment on lines +756 to +758
addReplyMapLen(c, 1);

addReplyBulkCString(c, "running_script");
Copy link
Member

Choose a reason for hiding this comment

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

Is the thought we will have more stats in the future? I'm wondering if we should just have a command just for the running script, instead of something generic like stats.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, this part is a reference to FUNCTION STATS.
Currently, there is really only one return value., but as a stat method, it is best to reserve the ability for subsequent extensions.
Alternatively, we can consider adding some Lua-related context, such as Lua execution time/ Lua use Memory, to help users locate problems.

Copy link
Member

@madolson madolson Dec 20, 2024

Choose a reason for hiding this comment

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

Why the f$% did the Redis folks do this. I swear they barely thought through functions and then dumped it on the community.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-decision-pending Major decision pending by TSC team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants