Add support for using Redis MONITOR inputs files as the commands replayed during memtier execution#330
Add support for using Redis MONITOR inputs files as the commands replayed during memtier execution#330fcostaoliveira wants to merge 1 commit intomasterfrom
Conversation
… during memtier execution
|
cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } | ||
| fprintf(stderr, "\n"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Debug code dumps raw responses to stderr
High Severity
Debug code that dumps raw response bytes to stderr was left in parse_response(). This code runs for every Redis response when debug mode is enabled (-D), printing up to 512 bytes of raw response data with a "RAW RESPONSE" prefix. The comment explicitly says "Debug: dump raw response buffer" indicating this was for development. Other debug output in this file uses benchmark_debug_log, but this uses direct fprintf(stderr, ...). This would produce extremely verbose output overwhelming users who enable debug mode.
| } else { | ||
| fprintf(stderr, "error: failed to parse arbitrary command.\n"); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
Monitor placeholders without --monitor-input not validated
Medium Severity
Using monitor placeholders like __monitor_c1__ without --monitor-input causes the literal placeholder string to be sent to Redis instead of producing an error. When parsing --command, monitor placeholders are recognized but only expanded when cfg.monitor_input is set. Without it, the placeholder passes through to format_arbitrary_command unchanged, resulting in Redis receiving an invalid command like __monitor_c1__.
Additional Locations (1)
| unsigned long long key_index; | ||
| get_key_response res = get_key_for_conn(command_index, conn_id, &key_index); | ||
| assert(res == available_for_conn); | ||
| cmd_size += m_connections[conn_id]->send_arbitrary_command(arg, m_obj_gen->get_key(), m_obj_gen->get_key_len()); |
There was a problem hiding this comment.
Key prefix/suffix handling missing in monitor command path
Medium Severity
The new monitor command handling path for key_type arguments doesn't check has_key_affixes, unlike the original code path at lines 351-386. When a monitor command contains a key argument with prefix/suffix like user:__key__:profile, the prefix and suffix are lost, sending only the generated key. The original path properly combines data_prefix, the generated key, and data_suffix.
| } | ||
| size_t random_index = rand() % commands.size(); | ||
| return commands[random_index]; | ||
| } |
There was a problem hiding this comment.
Unused function overload get_random_command() without parameter
Low Severity
The get_random_command() overload without the out_index parameter is declared and implemented but never called anywhere in the codebase. Only the overloaded version get_random_command(size_t* out_index) is used in client.cpp. This dead code should be removed to reduce maintenance burden.
Additional Locations (1)
| key_type = 1, | ||
| data_type = 2, | ||
| undefined_type = 3 | ||
| monitor_type = 3, |
There was a problem hiding this comment.
Unused monitor_type enum value defined but never referenced
Low Severity
The monitor_type enum value is defined in command_arg_type but is never used anywhere in the codebase. Only monitor_random_type is actually used. This unused enum value adds confusion about the intended design and should be removed if not needed.
| command_arg_type type; | ||
| std::string data; | ||
| // For monitor_type, stores the index (1-based) | ||
| size_t monitor_index; |
There was a problem hiding this comment.
Unused monitor_index field only written never read
Low Severity
The monitor_index field in command_arg struct is added and initialized to 0 in the constructor, and assigned to 0 in memtier_benchmark.cpp, but it is never read anywhere. This is a dead store that adds unnecessary complexity to the data structure without providing any functionality.
1 participant
This PR introduces first-class support for driving
memtier_benchmarkworkloads from RedisMONITORoutput files.It adds:
--monitor-input=FILEto load commands captured viaredis-cli MONITOR--monitor-pattern={S|R}to control runtime command selection (Sequential by default, or Random)__monitor_c*__placeholder family for--command:__monitor_c1__,__monitor_c2__, … to replay a specific command from the monitor file__monitor_c@__to select commands at runtime (sequential or random)This enables replaying real traffic and mixing captured workloads with synthetic commands using
--command-ratio, while remaining compatible with both standalone and Redis Cluster deployments.User-facing behavior
Replay a specific captured command on every request:
Runtime-selected replay (sequential by default, random with --monitor-pattern=R):
Documentation
MONITORoutput usingredis-cliMONITORdocumentationNote
Medium Risk
Adds new CLI surface and alters arbitrary-command request generation to dynamically load/expand commands from files, which can affect workload behavior and runtime stability/perf. Also introduces extra debug logging in the Redis protocol parser and adjusts URI cleanup, so regressions would show up at runtime rather than compile time.
Overview
Adds first-class support for driving
--commandworkloads from RedisMONITORoutput via--monitor-input, including__monitor_cN__placeholders (expanded at startup) and__monitor_c@__(expanded per-request, sequential by default or random with--monitor-pattern).Implements a
monitor_command_listfile loader and integrates it into startup parsing/validation, plus runtime selection/formatting inclient::create_arbitrary_request(with cluster-mode single-key checks preserved).Also makes
log_levelglobally visible and adds optional raw RESP response dumping inredis_protocol::parse_responsewhen debug is enabled, updates README documentation, and adds new integration tests covering specific, random, sequential, and mixed monitor-driven workloads.Written by Cursor Bugbot for commit fe67267. This will update automatically on new commits. Configure here.