valkey icon indicating copy to clipboard operation
valkey copied to clipboard

Scripting Engine Debugger Support

Open rjd15372 opened this issue 10 months ago • 2 comments

This PR introduces the support for implementing remote debuggers in scripting engines modules.

The module API is extended with scripting engines callbacks and new functions that can be used by scripting engine modules to implement a remote debugger.

Most of the code that was used to implement the Lua debugger, was refactored and moved to the scripting_engine.c file, and only the code specific to the Lua engine, remained in the debug_lua.c file.

The SCRIPT DEBUG (YES|NO|SYNC) command was extend with an optional parameter that can be used to specify the engine name, where we want to enable the debugger. If no engine name is specified, the Lua engine is used to keep backwards compatibility.

In src/valkeymodule.h we see the module API changes. And in the helloscripting.c file we can see how to implement a simple debugger for the dummy HELLO scripting engine.

rjd15372 avatar Feb 10 '25 12:02 rjd15372

Codecov Report

:x: Patch coverage is 46.84685% with 354 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 72.37%. Comparing base (7e0b3bb) to head (28c9b2e). :warning: Report is 3 commits behind head on unstable.

Files with missing lines Patch % Lines
src/scripting_engine.c 48.35% 236 Missing :warning:
src/lua/debug_lua.c 43.07% 74 Missing :warning:
src/module.c 20.83% 19 Missing :warning:
src/eval.c 50.00% 9 Missing :warning:
src/valkey-cli.c 0.00% 9 Missing :warning:
src/lua/engine_lua.c 76.00% 6 Missing :warning:
src/server.c 66.66% 1 Missing :warning:
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1701      +/-   ##
============================================
- Coverage     72.58%   72.37%   -0.22%     
============================================
  Files           128      128              
  Lines         71326    71626     +300     
============================================
+ Hits          51772    51839      +67     
- Misses        19554    19787     +233     
Files with missing lines Coverage Δ
src/commands.def 100.00% <ø> (ø)
src/server.c 88.43% <66.66%> (ø)
src/lua/engine_lua.c 86.48% <76.00%> (-1.64%) :arrow_down:
src/eval.c 87.91% <50.00%> (-1.33%) :arrow_down:
src/valkey-cli.c 54.53% <0.00%> (-1.69%) :arrow_down:
src/module.c 9.83% <20.83%> (+0.04%) :arrow_up:
src/lua/debug_lua.c 37.11% <43.07%> (-2.17%) :arrow_down:
src/scripting_engine.c 53.60% <48.35%> (-19.83%) :arrow_down:

... and 11 files with indirect coverage changes

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Feb 10 '25 12:02 codecov[bot]

Postponing until 9.0.

madolson avatar Feb 10 '25 15:02 madolson

We talked about this in a core team meeting. We said that nobody uses the Lua debugger and that we could basically deprecate it. No meed to make this large change to extend the debugger support. The main reason to do it is to avoid breaking it if we move Lua to a module?

Correct. This is required if we want to keep the debugger feature for Lua when moving Lua to a module. I'm also fine to deprecate the debugger feature, but if that is the case we should deprecate it in 9.0 and raise somekind of warning to the user when he tries to use the debugger.

rjd15372 avatar May 22 '25 10:05 rjd15372

@zuiderkwast I've updated the PR, and addressed your comments. Added new tests and fixed some bugs that appeared in the tests.

rjd15372 avatar Jun 18 '25 16:06 rjd15372

@zuiderkwast I've updated this PR with the current unstable branch. We should get this PR merged so I can open the PR that moves the lua engine into a module.

rjd15372 avatar Sep 29 '25 11:09 rjd15372

@madolson do you want to review the module API additions?

rjd15372 avatar Oct 02 '25 16:10 rjd15372

I believe we can merge this without waiting for the others to review it, so we can unblock the other work. If there are any concerns regarding the module API, we have time to change it until 9.1.

It needs some rebase now and bump the scripting engine API version to 3?

zuiderkwast avatar Oct 22 '25 14:10 zuiderkwast

@zuiderkwast yes, I've fixed the merge conflicts and updated the code accordingly. Run a few more tests and looks good. Will merge this now.

rjd15372 avatar Oct 23 '25 09:10 rjd15372