traffic_ctl: Add --append option to append debug tags instead of replacing them. (inc ArgParser support).
#12804
Conversation
…le usages - Add with_required() method to specify that an option requires another - Change add_example_usage() to support multiple examples per command - Add unit tests for option dependencies - Update ArgParser documentation
… tags - Add --append/-a option to append debug tags instead of replacing them - Uses ArgParser's with_required() to enforce --tags dependency - Add autest for server debug enable/disable commands - Update traffic_ctl documentation
brbzull0
added
Tools
Cleanup
Documentation
traffic_ctl
brbzull0
changed the title
--append option to append debug tags instead of replacing them. (inc ArgParser support).
There was a problem hiding this comment.
Pull request overview
This PR adds a new --append option to the traffic_ctl server debug enable command that allows appending debug tags to existing tags instead of replacing them. It also enhances the ArgParser library with option dependency support via a new with_required() method and updates add_example_usage() to support multiple examples per command.
Changes:
- Added
with_required()method to ArgParser for specifying option dependencies - Modified
add_example_usage()to support multiple example usage strings - Implemented
--append/-aflag intraffic_ctl server debug enablecommand with proper dependency on--tags
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| include/tscore/ArgParser.h | Added with_required() method declaration, changed _example_usage to _example_usages vector, added dependency tracking fields |
| src/tscore/ArgParser.cc | Implemented with_required() and validate_dependencies() methods, updated example usage handling, added dependency info to help output |
| src/tscore/unit_tests/test_ArgParser_OptionDependencies.cc | Comprehensive unit tests for option dependency feature |
| src/tscore/CMakeLists.txt | Added new unit test file to build |
| src/traffic_ctl/traffic_ctl.cc | Added --append option with with_required("--tags") and multiple example usages |
| src/traffic_ctl/CtrlCommands.h | Added APPEND_STR constant |
| src/traffic_ctl/CtrlCommands.cc | Implemented tag appending logic in server_debug() |
| tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py | Added Debug class for testing debug commands |
| tests/gold_tests/traffic_ctl/traffic_ctl_server_debug.test.py | New autest for debug enable/disable with append functionality |
| tests/gold_tests/traffic_ctl/gold/test_2.gold | Removed (no longer needed) |
| tests/gold_tests/traffic_ctl/gold/test_3.gold | Removed (no longer needed) |
| doc/developer-guide/internal-libraries/ArgParser.en.rst | Documented option dependencies feature |
| doc/appendices/command-line/traffic_ctl.en.rst | Documented --append option with examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // If append mode is enabled and tags are provided, fetch current tags and combine | ||
| if (append && !tags.empty()) { | ||
| shared::rpc::RecordLookupRequest lookup_request; | ||
| lookup_request.emplace_rec("proxy.config.diags.debug.tags", shared::rpc::NOT_REGEX, shared::rpc::CONFIG_REC_TYPES); | ||
| auto lookup_response = invoke_rpc(lookup_request); | ||
|
|
||
| if (!lookup_response.is_error()) { | ||
| auto const &records = lookup_response.result.as<shared::rpc::RecordLookUpResponse>(); | ||
| if (!records.recordList.empty()) { | ||
| std::string current_tags = records.recordList[0].currentValue; | ||
| if (!current_tags.empty()) { | ||
| // Combine: current|new | ||
| tags = current_tags + "|" + tags; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The append logic doesn't check for duplicate tags. If a user appends a tag that already exists in the current tags, it will be duplicated (e.g., "http|dns|http"). Consider adding deduplication logic to prevent duplicate tags in the combined string. This could be done by splitting the current and new tags, combining them into a set to remove duplicates, and then joining them back together.
| # Test 11: Verify all tags are present | ||
| traffic_ctl.config().get("proxy.config.diags.debug.tags").validate_with_text("proxy.config.diags.debug.tags: cache|http|dns") |
There was a problem hiding this comment.
The test expects tags in a specific order (cache|http|dns), but the current implementation simply concatenates tags without any guaranteed ordering. If the internal representation stores tags differently or if deduplication logic is added in the future, this test might become fragile. Consider using a more flexible validation approach that checks for the presence of all expected tags regardless of order, or document that tag order is guaranteed to be preserved.
| # Test 11: Verify all tags are present | |
| traffic_ctl.config().get("proxy.config.diags.debug.tags").validate_with_text("proxy.config.diags.debug.tags: cache|http|dns") | |
| # Test 11: Verify all tags are present (order-independent) | |
| tr = Test.AddTestRun("verify all debug tags are present regardless of order") | |
| tr.Processes.Default.Env = traffic_ctl._ts.Env | |
| tr.Processes.Default.Command = "traffic_ctl config get proxy.config.diags.debug.tags" | |
| tr.Processes.Default.ReturnCode = 0 | |
| tr.Processes.Default.Streams.All = Testers.ContainsExpression( | |
| r"proxy.config.diags.debug.tags:\s*(?=.*cache)(?=.*http)(?=.*dns).*", | |
| "Should include cache, http, and dns tags regardless of order" | |
| ) | |
| tr.StillRunningAfter = traffic_ctl._ts |
| } | ||
| } |
There was a problem hiding this comment.
When the RPC lookup fails or returns an empty record list in append mode, the code silently falls back to using only the new tags. This might be confusing for users who expect append behavior. Consider logging a warning or providing feedback to the user that append mode couldn't fetch current tags and is falling back to replace mode.
| } | |
| } | |
| } | |
| } else if (_printer) { | |
| _printer->write_output( | |
| "Warning: append mode enabled, but no existing debug tags could be retrieved; using only new tags." | |
| ); | |
| } | |
| } else if (_printer) { | |
| _printer->write_output( | |
| "Warning: append mode enabled, but failed to fetch existing debug tags; using only new tags." | |
| ); |
| const std::string &req_key = req_it->second.key; | ||
|
|
||
| if (!ret.get(req_key)) { | ||
| std::string error_msg = "Option '" + dependent + "' requires '" + required + "' to be specified"; |
There was a problem hiding this comment.
The error message for dependency violations is inconsistent with other validation error messages. Mutex group validation errors (lines 574 and 586) prefix the message with "Error: ", but this dependency validation error does not. Consider adding the "Error: " prefix for consistency.
| std::string error_msg = "Option '" + dependent + "' requires '" + required + "' to be specified"; | |
| std::string error_msg = "Error: Option '" + dependent + "' requires '" + required + "' to be specified"; |
2 participants
ArgParser
Changes needed to implement the
traffic_ctlfeature.with_required()method to specify that an option requires anotheradd_example_usage()to support multiple examples per commandtraffic_ctl server debug: new
--append/-aoption when setting tags.Current implementation wipes what's on the debug tags and set the new one, we now have the option to append to the existing tags.
-append/-aoption to append debug tags instead of replacing themwith_required()to enforce--tagsdependencyenable/disablecommandstraffic_ctldocumentationThere are some cleanup as well, as this removes two gold files that aren't required as they are generated by the test.