Re: [PATCH] ethdev: telemetry xstats support hide zero

[fengchengwen <fengchengwen@huawei.com>]

On 2023/2/10 7:30, Ferruh Yigit wrote:
> On 2/9/2023 3:07 AM, Chengwen Feng wrote:
>> The number of xstats may be large, after the hide zero option is added,
>> only non-zero values can be displayed.
>>
> 
> Hi Chengwen,
> 
> No objection on the functionality, we have similar config option in
> testpmd too, but I have some question on telemetry side of things:
> 
> 1) optional parameters, I don't know if there is a defined way to handle
> this in telemetry but with current method only one optional parameter
> can be supported and it has to be the last one. In the feature if a new
> mandatory option required, this changes the user interface. To prevent
> this, is it possible to use "key=value" syntax, like
> "/ethdev/xstats,0,hide_zero=true"
> 
> This way order or existence of multiple options doesn't matter.

Yes, KV (just like PMD's runtime parameters) is more flexible for multiple optional parameters.
AFAIK, only some dmadev commands have optional parameters (which using this patch way to identify).

Until there are more parameters, I think we can keep the status quo.

> 
> 
> 2) Where should be this functionality, it is possible to filter out zero
> values either in dpdk side or telemetry client side.
> Just for this one I think both options are OK, but if there is a
> possibility to have multiple and complex filtering, I think that should
> go to the client side since this is not really task of the dpdk library.

Agree.
This patch just target who using telemetry.py directly, it's valuable in this scenario.
If clients supports filtering, they could use original way (don't add options).

Thanks.

> 
> 
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> ---
>>  lib/ethdev/rte_ethdev.c | 23 +++++++++++++++++------
>>  1 file changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>> index d25db35f7f..8f79ae45d5 100644
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -5870,20 +5870,28 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
>>  {
>>  	struct rte_eth_xstat *eth_xstats;
>>  	struct rte_eth_xstat_name *xstat_names;
>> +	char *end_param, *hide_param;
>>  	int port_id, num_xstats;
>> +	int hide_zero = 0;
>>  	int i, ret;
>> -	char *end_param;
>>  
>>  	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
>>  		return -1;
>>  
>>  	port_id = strtoul(params, &end_param, 0);
>> -	if (*end_param != '\0')
>> -		RTE_ETHDEV_LOG(NOTICE,
>> -			"Extra parameters passed to ethdev telemetry command, ignoring");
>>  	if (!rte_eth_dev_is_valid_port(port_id))
>>  		return -1;
>>  
>> +	if (*end_param != '\0') {
>> +		hide_param = strtok(end_param, ",");
>> +		if (hide_param == NULL || strlen(hide_param) == 0 || !isdigit(*hide_param))
>> +			return -EINVAL;
>> +		hide_zero = strtoul(hide_param, &end_param, 0);
>> +		if (*end_param != '\0')
>> +			RTE_ETHDEV_LOG(NOTICE,
>> +				"Extra parameters passed to ethdev telemetry command, ignoring");
>> +	}
>> +
>>  	num_xstats = rte_eth_xstats_get(port_id, NULL, 0);
>>  	if (num_xstats < 0)
>>  		return -1;
>> @@ -5908,9 +5916,12 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
>>  	}
>>  
>>  	rte_tel_data_start_dict(d);
>> -	for (i = 0; i < num_xstats; i++)
>> +	for (i = 0; i < num_xstats; i++) {
>> +		if (hide_zero != 0 && eth_xstats[i].value == 0)
>> +			continue;
>>  		rte_tel_data_add_dict_uint(d, xstat_names[i].name,
>>  					   eth_xstats[i].value);
>> +	}
>>  	free(eth_xstats);
>>  	return 0;
>>  }
>> @@ -6328,7 +6339,7 @@ RTE_INIT(ethdev_init_telemetry)
>>  	rte_telemetry_register_cmd("/ethdev/stats", eth_dev_handle_port_stats,
>>  			"Returns the common stats for a port. Parameters: int port_id");
>>  	rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
>> -			"Returns the extended stats for a port. Parameters: int port_id");
>> +			"Returns the extended stats for a port. Parameters: int port_id, bool hide_zero (Optional, non-zero indicates hide zero xstats)");
>>  #ifndef RTE_EXEC_ENV_WINDOWS
>>  	rte_telemetry_register_cmd("/ethdev/dump_priv", eth_dev_handle_port_dump_priv,
>>  			"Returns dump private information for a port. Parameters: int port_id");
> 
> .
>