Skip to content

fdctl: add monitor --topo flag#8659

Open
ripatel-fd wants to merge 1 commit intofiredancer-io:mainfrom
riptl:monitor-topo-flag
Open

fdctl: add monitor --topo flag#8659
ripatel-fd wants to merge 1 commit intofiredancer-io:mainfrom
riptl:monitor-topo-flag

Conversation

@ripatel-fd
Copy link
Contributor

Allows running monitor for custom topologies

Allows running monitor for custom topologies
Copilot AI review requested due to automatic review settings March 3, 2026 05:03
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds support for selecting an alternate predefined topology when running fdctl monitor, enabling the monitor UI to attach to non-default topologies.

Changes:

  • Extend monitor command args with a topo string buffer to store the selected topology name.
  • Parse a new --topo flag in monitor and reconstruct config->topo by looking up the named action in ACTIONS[].
  • Invoke topology reconstruction at the start of monitor_cmd_fn.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/app/shared/fd_action.h Adds monitor.topo storage to args_t so the selected topology name can be passed through execution.
src/app/shared/commands/monitor/monitor.c Parses --topo, looks up the matching action topology via ACTIONS[], and rebuilds config->topo before starting the monitor.

char const * topo_name = fd_env_strip_cmdline_cstr( pargc, pargv, "--topo", NULL, "" );

ulong topo_name_len = strlen( topo_name );
if( FD_UNLIKELY( topo_name_len > sizeof(args->monitor.topo)-1 ) ) FD_LOG_ERR(( "Unknown --topo %s", topo_name ));
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

The length check for --topo reports "Unknown --topo", but the actual failure mode here is that the provided string is too long for args->monitor.topo. This is misleading for users and also logs the full (potentially very large) input string. Consider changing the message to something like "--topo too long (max N)" and/or truncating the logged value to the buffer size.

Suggested change
if( FD_UNLIKELY( topo_name_len > sizeof(args->monitor.topo)-1 ) ) FD_LOG_ERR(( "Unknown --topo %s", topo_name ));
ulong max_topo_len = sizeof(args->monitor.topo) - 1UL;
if( FD_UNLIKELY( topo_name_len > max_topo_len ) )
FD_LOG_ERR(( "--topo too long (max %lu chars)", max_topo_len ));

Copilot uses AI. Check for mistakes.
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.

4 participants